fix(hash): recreate container on project config content change #11931

idsulik · 2024-06-23T16:11:50Z

What I did
Fixed hash.ServiceHash() to support config content change

Related issue
#11900

ndeloof · 2024-07-01T14:21:07Z

While I understand the intent, I don't like we get the config content added into the service hash. This also only makes sense as the config content is inlined.
I wonder we could rely on a label to track the config state by the time it was created : com.docker.compose.config.name=<config hash>.

ndeloof · 2024-07-02T07:02:07Z

pkg/compose/hash.go

+
+	for _, serviceConfig := range o.Configs {
+		if projectConfig, ok := project.Configs[serviceConfig.Source]; ok {
+			bytes = append(bytes, []byte(projectConfig.Content)...)


This won't detect changes for config set by environment of file

added support for env and config file

idsulik · 2024-07-06T18:32:39Z

I don't like we get the config content added into the service hash

why?

I wonder we could rely on a label to track the config state by the time it was created : com.docker.compose.config.name=<config hash>.

I don't get the idea, time it was created - do you mean config file? or docker-compose.yaml? but docker-compose can refer to external config file

ndeloof · 2024-07-08T15:01:12Z

time container was created, so we can check it needs to be recreated if current config doesn't match

jhrotko · 2024-07-09T15:34:32Z

pkg/compose/hash.go

@@ -37,5 +38,22 @@ func ServiceHash(o types.ServiceConfig) (string, error) {
 	if err != nil {
 		return "", err
 	}
+
+	for _, serviceConfig := range o.Configs {
+		if projectConfig, ok := project.Configs[serviceConfig.Source]; ok {


nit: I would invert the if statements so that we do not get the code so nested. Other than that looks good to me!

@jhrotko done

ndeloof · 2024-07-10T06:12:59Z

Some poins:

At some point we will have to do the same for secrets (same constraints) and networks/volumes (which can get labeled with a has). Solution we adopt here should be extensible to allow this in the future
Configs set by file are implemented by a bind mount but this may change in near future (see Enable configs.file's on remote docker hosts #11871) so we also need to consider those
There's no technical reason docker engine can't offer secrets/configs natively, just this is guarded by Swarm mode, but AFAIK some discussion happened to get them also available in standalone mode. I can't tell if/when this would take place, but preferably the logic here should consider this may happen
Last but not least, I'd prefer we don't mix service config hash with resources it depends on, so my suggestion to introduce an additional label com.docker.compose.config.xx=hash that we can use to track this relation, and need to recreate container, without the need to change the service hash computation (which as impact on existing installations)

idsulik · 2024-07-11T16:37:10Z

@ndeloof thanks for the details. pushed changes:

reverted old changes
added new func ServiceDependenciesHash and label

// ConfigHashDependenciesLabel stores configuration hash for a compose service dependencies
ConfigHashDependenciesLabel = "com.docker.compose.config-hash-dependencies"

Let me know if you have better idea for the label name, because I'm not fully satisfied with my name)

ndeloof · 2024-07-22T08:50:16Z

pkg/compose/hash.go

+		if projectConfig.Content != "" {
+			bytes = append(bytes, []byte(projectConfig.Content)...)
+		} else if projectConfig.File != "" {
+			content, err := os.ReadFile(projectConfig.File)


File could also be a folder. Maybe an option would be to read content into a tar (code exists to inject secrets) and compute hash from it

While the general approach looks good, I'd prefer we have a more explicit hash and label per resource, and a dedicated hash function for configs (and secrets), so that it make it easier to diagnose use of those labels

@ndeloof thank you the comment. Pushed fixes: added secrets hash, added separate hashes for configs and secrets, added support for file.File directory

ndeloof · 2024-09-18T07:31:22Z

pkg/compose/hash.go

+				return err
+			}
+			if !info.IsDir() {
+				fileContent, err := os.ReadFile(path)


this will not detect file being rename. Maybe better to create a tar stream from file/dir content

@ndeloof it creates a tar from the content here pkg/compose/hash.go:122.

b, err := utils.CreateTar(content, serviceConfig, modTime)

@ndeloof is there any reason to create tar inside a tar? the createTarForConfig func calls this readPathContent func and creates a tar with the path content

here you are creating a tar with all content appended as []byte, this doesn't reflect the filesystem structure for intput. You have to create one tar entry per file with file/folder name.

@ndeloof you're right, thank you! Updated the code, now it creates a tar file for a given path and keeps file structure

Signed-off-by: Suleiman Dibirov <[email protected]>

This reverts commit 64c37bf. Signed-off-by: Suleiman Dibirov <[email protected]>

Signed-off-by: Suleiman Dibirov <[email protected]>

…older support Signed-off-by: Suleiman Dibirov <[email protected]>

Signed-off-by: Suleiman Dibirov <[email protected]>

codecov · 2024-10-06T16:09:06Z

Codecov Report

Attention: Patch coverage is 60.73620% with 64 lines in your changes missing coverage. Please review.

Project coverage is 51.21%. Comparing base (d9df7aa) to head (21d0e4c).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/utils/tar.go	58.22%	22 Missing and 11 partials ⚠️
pkg/compose/hash.go	68.88%	11 Missing and 3 partials ⚠️
pkg/compose/convergence.go	52.17%	6 Missing and 5 partials ⚠️
pkg/compose/create.go	57.14%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11931      +/-   ##
==========================================
+ Coverage   51.10%   51.21%   +0.11%     
==========================================
  Files         149      152       +3     
  Lines       14537    14743     +206     
==========================================
+ Hits         7429     7551     +122     
- Misses       6356     6419      +63     
- Partials      752      773      +21

Flag	Coverage Δ
	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ndeloof · 2024-10-08T06:22:15Z

pkg/compose/hash.go

+		data = append(data, b.Bytes()...)
+	}
+
+	return digest.SHA256.FromBytes(data).Encoded(), nil


I'd prefer we have one label per config/secret mount, so it makes it easier to track|debug changes and container being recreated.
Also need to consider config can be mounted from docker host, i.e. file is not available for compose to compute hash, and then must be excluded from label / no label created. createTarForConfig could return ErrNotFound and we would ignore it for this specific usage

do you mean something like that:

com.docker.compose.service.configs-hash-{configName}={hash} com.docker.compose.service.configs-hash-{serviceName}={hash}

?

indeed, or maybe, to follow the dot-notation style used for labels, com.docker.compose.service.configs.{configName}.hash

It seems to me that this will complicate the logic, first you need to generate a hash for each item separately, then you need to go through all labels whose names start with “com.docker.compose.service.configs.” to check if the hash has changed.

Doesn't look such a pain to me, as this would allow to trace reason we recreate a container, and make it easier to diagnose potential regressions (this sometimes happened :P)

for c := range service.Configs { hash := labels["com.docker.compose.configs."+c+".hash"] expected := ConfigHash(project.Configs[c] if hash := expected { log.Debug("container has to be recreated after config %s has been updated", c) return DIVERGED } }

ndeloof · 2024-10-08T06:33:43Z

pkg/utils/tar.go

+	if err != nil {
+		return nil, err
+	}
+
+	return b, nil


nit: make it simpler as return b, err

ndeloof reviewed Jul 2, 2024

View reviewed changes

idsulik force-pushed the issue-11900 branch from 19daddd to f75d940 Compare July 6, 2024 18:30

jhrotko reviewed Jul 9, 2024

View reviewed changes

idsulik force-pushed the issue-11900 branch from b67627e to b506004 Compare July 11, 2024 16:35

idsulik force-pushed the issue-11900 branch from b506004 to 17237d4 Compare July 12, 2024 06:54

schaubl mentioned this pull request Jul 17, 2024

Support remote DOCKER_HOST by copying configs and secrets files instead of bind mounting them #11867

Open

idsulik requested review from ndeloof and jhrotko July 22, 2024 08:03

ndeloof reviewed Jul 22, 2024

View reviewed changes

idsulik force-pushed the issue-11900 branch from f7667bb to 878fea5 Compare July 22, 2024 16:30

idsulik requested a review from ndeloof July 22, 2024 16:32

idsulik force-pushed the issue-11900 branch from 284468c to 2aa70c0 Compare September 16, 2024 08:34

ndeloof reviewed Sep 18, 2024

View reviewed changes

aequitas mentioned this pull request Sep 18, 2024

Changes in Docker compose configs not propagated to containers during compose up internetstandards/Internet.nl#1490

Open

idsulik requested a review from ndeloof September 19, 2024 04:18

idsulik and others added 6 commits October 6, 2024 17:10

fix(hash): recreate container on project config content change

c13725f

Signed-off-by: Suleiman Dibirov <[email protected]>

Revert "fix(hash): recreate container on project config content change"

555c2f9

This reverts commit 64c37bf. Signed-off-by: Suleiman Dibirov <[email protected]>

feat: Add service dependencies hash

ce7fe8f

Signed-off-by: Suleiman Dibirov <[email protected]>

feat: add separate hashes for service configs and secrets, add file f…

d4f3c7b

…older support Signed-off-by: Suleiman Dibirov <[email protected]>

Renamed variable

c4d8087

updated hash.go and tar.go

18ca016

Signed-off-by: Suleiman Dibirov <[email protected]>

idsulik force-pushed the issue-11900 branch from 2aa70c0 to 18ca016 Compare October 6, 2024 15:29

idsulik force-pushed the issue-11900 branch from 21d0e4c to cc14e73 Compare October 6, 2024 17:15

lint fixes

9695d57

add tar.go

309fd59

idsulik force-pushed the issue-11900 branch from cc14e73 to 309fd59 Compare October 6, 2024 17:41

ndeloof reviewed Oct 8, 2024

View reviewed changes

ndeloof mentioned this pull request Oct 24, 2024

[BUG] docker inline config when changed is not being taken into account #11900

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(hash): recreate container on project config content change #11931

fix(hash): recreate container on project config content change #11931

idsulik commented Jun 23, 2024 •

edited

Loading

ndeloof commented Jul 1, 2024

ndeloof Jul 2, 2024

idsulik Jul 6, 2024

idsulik commented Jul 6, 2024

ndeloof commented Jul 8, 2024

jhrotko Jul 9, 2024 •

edited

Loading

idsulik Jul 10, 2024

ndeloof commented Jul 10, 2024

idsulik commented Jul 11, 2024 •

edited

Loading

ndeloof Jul 22, 2024

idsulik Jul 22, 2024

ndeloof Sep 18, 2024

idsulik Sep 18, 2024

idsulik Oct 6, 2024

ndeloof Oct 6, 2024

idsulik Oct 6, 2024

codecov bot commented Oct 6, 2024

ndeloof Oct 8, 2024 •

edited

Loading

idsulik Oct 8, 2024

ndeloof Oct 8, 2024

idsulik Oct 8, 2024

ndeloof Oct 8, 2024 •

edited

Loading

ndeloof Oct 8, 2024

fix(hash): recreate container on project config content change #11931

Are you sure you want to change the base?

fix(hash): recreate container on project config content change #11931

Conversation

idsulik commented Jun 23, 2024 • edited Loading

ndeloof commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idsulik commented Jul 6, 2024

ndeloof commented Jul 8, 2024

jhrotko Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndeloof commented Jul 10, 2024

idsulik commented Jul 11, 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

codecov bot commented Oct 6, 2024

Codecov Report

ndeloof Oct 8, 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

ndeloof Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idsulik commented Jun 23, 2024 •

edited

Loading

jhrotko Jul 9, 2024 •

edited

Loading

idsulik commented Jul 11, 2024 •

edited

Loading

ndeloof Oct 8, 2024 •

edited

Loading

ndeloof Oct 8, 2024 •

edited

Loading