Codecov Report

Merging #38499 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #38499      +/-   ##
==========================================
- Coverage   36.65%   36.62%   -0.04%     
==========================================
  Files         608      608              
  Lines       45174    45174              
==========================================
- Hits        16560    16544      -16     
- Misses      26329    26341      +12     
- Partials     2285     2289       +4

(reserved for my derek commands)

Wow 😮 this PR passed CI on first try. Haven't see that happening earlier.

Anyway as purpose is see if those flaky tests are fixed I will do force push to see if they passes second time.

Only RS1 failed on second run. Looks nice.
@thaJeztah @vdemeester PTAL

@kolyshkin you maybe also want to look this?
@cpuguy83 promised to do some checks for it (conversation is on slack)

@thaJeztah on that case IMO correct approach would be add similar label modify tests to swarmkit side.

This is interesting that updating labels actually even sets the update status to nil.

In swarmkit when the service is updated in the store, the UpdateStatus is always set to nil. I guess since there's nothing left to reconcile no update status needs to be set.

This isServiceUpdated seems a bit weak. I think we should pass in the original spec version and make sure the new index version is > the original just like we do in isServiceSpecUpdated.

@cpuguy83 can be done but I would still prefer to merge this and create another PR for those improvements so we get CI working again.

UpdateStatus is set to Nil on https://github.com/docker/swarmkit/blob/0091d9285168040649140c76629e4284857e5365/manager/controlapi/service.go#L827-L828

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:

• Updater.isSlotDirty()
  • -> Updater.isTaskDirty
    • -> orchestrator.IsTaskDirty()

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()