Skip to content

loader: reconcile restart and deploy.restart_policy, reject contradictions - #989

Open
ndeloof wants to merge 1 commit into
compose-spec:mainfrom
ndeloof:restart-policy-consistency
Open

ndeloof wants to merge 1 commit into
compose-spec:mainfrom
ndeloof:restart-policy-consistency

Conversation

@ndeloof

@ndeloof ndeloof commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

What this PR does, in one sentence

restart and deploy.restart_policy are now reconciled by the loader: each may complete the other, the outcome is carried in restart, and a contradiction between the two is rejected instead of being settled silently.

Context

A service can express its restart policy twice: with restart (on-failure:3) and with deploy.restart_policy (condition: on-failure, max_attempts: 3). The spec says restart_policy wins when it is set, and every consumer has to implement that on its own. Docker Compose does, by replacing the whole policy computed from restart, which has two bad outcomes (docker/compose#14383):

  • a restart_policy that does not set a condition (a delay alone, say) wipes out the policy of restart, and the engine then silently creates a container that never restarts;
  • two policies that disagree (restart: unless-stopped with condition: on-failure) are not reported: one of them is dropped without a word.

What the PR brings

  • The two attributes complete each other. condition is the name of the policy and max_attempts its retry count, so restart: on-failure with max_attempts: 3 is on-failure:3, and deploy.restart_policy.condition: any alone is restart: always. The outcome is written in restart, on the merged model (an override file or an extends may still change either attribute until then), for every service. An attribute that is not set says nothing: a restart_policy made of a delay alone leaves restart as it is. deploy.restart_policy itself is left untouched.
  • A contradiction is an error, in the consistency check next to scale / deploy.replicas: services.foo: can't set distinct values on 'restart' and 'deploy.restart_policy.condition' (or .max_attempts). That covers another policy, another retry count, and a retry count given to a policy that has none (restart: always with max_attempts: 3). As for the other consistency checks it is skipped with SkipConsistencyCheck, and the model is then kept as written.
  • Left to the platform: names Compose does not know, a malformed count (on-failure:abc), and a retry count with any / none and no restart (the engine rejects it today, and it is no restart value, so nothing is carried).

Compatibility

A service that sets both restart and deploy.restart_policy with values that disagree is now rejected at load time, where one of the two used to be dropped without notice. The error names the two attributes to align, and SkipConsistencyCheck skips the check. Services that set only one of the two, or both with the same policy, load as before, with the policy now also carried in restart. Worth a line in the release notes.

What comes next

Once released, docker/compose can read the policy from restart only, instead of merging the two attributes in getRestartPolicy (docker/compose#14459 does the field-by-field merge in the meantime).

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Restart-policy handling incorrectly rejects unrestricted retries expressed as max_attempts: 0 and fails to propagate restart settings to LoadModelWithContext consumers.

2 open issues
What changed in this PR

This PR centralizes restart-policy reconciliation in the Compose loader, reducing policy-merging work for consumers and rejecting contradictory service settings.

Changes:

  • Carries compatible restart and deploy.restart_policy values into restart after merging.
  • Adds contradiction checks that respect SkipConsistencyCheck.
  • Adds tests for reconciliation, conflicts, reloads, overrides, and inheritance.
File Description
loader/​validate.go Integrates restart-policy contradiction checks.
loader/​tests/​restart_test.go Covers reconciliation and loader-option behavior.
loader/​restart.go Implements policy mapping, merging, and conflict detection.
loader/​loader.go Applies reconciliation during normalized project conversion.

Review effort: Balanced

Comment thread loader/loader.go Resolved
Comment thread loader/restart.go Resolved
@ndeloof

ndeloof commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

Demo of what this enables on the Docker Compose side: docker/compose#14461 (draft, go.mod replace to this branch). getRestartPolicy goes down to a single read of restart, with a test loading real compose YAML for the cases above.

…tory values

restart and deploy.restart_policy are two spellings of one policy:
condition is its name and max_attempts its retry count. Each may complete
the other, and normalization writes the outcome in restart, so a
restart_policy made of a delay alone (docker/compose#14383) leaves restart
as it is instead of clobbering it.

Where both say something it must be the same thing: the consistency check
now rejects them with 'can't set distinct values on', as it does for scale
and deploy.replicas, instead of letting one silently win. A max_attempts of
zero is no limit, as for on-failure:0, so it contradicts nothing but a retry
count.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof
ndeloof force-pushed the restart-policy-consistency branch from 860ac4a to 9652c7f Compare November 5, 2026 14:50
@glours

glours commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator

Why an error rather than a warning here?

The spec defines the precedence: "If restart_policy is not set, Compose considers the restart field" (deploy.md#restart_policy). So when both are set, restart_policy wins. Unlike scale / deploy.replicas, the spec never says they must be consistent.

Rejecting at load time is stricter than the spec and breaks files that load today (restart: unless-stopped + condition: any, restart: always + condition: on-failure). As far as I can tell, up has no way to skip the check.

The carry into restart already fixes docker/compose#14383. For contradictions, I'd keep the spec precedence and log a warning.

WDYT @ndeloof?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants