Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updater: Check entire service spec when deciding whether to replace ongoing update #1497

Merged

Conversation

aaronlehmann
Copy link
Collaborator

Currently, updater checks isServiceDirty to decide whether an existing
update should be restarted. This only checks the task spec and the
endpoint spec, so it will ignore changes to fields like UpdateConfig.
For example, it's not possible to change just the parallelism while an
update is already in progress. Fix this by comparing the whole spec.

cc @dongluochen @pnickolov

@@ -49,7 +49,7 @@ func (u *UpdateSupervisor) Update(ctx context.Context, cluster *api.Cluster, ser
id := service.ID

if update, ok := u.updates[id]; ok {
if !update.isServiceDirty(service) {
if reflect.DeepEqual(service.Spec, update.newService.Spec) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not modify isServiceDirty? This is the only place that method is called.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, if the better way to do things is the way it's done here, isServiceDirty is unused and should be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed isServiceDirty.

@codecov-io
Copy link

codecov-io commented Sep 6, 2016

Current coverage is 55.49% (diff: 0.00%)

Merging #1497 into master will increase coverage by 0.09%

@@             master      #1497   diff @@
==========================================
  Files            82         82          
  Lines         12885      12882     -3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7138       7149    +11   
+ Misses         4757       4750     -7   
+ Partials        990        983     -7   

Sunburst

Powered by Codecov. Last update 27fbaef...f2ba23b

…ngoing update

Currently, updater checks isServiceDirty to decide whether an existing
update should be restarted. This only checks the task spec and the
endpoint spec, so it will ignore changes to fields like UpdateConfig.
For example, it's not possible to change just the parallelism while an
update is already in progress. Fix this by comparing the whole spec.

Signed-off-by: Aaron Lehmann <[email protected]>
@dperny
Copy link
Collaborator

dperny commented Sep 6, 2016

LGTM

1 similar comment
@dongluochen
Copy link
Contributor

LGTM

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

Successfully merging this pull request may close these issues.

5 participants