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

[WIP] try make Service.UpdateStatus non-ambiguous #38520

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@thaJeztah
Copy link
Member

thaJeztah commented Jan 9, 2019

In situations where only annotations of a service were updated, SwarmKit never marks a service as UpdateStateUpdating; the service's UpdateStatus is set to nil, and never updated after.

Because of this, it's not possible to verify if the update actually happened (unless comparing the services Version before and after).

This patch modifies the SwarmKit vendored code to try to make this situation non-ambigious, and mark the services as "updated" in all cases.

Relates to #38499 (comment) :

Looking at that code; #38499 (comment)

As part of the Service update, controlapi.UpdateService() sets Updatestatus to nil to indicate that an update is in progress; after that the updated service is written to the store (store.UpdateService()), which updates the UpdatedAt and Version.

Updater.Run() checks if any tasks are "dirty" and need updating; https://github.com/docker/swarmkit/blob/486ad69951868792367a503042ef3f55b399cc40/manager/orchestrator/update/updater.go#L137-L141

	var dirtySlots []orchestrator.Slot
	for _, slot := range slots {
		if u.isSlotDirty(slot) {
			dirtySlots = append(dirtySlots, slot)
		}
	}

Some changes on a service don't require updates to the task (container); to check if updates are needed, these functions are called:

If changes are needed, len(dirtySlots) > 0, in which case and update is started (if no update is in progress yet). From that part of the code, it's clear that service.UpdateStatus == nil is required to even consider starting an update. Updater.startUpdate() is called, which sets the service's UpdateStatus to UpdateStatus_UPDATING, and sets the UpdateStatus.StartedAt to the current time.

However, in case of the service-label update, no updates to the tasks are needed, so len(dirtySlots) == 0; in that case, no action is needed, given that no update (or "rollback") was ever started for this. In that case, the Service.UpdateStatus is not updated, so remains nil

	// Abort immediately if all tasks are clean.
	if len(dirtySlots) == 0 {
		if service.UpdateStatus != nil &&
			(service.UpdateStatus.State == api.UpdateStatus_UPDATING ||
				service.UpdateStatus.State == api.UpdateStatus_ROLLBACK_STARTED) {
			u.completeUpdate(ctx, service.ID)
		}
		return
	}

Even if we would skip the check (if an updated or rollback was started), update.completeUpdate() checks service.UpdateStatus. If service.UpdateStatus == nil, it assumes that another update was started, so the service.UpdateStatus is not updated;

	if service.UpdateStatus == nil {
		// The service was changed since we started this update
		return nil
	}

Based on the above, it looks like service.UpdateStatus == nil is ambiguous; as it can either be;

  • An update was triggered, but no action was required (in which case it's equal to UpdateStatus_COMPLETED or UpdateStatus_ROLLBACK_COMPLETED)
  • An update was triggered, but the actual update was not yet started

I think we can make it non-ambiguous if we set the status to UpdateStatus_COMPLETED if there are no tasks to update (len(dirtySlots) == 0); this requires a change to Updater.Run() (change the check), and to update.completeUpdate()

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jan 9, 2019

Let's see if this works; updated the vendored code in this PR just to give it a quick try 😅

@thaJeztah thaJeztah force-pushed the thaJeztah:fix_update_status_check branch from ad96b5a to b3b739a Jan 9, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 9, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@cb50188). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #38520   +/-   ##
=========================================
  Coverage          ?    36.6%           
=========================================
  Files             ?      608           
  Lines             ?    45174           
  Branches          ?        0           
=========================================
  Hits              ?    16535           
  Misses            ?    26349           
  Partials          ?     2290
WIP try make Service.UpdateStatus non-ambigious
In situations where only annotations of a service were updated,
SwarmKit never marks a service as `UpdateStateUpdating`; the
service's `UpdateStatus` is set to `nil`, and never updated
after.

Because of this, it's not possible to verify if the update
actually happened (unless comparing the services `Version`
before and after).

This patch modifies the SwarmKit vendored code to try to
make this situation non-ambigious, and mark the services
as "updated" in all cases.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@thaJeztah thaJeztah force-pushed the thaJeztah:fix_update_status_check branch from b3b739a to cf9f75e Jan 9, 2019

@thaJeztah thaJeztah changed the title [WIP] try make Service.UpdateStatus non-ambigious [WIP] try make Service.UpdateStatus non-ambiguous Jan 9, 2019

@thaJeztah thaJeztah added the rebuild/* label Jan 9, 2019

@olljanat

This comment has been minimized.

Copy link
Contributor

olljanat commented Jan 9, 2019

Tested with my new tool #38523 works (renamed test to trigger test). Passed nicely all 25 tries. Log from that test: 38520_test-integration-flaky.log

Looks good 👍

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jan 10, 2019

Opened an upstream PR in SwarmKit; docker/swarmkit#2804

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