Skip to content
This repository has been archived by the owner on Sep 26, 2021. It is now read-only.

Ignore "unknown instance" error when rm instance #4116

Merged
merged 2 commits into from
Jun 2, 2017
Merged

Ignore "unknown instance" error when rm instance #4116

merged 2 commits into from
Jun 2, 2017

Conversation

threadproc
Copy link
Contributor

Fixes #4112

This is especially useful when using Spot instances on EC2, as they may be removed at any time.

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "4112-allow-remove-unknown" [email protected]:parryjacob/machine.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@nathanleclaire
Copy link
Contributor

Good idea, just one suggestion:

How about instead of returning nil you return something like errors.New("Can't locate remote instance -- The instance may have been removed by another method")? I'd still consider a rm that doesn't actually do anything an error (non-zero exit, just like how rm i_dont_exist would return 1).

@threadproc
Copy link
Contributor Author

@nathanleclaire The general motivation behind this is the idea that when this function returns an error, it cascades up the stack and causes docker-machine to not remove the instance from its local state when it doesn't exist remotely either. Generally, the idea is that rm should remove the machine locally and remotely, and if it exists locally but not remotely it isn't really an error and it should still remove it locally instead of throwing an error.

This circumstance is most common when using Amazon EC2 Spot Instances, when the remote machine being gone isn't exceptional and is expected.

This also supposedly brings the amazonec2 driver inline with the behaviour of other drivers like the digitalocean driver which apparently does not have this issue. (see related issue in the GitLab CI runner project: https://gitlab.com/gitlab-org/gitlab-ci-multi-runner/issues/1947#note_19955573)

@nathanleclaire
Copy link
Contributor

OK -- seems reasonable -- how about just a log.Warn (Remote instance does not exist, proceeding with removing local reference) before the return nil then.

@threadproc
Copy link
Contributor Author

@nathanleclaire I've added that, thanks for the feedback!

@nathanleclaire
Copy link
Contributor

LGTM

@nathanleclaire nathanleclaire merged commit 4c51397 into docker:master Jun 2, 2017
@threadproc threadproc deleted the 4112-allow-remove-unknown branch June 2, 2017 19:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants