Recreate on volume change #12223

jhrotko · 2024-10-21T11:04:12Z

What I did
Add volumes on configuration hash in order to recreate container when volumes changes

Related issue

fixes [BUG] docker compose up does not recreate containers when named volume configuration is changed #10060

(not mandatory) A picture of a cute animal, if possible in relation to what you did

ndeloof · 2024-10-21T12:07:48Z

IMHO the right way to address this issue is for volumes/networks to have a config-hash label just like services, so we detect those need to be recreated, and then in cascade we re-create the attached containers

pkg/compose/hash.go

ndeloof · 2024-10-23T17:10:28Z

pkg/compose/hash.go

+	if o.Driver == "" { // (TODO: jhrotko) This probably should be fixed in compose-go
+		o.Driver = "local"
+	}
+	o.External = false // the name can change. Need to think about this case


if volume is external you should just never enter this func

there is an edge case where you can change the name of an external volume and we need to update the service. The current code does not comtemplate this case. I need to implement this

by nature, external resources are not managed by compose. There's nothing we can and should do to cover this scenario

I better understand your point, the issue you refer to is a special case for a feature we don't support (yet): recreate services when referred resource is reconfigured. I agree we could kill 2 birds with one stone here, but this makes many changes at once.
Maybe split this into 2 separate PRs ?

1 to recreate volumes on config changes

1 to detect named resource changed (ensureService to inspect mounted volumes for container and check name matches expectation)

I applied both solutions to this PR

sure, I mean it could be simpler to review if we split this effort into two separate PRs

jhrotko · 2024-10-24T16:53:49Z

Added --recreate-volumes in up command. Missing e2e tests @ndeloof @glours . I will clean up the commits after the approvals :)

I still need to not ignore labels to the VolumeHash!

I am also wondering if I should add this flag to create command.

pkg/compose/create.go

jhrotko · 2024-10-24T16:59:35Z

cmd/compose/up.go

@@ -148,6 +148,7 @@ func upCommand(p *ProjectOptions, dockerCli command.Cli, backend api.Service) *c
 	flags.BoolVar(&create.noBuild, "no-build", false, "Don't build an image, even if it's policy")
 	flags.StringVar(&create.Pull, "pull", "policy", `Pull image before running ("always"|"missing"|"never")`)
 	flags.BoolVar(&create.removeOrphans, "remove-orphans", false, "Remove containers for services not defined in the Compose file")
+	flags.BoolVar(&create.recreateVolumes, "recreate-volumes", false, "Recreate volumes when volume configuration in the Compose file changes.")


we can rename it :P

I also wonder this could be ambiguous, considering we have --renew-anon-volumes. Need to noun that make it clear we recreate volumes when config has diverged

how about update-volumes?

@ndeloof the recreate-volumes makes somewhat sense to me, also given the 'pattern' for force-recreate flag. I don't think it is ambiguous with the flag --renew-anon-volumes

it is, as we only recreate diverged volumes. Not all volumes (and not anonymous ones)

@ndeloof I can add that at the flag description: that this will ignore anonymous volumes. As for the names maybe? If you have any other ideas please feel free to suggest

--recreate-volumes-if-changed
--force-recreate-volumes
--sync-volumes
--update-volumes
--refresh-named-volumes
--rebuild-volumes

Or do you prefer to make this the default behaviour with a prompt?

ndeloof

ensureService doesn't need to be updated for this feature AFAICT, as we remove containers mounting a volume before we recreate volume, so ensureService will start from scratch - possibly just requires observedState to be updated after Remove?

pkg/api/labels.go

pkg/compose/create.go

pkg/compose/convergence.go

ndeloof · 2024-10-25T13:52:50Z

cmd/compose/up.go

@@ -148,6 +148,7 @@ func upCommand(p *ProjectOptions, dockerCli command.Cli, backend api.Service) *c
 	flags.BoolVar(&create.noBuild, "no-build", false, "Don't build an image, even if it's policy")
 	flags.StringVar(&create.Pull, "pull", "policy", `Pull image before running ("always"|"missing"|"never")`)
 	flags.BoolVar(&create.removeOrphans, "remove-orphans", false, "Remove containers for services not defined in the Compose file")
+	flags.BoolVar(&create.recreateVolumes, "recreate-volumes", false, "Recreate volumes when volume configuration in the Compose file changes.")


I also wonder this could be ambiguous, considering we have --renew-anon-volumes. Need to noun that make it clear we recreate volumes when config has diverged

pkg/compose/create.go

ndeloof · 2024-11-06T12:27:46Z

pkg/compose/create.go

+		if !shouldRecreateVolume {
+			continue
+		}
+		if !recreateVolumes && shouldRecreateVolume {


we could rely on user interaction to confirm recreation, the way we do on https://github.com/docker/compose/blob/main/pkg/compose/remove.go#L88

looks really nice, but I will defer it for a follow-up PR

pkg/compose/hash.go

Signed-off-by: Joana Hrotko <[email protected]>

jhrotko requested review from ndeloof and glours October 21, 2024 11:04

jhrotko self-assigned this Oct 21, 2024

jhrotko force-pushed the recreate-on-volume-change branch 3 times, most recently from 4032d1b to dd57c3d Compare October 22, 2024 15:40

ndeloof reviewed Oct 23, 2024

View reviewed changes

pkg/compose/hash.go Outdated Show resolved Hide resolved

ndeloof reviewed Oct 23, 2024

View reviewed changes

jhrotko force-pushed the recreate-on-volume-change branch from fb72af3 to 2823c25 Compare October 24, 2024 15:11

jhrotko commented Oct 24, 2024

View reviewed changes

pkg/compose/create.go Outdated Show resolved Hide resolved

jhrotko commented Oct 24, 2024

View reviewed changes

jhrotko force-pushed the recreate-on-volume-change branch from c430b99 to 0b886e9 Compare October 25, 2024 13:51

ndeloof reviewed Oct 25, 2024

View reviewed changes

jhrotko force-pushed the recreate-on-volume-change branch from 0b886e9 to f9a8185 Compare October 25, 2024 14:03

ndeloof reviewed Oct 25, 2024

View reviewed changes

pkg/compose/create.go Outdated Show resolved Hide resolved

jhrotko force-pushed the recreate-on-volume-change branch 10 times, most recently from 52a5d2e to e83964b Compare October 30, 2024 13:57

ndeloof reviewed Nov 6, 2024

View reviewed changes

pkg/compose/hash.go Show resolved Hide resolved

jhrotko force-pushed the recreate-on-volume-change branch from e83964b to b80222b Compare November 6, 2024 14:26

Recreate volumes on volume configuration change

43d3b27

Signed-off-by: Joana Hrotko <[email protected]>

jhrotko force-pushed the recreate-on-volume-change branch from b80222b to 43d3b27 Compare November 6, 2024 15:40

ndeloof mentioned this pull request Dec 9, 2024

Recreate container on volume configuration change #12363

Merged

ndeloof closed this Dec 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recreate on volume change #12223

Recreate on volume change #12223

jhrotko commented Oct 21, 2024

ndeloof commented Oct 21, 2024

ndeloof Oct 23, 2024

jhrotko Oct 25, 2024 •

edited

Loading

ndeloof Oct 25, 2024

ndeloof Oct 25, 2024

jhrotko Nov 6, 2024

ndeloof Nov 6, 2024

jhrotko commented Oct 24, 2024 •

edited

Loading

jhrotko Oct 24, 2024

ndeloof Oct 25, 2024

jhrotko Oct 25, 2024

jhrotko Nov 6, 2024

ndeloof Nov 6, 2024

jhrotko Nov 6, 2024

jhrotko Nov 6, 2024

ndeloof left a comment

ndeloof Oct 25, 2024

ndeloof Nov 6, 2024

jhrotko Nov 6, 2024

Recreate on volume change #12223

Recreate on volume change #12223

Conversation

jhrotko commented Oct 21, 2024

ndeloof commented Oct 21, 2024

Choose a reason for hiding this comment

jhrotko Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrotko commented Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndeloof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrotko Oct 25, 2024 •

edited

Loading

jhrotko commented Oct 24, 2024 •

edited

Loading