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

Allow images tagged latest to update on each run #468

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

electrofelix
Copy link
Contributor

Restore the code to update images tagged with 'latest' on each run,
moving this to be guarded by a refreshonly meta-parameter that is
triggered by a notify resource that is disabled for 'noop'.

Because the original code to update images was run via 'onlyif' to
indicate if the image was in sync by performing a pull and testing the
new digest against the old, this meant that simply applying 'noop' to
this exec resource would not work. 'noop' does not affect running the
components of a resource to validate if it is in-sync, which is the
purpose of the 'onlyif' & 'unless' settings for an exec.

To support 'noop' mode, previously the code to update the image on each
run was dropped to prevent it accidentally refreshing the local image
and then skipping an image restart because there was no update event on
a subsequent real run.

To resolve, it is necessary to have a prior resource that is used as a
proxy, and only when it notifies the image update resource, will the
'onlyif' action be tested to determine if in sync. This prior resource
can be disabled from a noop run, and subsequently although puppet
records that it would send a refresh event in 'noop' mode, none are
actually sent during a 'noop' run thus preventing the image from being
updated by a side effect of checking if the notified exec was in-sync.

Fixes #316

Restore the code to update images tagged with 'latest' on each run,
moving this to be guarded by a refreshonly meta-parameter that is
triggered by a notify resource that is disabled for 'noop'.

Because the original code to update images was run via 'onlyif' to
indicate if the image was in sync by performing a pull and testing the
new digest against the old, this meant that simply applying 'noop' to
this exec resource would not work. 'noop' does not affect running the
components of a resource to validate if it is in-sync, which is the
purpose of the 'onlyif' & 'unless' settings for an exec.

To support 'noop' mode, previously the code to update the image on each
run was dropped to prevent it accidentally refreshing the local image
and then skipping an image restart because there was no update event on
a subsequent real run.

To resolve, it is necessary to have a prior resource that is used as a
proxy, and only when it notifies the image update resource, will the
'onlyif' action be tested to determine if in sync. This prior resource
can be disabled from a noop run, and subsequently although puppet
records that it would send a refresh event in 'noop' mode, none are
actually sent during a 'noop' run thus preventing the image from being
updated by a side effect of checking if the notified exec was in-sync.

Fixes puppetlabs#316
@electrofelix
Copy link
Contributor Author

What I'm not sure about for this change, is whether it's possible to spec test the noop mode to make sure the behaviour is not broken in the future.

Testing locally - initial state

vagrant@srv-1:~$ docker inspect --type image --format='{{.Id}}' 192.168.32.1:5000/jenkins:latest
sha256:c34f57b44244e965db8ad324ef6abe64b1bb026c2a02756d8af7a64028fff2de

Build and published an updated image to my local registry (192.168.32.1 refers to my host machine used by my test vagrant-libvirt VMs)
Executed puppet agent in 'noop' mode:

vagrant@srv-1:~$ sudo /opt/puppetlabs/bin/puppet agent --test --noop
Info: Using configured environment 'vagrant'
Info: Retrieving pluginfacts
Info: Retrieving plugin
Info: Retrieving locales
Info: Applying configuration version '1554306803'
...
Notice: /Stage[main]/Local::Jenkins/Local::Docker::Run[jenkins]/Notify[Getting image: 192.168.32.1:5000/jenkins:latest]/message: current_value 'absent', should be 'Getting image: 192.168.32.1:5000/jenkins:latest' (noop)
Notice: Refresh of 192.168.32.1:5000/jenkins:latest image
Notice: /Stage[main]/Local::Jenkins/Local::Docker::Run[jenkins]/Docker::Image[192.168.32.1:5000/jenkins:latest]/Notify[Refresh of 192.168.32.1:5000/jenkins:latest image]/message: defined 'message' as 'Refresh of 192.168.32.1:5000/jenkins:latest image'
Info: /Stage[main]/Local::Jenkins/Local::Docker::Run[jenkins]/Docker::Image[192.168.32.1:5000/jenkins:latest]/Notify[Refresh of 192.168.32.1:5000/jenkins:latest image]: Scheduling refresh of Exec[echo 'Update of 192.168.32.1:5000/jenkins:latest complete']
Notice: /Stage[main]/Local::Jenkins/Local::Docker::Run[jenkins]/Docker::Image[192.168.32.1:5000/jenkins:latest]/Exec[echo 'Update of 192.168.32.1:5000/jenkins:latest complete']: Would have triggered 'refresh' from 1 event
Notice: Docker::Image[192.168.32.1:5000/jenkins:latest]: Would have triggered 'refresh' from 2 events

The message about Would have triggered refresh on echo 'Update of 192.168.32.1:5000/jenkins:latest complete' is the important one.

Confirm that it's not actually been updated

vagrant@srv-1:~$ docker inspect --type image --format='{{.Id}}' 192.168.32.1:5000/jenkins:latest
sha256:c34f57b44244e965db8ad324ef6abe64b1bb026c2a02756d8af7a64028fff2de

Perform the same puppet agent run without --noop

sudo /opt/puppetlabs/bin/puppet agent --test
Info: Using configured environment 'vagrant'
Info: Retrieving pluginfacts
Info: Retrieving plugin
Info: Retrieving locales
...
Notice: Getting image: 192.168.32.1:5000/jenkins:latest
Notice: /Stage[main]/Local::Jenkins/Local::Docker::Run[jenkins]/Notify[Getting image: 192.168.32.1:5000/jenkins:latest]/message: defined 'message' as 'Getting image: 192.168.32.1:5000/jenkins:latest'
Notice: Refresh of 192.168.32.1:5000/jenkins:latest image
Notice: /Stage[main]/Local::Jenkins/Local::Docker::Run[jenkins]/Docker::Image[192.168.32.1:5000/jenkins:latest]/Notify[Refresh of 192.168.32.1:5000/jenkins:latest image]/message: defined 'message' as 'Refresh of 192.168.32.1:5000/jenkins:latest image'
Info: /Stage[main]/Local::Jenkins/Local::Docker::Run[jenkins]/Docker::Image[192.168.32.1:5000/jenkins:latest]/Notify[Refresh of 192.168.32.1:5000/jenkins:latest image]: Scheduling refresh of Exec[echo 'Update of 192.168.32.1:5000/jenkins:latest complete']
Notice: /Stage[main]/Local::Jenkins/Local::Docker::Run[jenkins]/Docker::Image[192.168.32.1:5000/jenkins:latest]/Exec[echo 'Update of 192.168.32.1:5000/jenkins:latest complete']/returns: Update of 192.168.32.1:5000/jenkins:latest complete
Notice: /Stage[main]/Local::Jenkins/Local::Docker::Run[jenkins]/Docker::Image[192.168.32.1:5000/jenkins:latest]/Exec[echo 'Update of 192.168.32.1:5000/jenkins:latest complete']: Triggered 'refresh' from 1 event

Confirm it is now updated

vagrant@srv-1:~$ docker inspect --type image --format='{{.Id}}' 192.168.32.1:5000/jenkins:latest
sha256:ffdb60d16687665c255cb456bd898d065f9971e1b7f951b325b87e18feac748b
vagrant@srv-1:~$

@electrofelix
Copy link
Contributor Author

@davejrt is this PR a reasonable approach to fixing the issue referenced in #316 ? Any suggestions on whether there is a good acceptance test possible for this? I'm thinking the only way would involve the following steps:

  1. Start a local registry
  2. Build a dummy image from scratch
  3. Push to the local registry
  4. Apply a manifest referencing this image with version set to latest
  5. Build a modified version of the image (change the message outputted)
  6. Apply the manifest with :noop => true (would this be picked up)
  7. Validate that the image within the acceptance VM is still the old SHA256
  8. Apply the manifest without :noop => true again
  9. Validate that the image within the acceptance VM has the updated SHA256
  10. Remove the local registry

Not sure how to run the registry without conflicting with a user running another local instance in a consistent manner. Maybe run it in the acceptance VM and push the image to it that way?

Or is this over-doing trying to test this?

@davejrt
Copy link
Contributor

davejrt commented Apr 15, 2019

Hi @electrofelix this looks good to me, and regarding your test that may be considered overkill, however logical it may be. I'm no longer a core maintainer on the module so I'll tag in @carabasdaniel to have someone triage the PR for you so that group of people are aware of any PR's.

Thanks for your contribution

@carabasdaniel
Copy link
Contributor

👍 LGTM

@carabasdaniel carabasdaniel merged commit 813dcbd into puppetlabs:master Apr 18, 2019
@electrofelix electrofelix deleted the noop-latest-images branch September 18, 2019 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

puppet --noop fix has broken ensure=>latest
4 participants