Skip to content
This repository was archived by the owner on Sep 12, 2024. It is now read-only.

More robust cleanup of removed hosts for deployment groups #1042

Merged
merged 2 commits into from
Dec 13, 2016

Conversation

rohansingh
Copy link
Contributor

Instead of the previous method where we would try to undeploy from hosts
that were no longer registered, instead just apply a straightforward filter
to the removed hosts list.

This is clearer and works without generating a bunch of unnecessary tasks.
Also, the previous version would only work if the set of UP hosts for the
deployment group changed. This version doesn't have that flaw.

This reverts commit 6412fc0.

@rohansingh
Copy link
Contributor Author

@spotify/helios-team

@mattnworb
Copy link
Member

so hosts that are no longer in the DG and happen to be down will stay in the /removed znode until they are deregistered and another rolling-update happens for that group (either manually or because of another host change)?

@rohansingh
Copy link
Contributor Author

rohansingh commented Dec 13, 2016

@mattnworb Almost right. The hosts will be removed from the /removed znode when they are deregistered, regardless of another rolling-update happening. This is because we periodically execute the reactor to update the host lists, regardless of rolling updates.

@mattnworb
Copy link
Member

thanks for the clarification. I think the stated outcome makes sense. It's hard to evaluate the code on it's own - it is a big bummer that we don't have any real unit tests of any of this rolling-update logic.

Instead of the previous method where we would try to undeploy from hosts
that were no longer registered, instead just apply a straightforward filter
to the removed hosts list.

This is clearer and works without generating a bunch of unnecessary tasks.
Also, the previous version would only work if the set of UP hosts for the
deployment group changed. This version doesn't have that flaw.
@codecov-io
Copy link

codecov-io commented Dec 13, 2016

Current coverage is 51.37% (diff: 92.30%)

Merging #1042 into master will increase coverage by 0.03%

@@             master      #1042   diff @@
==========================================
  Files           275        275          
  Lines         13074      13070     -4   
  Methods           0          0          
  Messages          0          0          
  Branches       1694       1693     -1   
==========================================
+ Hits           6712       6715     +3   
+ Misses         5858       5852     -6   
+ Partials        504        503     -1   

Powered by Codecov. Last update 65eccee...9b744d1

@gimaker
Copy link
Contributor

gimaker commented Dec 13, 2016

👍

@rohansingh rohansingh merged commit c28b431 into master Dec 13, 2016
@rohansingh rohansingh deleted the rohan/unregister branch December 13, 2016 16:41
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.

4 participants