fleetctl: on destroy command ignore units that do not exist #1417
Conversation
fleetctl used to ignore requests to destory units which do not actually exist. This behaviour was changed by commit 9530eed, restore the older behaviour by calling findUnits() to get the right list of units. This fixes: coreos#1383
|
@jonboulle Have checked. It works, but I'd argue with this. If unit doesn't exist - fleetctl should return non-zero code. And we have to document all possible exit statuses. |
|
capturing @mirkoboehm's suggestion - we could add a |
| } | ||
|
|
||
| for _, v := range units { | ||
| err := cAPI.DestroyUnit(v.Name) |
There was a problem hiding this comment.
instead of doing two separate calls, can we check the type of the error returned, and ignore it if it's not-exist?
There was a problem hiding this comment.
I thought that findUnits() is needed since it retrieves all units then it filters them, making sure to mangle names etc. It will only fail if it can't retrieve any unit at all which is another problem. Or perhaps I'm missing something ?
There was a problem hiding this comment.
@tixxdz I'm not really happy this got merged without a follow up to this comment (or an LGTM). It's actually a change in behaviour (and does not restore to the semantic of pre- 9530eed as the commit message implies).
Previously, if we received a command fleetctl destroy a b c, and a and b existed but c did not, we would nevertheless successfully remove a and b. Now, we will immediately error out instead, because c couldn't be found.
My suggestion was to just check the response from each cAPI.DestroyUnit() call, and ignore the error if it's one indicating the unit doesn't exist (this might require some error plumbing). As it is now, we have a race condition between getting the list of units from findUnits and doing the Destroy call.
There was a problem hiding this comment.
Seems there was a misunderstanding regarding the discussion in the main thread. Will follow up with a correction.
There was a problem hiding this comment.
@jonboulle oh for the race condition you have a point here, yes. Adding checks for Destroy will fix it of course, it could be done without removing findUnits() call since my aim was to have more consistency and avoid hard coding logic for each command. @antrik will follow up with a fix.
|
@jonboulle please clarify: do you think we need actually to add the --error-on-not-exist option, or is this regression fix fine to be merged alone? (Once the feedback comment on the patch has been addressed...) |
|
I think this regression fix should be merged, and then if we want to On Wed, Feb 10, 2016 at 6:20 PM, Olaf Buddenhagen [email protected]
|
fleetctl: on destroy command ignore units that do not exist
|
Would it be possible to backport this fix in #1417 into |
|
@megahall 👍 |
|
Downstream report filed as coreos/bugs#1448 . |
fleetctl used to ignore requests to destory units which do not actually exist.
This behaviour was changed by commit 9530eed, restore the older behaviour by calling findUnits()
to get the right list of units.
This fixes: #1383