Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Start LXD containers up after migration #13
Conversation
babbageclunk
requested changes
Aug 9, 2017
This is mostly great, but could you please either flatten down the two nested layers of parallelism in waitLXDContainersReady to one or pull the outer closure out to a function so that the two loops can be understood separately?
| + w.buf.Write(data) | ||
| + for { | ||
| + line, err := w.buf.ReadBytes('\n') | ||
| + if err != nil { |
babbageclunk
Aug 9, 2017
Member
I found the behaviour of Write for data that doesn't end with a '\n confusing, but looking at how it's used I guess it's because io.Copy will break the data at arbitrary points and this prevents doubling up the prefix? Probably worth a comment?
| + } | ||
| + if err := waitLXDContainersReady( | ||
| + lxcByHost, containerNames, | ||
| + time.Minute, // should be long enough for anyone |
| + lxdByHost map[*state.Machine]map[string]*lxdContainer, | ||
| + containerNames map[*state.Machine]containerNames, | ||
| +) error { | ||
| + running := make(map[string]bool) // keyed by LXD container name |
babbageclunk
Aug 9, 2017
Member
set.Strings here? Not much difference since you're only doing doing contains checks below.
| + } | ||
| + } | ||
| + var group errgroup.Group | ||
| + for host, containers := range lxcByHost { |
babbageclunk
Aug 9, 2017
Member
What about containers that were already migrated and renamed but not yet restarted? Oh, they'll still be in lxcByHost since that's populated from state, not from the actual machines, right?
| + | ||
| + ctx, cancel := context.WithTimeout(context.Background(), timeout) | ||
| + defer cancel() | ||
| + group, ctx := errgroup.WithContext(ctx) |
| + group, ctx := errgroup.WithContext(ctx) | ||
| + const interval = time.Second // time to wait between checks | ||
| + | ||
| + waitHostLXDContainersReady := func(host *state.Machine) error { |
babbageclunk
Aug 9, 2017
Member
I feel like two nested levels of closures and parallel execution are too many. Would it be difficult to extract waitHostLXDContainersReady to a separate method/function so that it can be understood in isolation? Then I can get my head around the fanout that you do there (from a host to its containers), so I can ignore it when understanding the fanout to the hosts.
Actually, does the parallelism need to be two levels? Can't you just flatten them down to a list of containers (wherever they're hosted) and then parallelize the sshing to them? I think that would be easier to follow.
axw
Aug 9, 2017
Member
must've been on crack, waitHostLXDContainersReady was iterating over all containers, regardless of host. I've greatly simplified the code - good call!
| + lxcByHost map[*state.Machine][]*state.Machine, | ||
| +) error { | ||
| + // Stop the Juju agents on the LXD containers. | ||
| + var flatMachines []FlatMachine |
axw commentedAug 7, 2017
After migrating LXC containers to LXD,
start the LXD containers up so that
other commands can SSH into them to
run the remaining parts of the upgrade,
such as upgrade-agents. After starting
the LXD containers, the Juju agents are
stopped.
The "serviceCall" and "serviceCommand"
functions are consolidated into a new
function, agentServiceCommand. This
function will return an error if any
of the executions exit with a non-zero
code. Failed executions will have their
output written to stderr, prefixed with
the machine ID.