docker::image with image_tag=latest does not apply idempotently

[h-haaks]

Describe the Bug

When using docker::image or docker::images with image_tag=latest puppet reports changes on evry puppet run.

Expected Behavior

I would expect docker::image to report changes only when the image digest is changed and a pull is really necessary.

Environment

• Version [5.10.1]
• Platform [RHEL7]

Additional Context

It looks like this bug was introduced in #553 which created exec { $image_install }.
The right way to solve this would be to check if a pull is required and then do a pull.

We are using something like this
LOCAL_SHA="$(docker image ls --digests ${REGISTRY_HOST}:${REGISTRY_PORT}/${REGISTRY_PATH} --format '{{json . }}' | jq ". | select( .Tag == \"$TAG\" ) | .Digest" | tr -d '"')"
REMOTE_SHA="$(curl -sfH 'Accept: application/vnd.docker.distribution.manifest.v2+json' -I http://${REGISTRY_HOST}:${REGISTRY_PORT}/v2/${REGISTRY_PATH}/manifests/${TAG} | tr -d '\r' | grep 'Docker-Content-Digest' | awk '{print $2}')"
[[ "$LOCAL_SHA" != "$REMOTE_SHA" ]]

The curl in REMOTE_SHA does not use auth, and I'm not sure how to implement it with auth in the module.
Thats why I have not created i pull request.

We use the script like this in our profile class
`
$instances = lookup('docker::run_instance::instance', undef, undef, undef)
if $instances != undef {
contain docker::run_instance
Class['::docker'] -> Class['docker::run_instance']
}
if $instances != undef {
$instances.each |$key, $value| {
if $value['ensure'] != 'absent' {
$registry_host = $value['image'].split('/')[0].split(':')[0]
$registry_port = $value['image'].split('/')[0].split(':')[1]
$tag = $value['image'].split('/')[-1].split(':')[1]
$path = $value['image'].split('/').reduce |$memo, $value| {
if $memo =~ $registry_host {
if $value =~ $tag {
"${value.split(':')[0]}"
} else {
$value
}
} elsif $value =~ $tag {
"${memo}/${value.split(':')[0]}"
} else {
"${memo}/${value}"
}
}

  exec {"${title} image digest changed ${key}":
    command => 'echo',
    path    => ['/bin', '/usr/bin', '/usr/local/bin'],
    onlyif  => "docker-image-digest-changed.sh standalone ${$registry_host} ${$registry_port} ${$path} ${$tag}",
    require => [
      File['/usr/local/bin/docker-image-digest-changed.sh'],
    ],
    notify  => [
      Exec["${title} docker rm ${key}"],
      Exec["${title} docker pull ${$value['image']}"]
    ]
  }
  exec {"${title} docker rm ${key}":
    command     => "docker rm -f ${key}",
    path        => ['/bin', '/usr/bin'],
    onlyif      => "docker ps -a | awk '{print \$NF}' | grep -qw '${key}'",
    refreshonly => true,
    require     => Exec["${title} docker pull ${$value['image']}"],
    before      => Docker::Run[$key],
  }
  # Multiple instanses could use the same image
  if !defined(Exec["${title} docker pull ${$value['image']}"]) {
    exec {"${title} docker pull ${$value['image']}":
      command     => "docker pull ${$value['image']}",
      path        => ['/bin', '/usr/bin'],
      refreshonly => true,
    }
  }
}

}
}
`

[h-haaks]

We use kind of the same solution to keep docker services up to date.
To find local sha we use
LOCAL_SHA="$(docker service inspect --format '{{.Spec.TaskTemplate.ContainerSpec.Image}}' $SERVICE_NAME | cut -d@ -f2)"

[voiprodrigo]

When I set image_tag to 'latest', not only the update script seems to run twice, the container is restarted every time Puppet runs.

[voiprodrigo]

If there's a good workaround for this, please do share. Thanks.

[github-actions]

This issue has been marked as stale because it has been open for a while and has had no recent activity. If this issue is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

[ghomem]

This should be kept open and if possibly clarified. Idempotency is critical.

[chelnak]

Hey @ghomem thank you for your response and I agree!

Have a great day!

[MalteMagnussen]

@chelnak So, if I use image tag latest with docker::run, and I then upload an update to my image, will docker::run automatically fetch the new image from our registry?

Like:

1. docker::run with image tag latest
2. upload image update with tag latest to our registry (From CI/CD pipeline fx)
3. on next puppet run, will docker::run update the container to use new image?

This would allow us to deploy on demand, instead of having to upload image with specific version, and then change our puppet code to point at the new version.

Would save us time.

[voiprodrigo]

No. The hash is not checked and there's no re-pull. You'd need to change the tag. Or you can add an additional exec after the run that does that work for you.

[kenyon]

With docker::run you can set $pull_on_start to true, or equivalently, set $extra_parameters to --pull=always (if your docker client supports that option).

[github-actions]

Hello! 👋

This issue has been open for a while and has had no recent activity. We've labelled it with attention-needed so that we can get a clear view of which issues need our attention.

If you are waiting on a response from us we will try and address your comments on a future Community Day.

Alternatively, if it is no longer relevant to you please close the issue with a comment.

[shoddyguard]

This has been closed but still appears to be an issue with the latest release.

[pmcmaw]

Apologies, this was an oversight, reopened and thank you for flagging @shoddyguard

[awoodrow-t17]

I was tearing my hair out trying to figure out what I was doing wrong with this module. So glad this issue is open - I'm experiencing exactly the same problem. This really needs to be fixed.

Version 6.0.2 running on Debian 10 with Docker version 20.10.23

[chelnak]

Hey everyone! While this is on our radar it's something we may not get to just yet.

In the meantime PRs are welcome which we are more than happy to help get through.

[sasubillis]

This is still a problem with the following.

docker-compose version 1.28.4
Docker version 24.0.4
Debian 11

[gerdriesselmann]

We face this problem in version 9.1.0

Since puppet runs every 10 minutes on all our machines, it restarts nearly all our docker containers that use latest images.

This module unfortunately is unusable the way it is. This bug IMO is a showstopper, and I wonder why it hasn't been fixes for more than 3 years now.

[gerdriesselmann]

We fixed this by turning the execution of $image_install from an exec to an onlyif dependency in image.pp, lines 146pp:

 } elsif $ensure == 'latest' or $image_tag == 'latest' or $force {
    /*
    notify { "Check if image ${image_arg} is in-sync":
      noop => false,
    }
    ~> exec { $image_install:
      environment => $exec_environment,
      path        => $exec_path,
      timeout     => $exec_timeout,
      returns     => ['0', '2'],
      require     => File[$update_docker_image_path],
      provider    => $exec_provider,
      logoutput   => true,
    }
    ~> */ exec { "echo 'Update of ${image_arg} complete'":
      environment => $exec_environment,
      path        => $exec_path,
      timeout     => $exec_timeout,
      require     => File[$update_docker_image_path],
      provider    => $exec_provider,
      logoutput   => true,
      #refreshonly => true,
      onlyif      => $image_install,
    }
  } elsif $ensure == 'present' {

We use it like this:

docker::image { $image:
	ensure    => present,
	image_tag => "${version}",
}

docker::run { "${title}":
	image => "$image:$version",
        [...]
	remove_container_on_stop => false,
	restart_service_on_docker_refresh => false,
	subscribe => Docker::Image["${image}"],
}

[ghomem]

@gerdriesselmann

Thank you for sharing this.

Would it be possible for you to fork the puppetlabs-docker to your github account and introduce the fix?

From there it would be easy to inspect the diff and maybe produce a PR.

[ghomem]

This issue is critical for whoever wants to maintain CI environments based on puppetlabs-docker . Having the images declared with fixed tags, if it works, allows for simple CI pipelines (that merely execute the puppet agent), having the complexity kept where it belongs: structured puppet code rather than horrible repo side YAML.

(pardon the opinionated text :-) )

[gerdriesselmann]

@ghomem The reason I hesitate to provide a PR is: The fix only works if docker::run subscribes to the Image class, which is not how the module is designed and documented to work at the moment. So this will be a breaking change.

This does not pose a problem for us, since we wrapped up all docker management in a distinct class, so there is only one place to change. But for others, it may well be more difficult.

[ghomem]

@ghomem if does not shock me that the run resource subscribes the image resource.

By forking the repo you allow people to check the diff and download directly your version instead of doing changes by hand. It is a cleaner process, even if no PR is merged back.

[gerdriesselmann]

@ghomem You are right. Here is the fork: https://github.com/gerdriesselmann/puppetlabs-docker/, and here is the commit: main...gerdriesselmann:puppetlabs-docker:main

[ghomem]

@gerdriesselmann very cool! the patch is very surgical! Thanks for the contribution. I might have an opportunity to try it soon and this might be extremely helpful for others too. Cheers!