/juju

Add machine undertaker worker #6082

Merged
merged 7 commits into from Aug 30, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@babbageclunk @jujubot @fwereade
@babbageclunk
Member

babbageclunk commented Aug 24, 2016 •
Edited 1 time

This ensures any necessary provider-level cleanup is done before the machine is fully removed.

In particular, for containers in MAAS we need to release allocated addresses - the container provisioner can't do this because it has no way to talk to the provider directly. So now the provisioner task will mark the machine as ready for removal when its work is completed. This worker watches for the removal request, releases addresses for containers (where the provider supports it), and then removes the machine and associated records. It's expected that other provider-level cleanup like IP address management will be added here.

Fixes http://pad.lv/1585878.

QA steps:

  • bootstrapped against LXD (no container networking)
    • added and removed machines
  • bootstrapped against MAAS 1.9
    • added and removed machines
    • added and removed single-nic containers - checked that the addresses for the containers are gone from the host machine in the MAAS UI
    • added and removed multi-nic containers - checked that the addresses for the containers are gone from the host machine in the MAAS UI
  • bootstrapped against MAAS 2
    • added and removed machines
    • added and removed single-nic containers - checked that the addresses for the containers are gone from the host machine in the MAAS UI
    • added and removed multi-nic containers - checked that the addresses for the containers are gone from the host machine in the MAAS UI

(Review request: http://reviews.vapour.ws/r/5540/)
cmd/jujud/agent/model/manifolds.go
+ machineUndertakerName: ifNotMigrating(machineundertaker.Manifold(machineundertaker.ManifoldConfig{
+ APICallerName: apiCallerName,
+ EnvironName: environTrackerName,
+ NewWorker: machineundertaker.NewWorker,
@fwereade

fwereade Aug 24, 2016

Contributor

you are likely to find manifold testing a bit easier if you supply NewFacade as a dependency too

@babbageclunk

babbageclunk Aug 25, 2016

Member

I haven't done this, since I'd already written the tests for the manifold, and in this case it wasn't too hard to contrive to get an error out of the real API client.

worker/machineundertaker/undertaker.go
+func (u *Undertaker) Handle(<-chan struct{}) error {
+ removals, err := u.api.AllMachineRemovals()
+ if err != nil {
+ // Should this be a fatal error instead?
@fwereade

fwereade Aug 24, 2016

Contributor

yeah, just fail out and trust the infrastructure to retry

@babbageclunk

babbageclunk Aug 25, 2016

Member

Thanks, changed.

babbageclunk added some commits Aug 19, 2016

@babbageclunk
Skeleton of a do-nothing machine undertaker 
Well, other than logging.
31bc3ed
@babbageclunk
Make provisioner task mark machines for removal 
So I can test worker setup.
d1540e5
@babbageclunk
Implemented body of machine undertaker worker 
No tests yet.
54fcbf7
@babbageclunk
Update manifold to deal with new api signature
8dfdd38
@babbageclunk
Fix provisioner tests to look for removal marks 
... rather than waiting for the machine to be removed.

Also a drive-by fix of some test code that had become nonsensical over
time - getContainerInstance was using the OS to choose between identical
alternatives.
6bed2e1
@babbageclunk
Unit tests for machine undertaker worker 
Plus some small tweaks to the worker to make it easier to test.
07ddeb5
@babbageclunk
Review feedback from fwereade
cd398d9
@babbageclunk
Member

babbageclunk commented Aug 30, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Aug 30, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 5de605c into juju:master Aug 30, 2016

@babbageclunk babbageclunk deleted the babbageclunk:machine-undertaker branch Aug 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment