feat: return error when service update triggers rollback

[Best2Two]

Return error when service update triggers rollback

When a service update fails and triggers a rollback, the command now
exits with code 1 instead of 0.

Fixes #6752

- What I did
Modified the ServiceProgress function to return an error when a service update is rolled back, instead of returning success (nil)

- How I did it
Added a check for the rollback flag after service convergence. When rollback is true, the function now returns an error instead of nil, which causes the CLI to exit with code 1.

- How to verify it

1. Deploy a stack with a healthy service
2. Update the service with a configuration that will fail (e.g., failing healthcheck)
3. The service will rollback automatically
4. Verify the command exits with code 1 instead of 0

Test setup:

• Use the docker-compose.yml from issue Unable to detect rollbacks during "docker stack deploy" in CI/CD #6752
• First deploy with exit 0 in healthcheck
• Then deploy with exit 1 in healthcheck to trigger rollback
• Check exit code with echo $?

- Human readable description for the release notes

- Fix `docker stack deploy` to exit with error code 1 when service update triggers rollback

- A picture of a cute animal (not mandatory but encouraged)

[Best2Two]

This is a new one, fixed the old bad PR!

[Best2Two]

@thaJeztah kind remember, this is not the old bad PR, it's a new one you can review

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cli/command/service/progress/progress.go	0.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[vvoland]

I think it makes sense, but I imagine that this would be a breaking change for a lot of users.

WDYT @corhere @thaJeztah?

[thaJeztah]

Thanks! I'll have a look at this.

My main concern is that this may be a breaking change; the current behavior considers "if the service is configured to rollback on failure, then rolling back is the expected outcome" (so a zero exit code); this change changes that to consider the update / deploy to be successful. I agree that that probably would make sense, but these things can be tricky to change as it's possible people depend on this 🤔

Before this change;

docker service create --name myservice --update-failure-action=rollback nginx:alpine
qcg3psc2azo9892h90e32rm8h
overall progress: 1 out of 1 tasks
1/1: running
verify: Service qcg3psc2azo9892h90e32rm8h converged

docker service update --entrypoint "exit 1" myservice
myservice
overall progress: rolling back update: 1 out of 1 tasks
1/1: running
rollback: update rolled back due to failure or early termination of task tebqn63e50c7l68dw98a4p9eu
verify: Service myservice converged

echo $?
0

That said; we currently DO return a non-zero exit code when pausing the update;

docker service create --name myservice --update-failure-action=pause nginx:alpine
uzdx5f97a7grbmd8dv541x8ut
overall progress: 1 out of 1 tasks
1/1: running
verify: Service uzdx5f97a7grbmd8dv541x8ut converged

docker service update --entrypoint "exit 1" myservice
myservice
overall progress: 0 out of 1 tasks
1/1: ready
service update paused: update paused due to failure or early termination of task sayp1gpmj2lspzd59ef0ktaia


echo $?
1

But in that case, the service will also be in an unhealthy state;

docker service ls
ID             NAME        MODE         REPLICAS   IMAGE          PORTS
uzdx5f97a7gr   myservice   replicated   0/1        nginx:alpine

@corhere @dperny any thoughts?

[Best2Two]

I believe patches should be backward compatible as well, but at the same moment CI/CD should react to it. One solution in my mind can't it return 0, but at the same time, show some warning about the rollback state so the pipeline can react to it? But my concern is that this may not be a consistent behaviour.

[thaJeztah]

(looks like I didn't refresh my browser, and missed @vvoland's comment 😅)

Yeah, if we decide to go ahead, at least we shouldn't include it in a patch release, but wait to (at least) v29.3.0.

[Best2Two]

Yes! Or maybe exit with 0, but throws a warning, but this may be inconsistent behaviour, if I am wrong, and this is considered a consistent behaviour, I can commit this change now

[Best2Two]

Another solution is that we can add this flag

• docker stack deploy --fail-on-rollback
  so no need to throw warning here and it's a consistent behaviour
  WDYT @vvoland @thaJeztah

[vvoland]

I think separate flag is fine for this for a minor release. I definitely wouldn't change that in a non-major version.

[thaJeztah]

No, we shouldn't add a flag specific for this case.

but throws a warning

There's already output indicating that a pause or rollback happened, which could be used as a temporary workaround; from my examples above;

rollback: update rolled back due to failure or early termination of task tebqn63e50c7l68dw98a4p9eu
service update paused: update paused due to failure or early termination of task sayp1gpmj2lspzd59ef0ktaia

[Best2Two]

@thaJeztah @vvoland
Thank you very much for your detailed review :)