Infra: Write static topology updates to file system

[oncilla]

Infra services can be instructed to write topology updates to file system by configuring the discovery.static.Filename.
In case of an empty string, the updated topology is not written.

---

This change is 

[oncilla]

a discussion (no related file):
Disregard first commit. (cherry-pick of pr #2429 )

---

[oncilla]

a discussion (no related file):
full run: https://buildkite.com/scionproto/scionproto/builds/1891

---

[scrye]

Reviewed 9 of 9 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 5 of 5 files at r4, 1 of 1 files at r5.
Reviewable status: 2 of 10 files reviewed, 6 unresolved discussions (waiting on @oncilla and @scrye)

---

go/lib/discovery/topofetcher/fetcher.go, line 33 at r2 (raw file):

type Callbacks struct {
	// Raw is called with the raw body from the discovery service response and the parsed topology.
	Raw func(common.RawBytes, *topology.Topo)

Isn't there a large amount of overlap between the content of the two arguments? Why are they both needed?

---

go/lib/infra/modules/idiscovery/idiscovery.go, line 136 at r5 (raw file):

}

// startPeriodicFetcherWriter starts a runner that periodically fetches the topology.

There's quite a bit of duplication here that might become awkward to maintain. I'm wondering if we can halve the number of types/implementations.

Is there a reason why the periodic runner/fetcher task of a dynamic topology can't have the same behavior/implementation as the static one? Sure, it's never useful to dump the dynamic to disk, so callers will never set the Filename field.

Would something like this work?

---

go/lib/infra/modules/idiscovery/idiscovery.go, line 230 at r5 (raw file):

// NewWriteFetcher creates a periodic.Task that fetches the topology from the discovery
// service and calls the provided handler on the received topology. If the handler indiacates

s/indiacates/indicates.

---

go/lib/infra/modules/idiscovery/idiscovery.go, line 232 at r5 (raw file):

// service and calls the provided handler on the received topology. If the handler indiacates
// an update, the topology is written to filename.
func NewWriteFetcher(handler TopoHandler, params discovery.FetchParams, filename string,

WriteFetcher parses oddly (it sounds like it fetches writes).

Maybe NewWritingFetcher?

Or, for such a specialized object, we can bite the bullet and go full Java with NewFetchThenWriteTask. Overall in the codebase it's going to be called rarely, so a bit of extra verbosity is not that big of an issue.

[scrye]

Reviewed 1 of 1 files at r6.
Reviewable status: 2 of 10 files reviewed, 6 unresolved discussions (waiting on @oncilla and @scrye)

[oncilla]

Reviewable status: 2 of 10 files reviewed, 5 unresolved discussions (waiting on @scrye)

---

go/lib/discovery/topofetcher/fetcher.go, line 33 at r2 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Isn't there a large amount of overlap between the content of the two arguments? Why are they both needed?

We want to avoid the callback from having to re-parse the topology unnecessarily.

---

go/lib/infra/modules/idiscovery/idiscovery.go, line 136 at r5 (raw file):

Previously, scrye (Sergiu Costea) wrote…

There's quite a bit of duplication here that might become awkward to maintain. I'm wondering if we can halve the number of types/implementations.

Is there a reason why the periodic runner/fetcher task of a dynamic topology can't have the same behavior/implementation as the static one? Sure, it's never useful to dump the dynamic to disk, so callers will never set the Filename field.

Would something like this work?

Refactored to reduced code duplication.
However, the two different types for StaticConfig and FetchConfig makes initialization cleaner IMO.

---

go/lib/infra/modules/idiscovery/idiscovery.go, line 230 at r5 (raw file):

Previously, scrye (Sergiu Costea) wrote…

s/indiacates/indicates.

Done.

---

go/lib/infra/modules/idiscovery/idiscovery.go, line 232 at r5 (raw file):

Previously, scrye (Sergiu Costea) wrote…

WriteFetcher parses oddly (it sounds like it fetches writes).

Maybe NewWritingFetcher?

Or, for such a specialized object, we can bite the bullet and go full Java with NewFetchThenWriteTask. Overall in the codebase it's going to be called rarely, so a bit of extra verbosity is not that big of an issue.

Removed during refactor.

[scrye]

Reviewed 4 of 9 files at r7, 1 of 1 files at r12.
Reviewable status: 3 of 10 files reviewed, all discussions resolved (waiting on @scrye)

[scrye]

Reviewed 4 of 9 files at r7.
Reviewable status: 3 of 10 files reviewed, all discussions resolved (waiting on @scrye)

[scrye]

Reviewed 1 of 1 files at r8, 5 of 5 files at r9, 1 of 1 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved