Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/api/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ const (
ServiceLabel = "com.docker.compose.service"
// ConfigHashLabel stores configuration hash for a compose service
ConfigHashLabel = "com.docker.compose.config-hash"
// ServiceConfigsHash stores configuration hash for a compose service configs
ServiceConfigsHash = "com.docker.compose.service.configs-hash"
// ServiceSecretsHash stores configuration hash for a compose service secrets
ServiceSecretsHash = "com.docker.compose.service.secrets-hash"
// ContainerNumberLabel stores the container index of a replicated service
ContainerNumberLabel = "com.docker.compose.container-number"
// VolumeLabel allow to track resource related to a compose volume
Expand Down
36 changes: 28 additions & 8 deletions pkg/compose/convergence.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,11 @@ func (c *convergence) ensureService(ctx context.Context, project *types.Project,

sort.Slice(containers, func(i, j int) bool {
// select obsolete containers first, so they get removed as we scale down
if obsolete, _ := mustRecreate(service, containers[i], recreate); obsolete {
if obsolete, _ := mustRecreate(project, service, containers[i], recreate); obsolete {
// i is obsolete, so must be first in the list
return true
}
if obsolete, _ := mustRecreate(service, containers[j], recreate); obsolete {
if obsolete, _ := mustRecreate(project, service, containers[j], recreate); obsolete {
// j is obsolete, so must be first in the list
return false
}
Expand All @@ -154,7 +154,7 @@ func (c *convergence) ensureService(ctx context.Context, project *types.Project,
continue
}

mustRecreate, err := mustRecreate(service, container, recreate)
mustRecreate, err := mustRecreate(project, service, container, recreate)
if err != nil {
return err
}
Expand Down Expand Up @@ -312,20 +312,40 @@ func (c *convergence) resolveSharedNamespaces(service *types.ServiceConfig) erro
return nil
}

func mustRecreate(expected types.ServiceConfig, actual moby.Container, policy string) (bool, error) {
func mustRecreate(project *types.Project, expected types.ServiceConfig, actual moby.Container, policy string) (bool, error) {
if policy == api.RecreateNever {
return false, nil
}
if policy == api.RecreateForce {
return true, nil
}
configHash, err := ServiceHash(expected)
serviceHash, err := ServiceHash(expected)
if err != nil {
return false, err
}
configChanged := actual.Labels[api.ConfigHashLabel] != configHash
imageUpdated := actual.Labels[api.ImageDigestLabel] != expected.CustomLabels[api.ImageDigestLabel]
return configChanged || imageUpdated, nil

if actual.Labels[api.ConfigHashLabel] != serviceHash {
return true, nil
}

if actual.Labels[api.ImageDigestLabel] != expected.CustomLabels[api.ImageDigestLabel] {
return true, nil
}

serviceConfigsHash, err := ServiceConfigsHash(project, expected)
if err != nil {
return false, err
}

serviceSecretsHash, err := ServiceSecretsHash(project, expected)
if err != nil {
return false, err
}

serviceConfigsChanged := actual.Labels[api.ServiceConfigsHash] != serviceConfigsHash
serviceSecretsChanged := actual.Labels[api.ServiceSecretsHash] != serviceSecretsHash

return serviceConfigsChanged || serviceSecretsChanged, nil
}

func getContainerName(projectName string, service types.ServiceConfig, number int) string {
Expand Down
20 changes: 16 additions & 4 deletions pkg/compose/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ func (s *composeService) getCreateConfigs(ctx context.Context,
inherit *moby.Container,
opts createOptions,
) (createConfigs, error) {
labels, err := s.prepareLabels(opts.Labels, service, number)
labels, err := s.prepareLabels(opts.Labels, p, service, number)
if err != nil {
return createConfigs{}, err
}
Expand Down Expand Up @@ -498,13 +498,25 @@ func parseSecurityOpts(p *types.Project, securityOpts []string) ([]string, bool,
return parsed, unconfined, nil
}

func (s *composeService) prepareLabels(labels types.Labels, service types.ServiceConfig, number int) (map[string]string, error) {
hash, err := ServiceHash(service)
func (s *composeService) prepareLabels(labels types.Labels, project *types.Project, service types.ServiceConfig, number int) (map[string]string, error) {
serviceHash, err := ServiceHash(service)
if err != nil {
return nil, err
}
labels[api.ConfigHashLabel] = hash

serviceConfigsHash, err := ServiceConfigsHash(project, service)
if err != nil {
return nil, err
}

serviceSecretsHash, err := ServiceSecretsHash(project, service)
if err != nil {
return nil, err
}

labels[api.ConfigHashLabel] = serviceHash
labels[api.ServiceConfigsHash] = serviceConfigsHash
labels[api.ServiceSecretsHash] = serviceSecretsHash
labels[api.ContainerNumberLabel] = strconv.Itoa(number)

var dependencies []string
Expand Down
74 changes: 72 additions & 2 deletions pkg/compose/hash.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@
package compose

import (
"bytes"
"encoding/json"
"fmt"
"time"

"github.com/compose-spec/compose-go/v2/types"
"github.com/docker/compose/v2/pkg/utils"
"github.com/opencontainers/go-digest"
)

Expand All @@ -34,9 +38,75 @@ func ServiceHash(o types.ServiceConfig) (string, error) {
}
o.DependsOn = nil

bytes, err := json.Marshal(o)
data, err := json.Marshal(o)
if err != nil {
return "", err
}
return digest.SHA256.FromBytes(bytes).Encoded(), nil
return digest.SHA256.FromBytes(data).Encoded(), nil
}

// ServiceConfigsHash computes the configuration hash for service configs.
func ServiceConfigsHash(project *types.Project, serviceConfig types.ServiceConfig) (string, error) {
data := make([]byte, 0)
for _, config := range serviceConfig.Configs {
file := project.Configs[config.Source]
b, err := createTarForConfig(project, types.FileReferenceConfig(config), types.FileObjectConfig(file))

if err != nil {
return "", err
}

data = append(data, b.Bytes()...)
}

return digest.SHA256.FromBytes(data).Encoded(), nil
Copy link
Contributor

@ndeloof ndeloof Oct 8, 2024

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you mean something like that:

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

?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ndeloof ndeloof Oct 8, 2024

Choose a reason for hiding this comment

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

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
  }
}

}

// ServiceSecretsHash computes the configuration hash for service secrets.
func ServiceSecretsHash(project *types.Project, serviceConfig types.ServiceConfig) (string, error) {
data := make([]byte, 0)
for _, secret := range serviceConfig.Secrets {
file := project.Secrets[secret.Source]
b, err := createTarForConfig(project, types.FileReferenceConfig(secret), types.FileObjectConfig(file))

if err != nil {
return "", err
}

data = append(data, b.Bytes()...)
}

return digest.SHA256.FromBytes(data).Encoded(), nil
}

func createTarForConfig(
project *types.Project,
serviceConfig types.FileReferenceConfig,
file types.FileObjectConfig,
) (*bytes.Buffer, error) {
// fixed time to ensure the tarball is deterministic
modTime := time.Unix(0, 0)

if serviceConfig.Target == "" {
serviceConfig.Target = "/" + serviceConfig.Source
}

switch {
case file.Content != "":
return bytes.NewBuffer([]byte(file.Content)), nil
case file.Environment != "":
env, ok := project.Environment[file.Environment]
if !ok {
return nil, fmt.Errorf(
"environment variable %q required by file %q is not set",
file.Environment,
file.Name,
)
}
return bytes.NewBuffer([]byte(env)), nil
case file.File != "":
return utils.CreateTarByPath(file.File, modTime)
}

return nil, fmt.Errorf("config %q is empty", file.Name)
}
150 changes: 145 additions & 5 deletions pkg/compose/hash_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,161 @@ import (
"gotest.tools/v3/assert"
)

func TestServiceHash(t *testing.T) {
hash1, err := ServiceHash(serviceConfig(1))
func TestServiceHashWithAllValuesTheSame(t *testing.T) {
hash1, err := ServiceHash(serviceConfig("myContext1", "always", 1))
assert.NilError(t, err)
hash2, err := ServiceHash(serviceConfig(2))
hash2, err := ServiceHash(serviceConfig("myContext1", "always", 1))
assert.NilError(t, err)
assert.Equal(t, hash1, hash2)
}

func serviceConfig(replicas int) types.ServiceConfig {
func TestServiceHashWithIgnorableValues(t *testing.T) {
hash1, err := ServiceHash(serviceConfig("myContext1", "always", 1))
assert.NilError(t, err)
hash2, err := ServiceHash(serviceConfig("myContext2", "never", 2))
assert.NilError(t, err)
assert.Equal(t, hash1, hash2)
}

func TestServiceConfigsHashWithoutChangesContent(t *testing.T) {
hash1, err := ServiceConfigsHash(projectWithConfigs("a", "", ""), serviceConfig("myContext1", "always", 1))
assert.NilError(t, err)
hash2, err := ServiceConfigsHash(projectWithConfigs("a", "", ""), serviceConfig("myContext2", "never", 2))
assert.NilError(t, err)
assert.Assert(t, hash1 == hash2)
}

func TestServiceConfigsHashWithChangedConfigContent(t *testing.T) {
hash1, err := ServiceConfigsHash(projectWithConfigs("a", "", ""), serviceConfig("myContext1", "always", 1))
assert.NilError(t, err)
hash2, err := ServiceConfigsHash(projectWithConfigs("b", "", ""), serviceConfig("myContext2", "never", 2))
assert.NilError(t, err)
assert.Assert(t, hash1 != hash2)
}

func TestServiceConfigsHashWithChangedConfigEnvironment(t *testing.T) {
hash1, err := ServiceConfigsHash(projectWithConfigs("", "a", ""), serviceConfig("myContext1", "always", 1))
assert.NilError(t, err)
hash2, err := ServiceConfigsHash(projectWithConfigs("", "b", ""), serviceConfig("myContext2", "never", 2))
assert.NilError(t, err)
assert.Assert(t, hash1 != hash2)
}

func TestServiceConfigsHashWithChangedConfigFile(t *testing.T) {
hash1, err := ServiceConfigsHash(
projectWithConfigs("", "", "./testdata/config1.txt"),
serviceConfig("myContext1", "always", 1),
)
assert.NilError(t, err)
hash2, err := ServiceConfigsHash(
projectWithConfigs("", "", "./testdata/config2.txt"),
serviceConfig("myContext2", "never", 2),
)
assert.NilError(t, err)
assert.Assert(t, hash1 != hash2)
}

func TestServiceSecretsHashWithoutChangesContent(t *testing.T) {
hash1, err := ServiceSecretsHash(projectWithSecrets("a", "", ""), serviceConfig("myContext1", "always", 1))
assert.NilError(t, err)
hash2, err := ServiceSecretsHash(projectWithSecrets("a", "", ""), serviceConfig("myContext2", "never", 2))
assert.NilError(t, err)
assert.Assert(t, hash1 == hash2)
}

func TestServiceSecretsHashWithChangedSecretContent(t *testing.T) {
hash1, err := ServiceSecretsHash(projectWithSecrets("a", "", ""), serviceConfig("myContext1", "always", 1))
assert.NilError(t, err)
hash2, err := ServiceSecretsHash(projectWithSecrets("b", "", ""), serviceConfig("myContext2", "never", 2))
assert.NilError(t, err)
assert.Assert(t, hash1 != hash2)
}

func TestServiceSecretsHashWithChangedSecretEnvironment(t *testing.T) {
hash1, err := ServiceSecretsHash(projectWithSecrets("", "a", ""), serviceConfig("myContext1", "always", 1))
assert.NilError(t, err)
hash2, err := ServiceSecretsHash(projectWithSecrets("", "b", ""), serviceConfig("myContext2", "never", 2))
assert.NilError(t, err)
assert.Assert(t, hash1 != hash2)
}

func TestServiceSecretsHashWithChangedSecretFile(t *testing.T) {
hash1, err := ServiceSecretsHash(
projectWithSecrets("", "", "./testdata/config1.txt"),
serviceConfig("myContext1", "always", 1),
)
assert.NilError(t, err)
hash2, err := ServiceSecretsHash(
projectWithSecrets("", "", "./testdata/config2.txt"),
serviceConfig("myContext2", "never", 2),
)
assert.NilError(t, err)
assert.Assert(t, hash1 != hash2)
}

func projectWithConfigs(configContent, configEnvironmentValue, configFile string) *types.Project {
envName := "myEnv"

if configEnvironmentValue == "" {
envName = ""
}

return &types.Project{
Environment: types.Mapping{
envName: configEnvironmentValue,
},
Configs: types.Configs{
"myConfigSource": types.ConfigObjConfig{
Content: configContent,
Environment: envName,
File: configFile,
},
},
}
}

func projectWithSecrets(secretContent, secretEnvironmentValue, secretFile string) *types.Project {
envName := "myEnv"

if secretEnvironmentValue == "" {
envName = ""
}

return &types.Project{
Environment: types.Mapping{
envName: secretEnvironmentValue,
},
Secrets: types.Secrets{
"mySecretSource": types.SecretConfig{
Content: secretContent,
Environment: envName,
File: secretFile,
},
},
}
}

func serviceConfig(buildContext, pullPolicy string, replicas int) types.ServiceConfig {
return types.ServiceConfig{
Scale: &replicas,
Build: &types.BuildConfig{
Context: buildContext,
},
PullPolicy: pullPolicy,
Scale: &replicas,
Deploy: &types.DeployConfig{
Replicas: &replicas,
},
Name: "foo",
Image: "bar",
Configs: []types.ServiceConfigObjConfig{
{
Source: "myConfigSource",
},
},
Secrets: []types.ServiceSecretConfig{
{
Source: "mySecretSource",
},
},
}
}
Loading
Loading