Autoupdate Local #10063

ParkerVR · 2021-04-16T19:39:32Z

Introduces a new auto-update policy: "local"

Although it should probably be using NewFromLocal() and comparing image digests, the current method is to compare creation dates for autoupdate - Though I'd like to discuss before changing this system

I tested this using a modified version of the docs autoupdate instructions & it worked 'successfully'
(it required the ctr to be stopped before updating it- is this proper?)

Todo List:
Digest-Based Diff Check ?
Refactor/Merge similar functions
Write Tests
Alias "image" policy as "registry"

Will build off of #9740 to write tests

Signed-off-by: Parker Van Roy [email protected]

vrothberg · 2021-04-19T09:08:54Z

pkg/autoupdate/autoupdate.go

+	//return ctr_digest == img_digest, nil
+
+	img_created := img.Created()
+	ctr_created := ctr.CreatedTime()


I think we should continue comparing digests rather than using time stamps. The latter is prone to errors.

vrothberg · 2021-04-19T09:15:18Z

pkg/autoupdate/autoupdate.go

@@ -174,6 +183,36 @@ func AutoUpdate(runtime *libpod.Runtime, options Options) ([]string, []error) {
 		}
 	}

+	// Autoupdate Local Iteration
+	for imageID, containers := range localMap {


That adds quite some duplicate code. I think we should have only loop and make the needsUpdate check conditional on the policy. We could achieve that by merging the maps into one.

map[imageUpdate][]*libpod.Container with

type imageUpdate struct { image string policy string }

That allows for bundling an image with a policy:

for imageUpdate, containers := range myMap { switch imageUpdate.policy { case ...: case ...: } }

ParkerVR · 2021-04-23T02:03:18Z

I have a working edition of the code and made some changes to refactor - new struct including the policy and changing the naming convention of the original autoupdate to 'registry'

Here is my manual test/results:
auto-update-local-test.txt

vrothberg · 2021-04-23T06:42:33Z

pkg/autoupdate/autoupdate.go

-	// PolicyNewImage is the policy to update as soon as there's a new image found.
-	PolicyNewImage = "image"
+	// PolicyRegistryImage is the policy to update as soon as there's a new image found.
+	PolicyRegistryImage = "image"


Could you change it to "registry"?

I would love us to use this term in the future as it's less ambiguous than "image". But (as you already did 👍) we still need to support "image" for backwards compat.

vrothberg · 2021-04-23T06:44:01Z

pkg/autoupdate/autoupdate.go

+}
+
+// Struct for tying a container to it's autoupdate policy
+type PolicyContainer struct {


I think this should be a private type.

vrothberg · 2021-04-23T06:50:57Z

pkg/autoupdate/autoupdate.go

@@ -198,14 +233,14 @@ func AutoUpdate(runtime *libpod.Runtime, options Options) ([]string, []error) {

 // imageContainersMap generates a map[image ID] -> [containers using the image]
 // of all containers with a valid auto-update policy.
-func imageContainersMap(runtime *libpod.Runtime) (map[string][]*libpod.Container, []error) {
+func imageContainersMap(runtime *libpod.Runtime) (map[string][]PolicyContainer, []error) {


Can you extend the comment of this function? I think it'll be helpful for future readers to understand the reasoning behind this structure:

policyContainer to bundle a policy with a container

Mapping from image to a slice of policy containers since it's possible that multiple containers refer to the same image but with different policies.

Thinking about the upper points. If the slice has [policy A, policy B, policy A] ... I think it would make sense to further bundle the items together such that we have to run a given policy only once instead of for each item individually.

A PolicyContainer could look as follows:

type policyMapper struct { map[Policy][]*libpod.Container }

Now, when executing the policies we have direct mapping image -> policy -> containers using this policy.

vrothberg

Looking good!

Along with tests, we also need to extend the man pages a bit.

I think that some may argue that having different policies for one image may be wrong (configuration error). But I haven't made my mind up on that. @ParkerVR, what do you think? Do you think we should (in theory) yield an error if image foo as containers configured with registry and local?

vrothberg · 2021-04-26T06:56:58Z

pkg/autoupdate/autoupdate.go

+
+// policyMapper is used for tying a container to it's autoupdate policy
+type policyMapper struct {
+	pMap map[Policy][]*libpod.Container


Nit: we could rewrite it to type policyMappyer map[Policy][]*libpod.Container

vrothberg · 2021-04-26T07:00:43Z

pkg/autoupdate/autoupdate.go

+			policyMap.pMap[policy] = append(policyMap.pMap[policy], ctr)
+			containerMap[id] = policyMap
+			// Now we know that `ctr` is configured for auto updates.
+		} else {


If we invert the branch logic, we can get rid of one branch:

if policy == PolicyDefault { continue } id, _ := ...

ParkerVR · 2021-04-27T19:03:22Z

@vrothberg to address your concern with multiple configurations, I think while this could be implemented I don't see a realistic use and as we just use a difference in digests it doesn't seem particularly rational to implement - so, I'm adding an error case.
On that note, is it even possible to use multiple labels like this- 'io.containers.autoupdate=local', 'io.containers.autoupdate=registry' would not overwrite each other?

Digests were used to compare local image and container image Registry alias added for Image Policy Refactored to integrate new feature + change some naming conventions Tested this using a modified version of the docs autoupdate instructions & it worked successfully Signed-off-by: Parker Van Roy <[email protected]>

Signed-off-by: Parker Van Roy <[email protected]>

vrothberg · 2021-04-28T07:26:06Z

docs/source/markdown/podman-auto-update.1.md

 An image is considered updated if the digest in the local storage is different than the one of the remote image.
 If an image must be updated, Podman pulls it down and restarts the systemd unit executing the container.

+The registry policy requires a requires a fully-qualified image reference (e.g., quay.io/podman/stable:latest) to be used to create the container.
+This enforcement is necessary to know which image to actually check and pull.
+If an image ID was used, Podman would not know which image to check/pull anymore.


Note that "registry" and "image" refer to the same policy for backwards compatibility.

vrothberg · 2021-04-28T07:33:20Z

test/system/250-systemd.bats

@@ -42,7 +42,7 @@ function teardown() {

    cname=$(random_string)
    # See #7407 for --pull=always.
-    run_podman create --pull=always --name $cname --label "io.containers.autoupdate=image" $IMAGE top
+    run_podman create --pull=always --name $cname --label "io.containers.autoupdate=local" $IMAGE top


Please create a new test for the local policy. --pull=always and local conflict a bit and the TODO not below is a rather important reminder of open work items.

vrothberg

Besides the nits in the system tests, LGTM

changed struct to policyMapper change "image" to "registry" in multiple locations Updated documentation with registry alias & autoupdate local Added relevant test Signed-off-by: Parker Van Roy <[email protected]>

jwhonce · 2021-04-29T17:25:33Z

/lgtm

mheon · 2021-04-29T17:36:32Z

LGTM

mheon · 2021-04-29T17:36:45Z

/approve

openshift-ci-robot · 2021-04-29T17:36:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, ParkerVR

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ParkerVR requested a review from vrothberg April 16, 2021 19:39

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 16, 2021

vrothberg reviewed Apr 19, 2021

View reviewed changes

ParkerVR linked an issue Apr 20, 2021 that may be closed by this pull request

auto update without registry on local images #9088

Closed

ParkerVR removed a link to an issue Apr 20, 2021

auto update without registry on local images #9088

Closed

ParkerVR force-pushed the autoupdate-local branch 4 times, most recently from a1261c9 to 7966d6c Compare April 23, 2021 00:51

ParkerVR requested review from vrothberg, jwhonce and mheon April 23, 2021 01:15

vrothberg reviewed Apr 23, 2021

View reviewed changes

ParkerVR requested a review from vrothberg April 23, 2021 20:04

vrothberg reviewed Apr 26, 2021

View reviewed changes

ParkerVR added 2 commits April 28, 2021 01:54

Vendored

150e2a3

Signed-off-by: Parker Van Roy <[email protected]>

ParkerVR force-pushed the autoupdate-local branch 4 times, most recently from 996d03b to d10d8e8 Compare April 28, 2021 06:41

ParkerVR requested a review from vrothberg April 28, 2021 07:23

vrothberg reviewed Apr 28, 2021

View reviewed changes

ParkerVR force-pushed the autoupdate-local branch from d10d8e8 to 571345f Compare April 28, 2021 20:38

ParkerVR changed the title ~~[WIP] Autoupdate Local~~ Autoupdate Local Apr 28, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2021

ParkerVR force-pushed the autoupdate-local branch from 571345f to d951415 Compare April 29, 2021 05:17

TODO complete

caf19ef

changed struct to policyMapper change "image" to "registry" in multiple locations Updated documentation with registry alias & autoupdate local Added relevant test Signed-off-by: Parker Van Roy <[email protected]>

ParkerVR force-pushed the autoupdate-local branch from d951415 to caf19ef Compare April 29, 2021 06:44

ParkerVR requested a review from vrothberg April 29, 2021 15:04

openshift-ci-robot assigned jwhonce Apr 29, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2021

openshift-merge-robot merged commit 4d2ba32 into containers:master Apr 29, 2021

vrothberg mentioned this pull request Apr 30, 2021

auto update without registry on local images #9088

Closed

edsantiago mentioned this pull request Sep 26, 2022

auto-update: validate container image #15933

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autoupdate Local #10063

Autoupdate Local #10063

ParkerVR commented Apr 16, 2021

vrothberg Apr 19, 2021

vrothberg Apr 19, 2021

ParkerVR commented Apr 23, 2021

vrothberg Apr 23, 2021

vrothberg Apr 23, 2021

vrothberg Apr 23, 2021

vrothberg left a comment

vrothberg Apr 26, 2021

vrothberg Apr 26, 2021

ParkerVR commented Apr 27, 2021

vrothberg Apr 28, 2021

vrothberg Apr 28, 2021

vrothberg left a comment

jwhonce commented Apr 29, 2021

mheon commented Apr 29, 2021

mheon commented Apr 29, 2021

openshift-ci-robot commented Apr 29, 2021

Autoupdate Local #10063

Autoupdate Local #10063

Conversation

ParkerVR commented Apr 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParkerVR commented Apr 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParkerVR commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

jwhonce commented Apr 29, 2021

mheon commented Apr 29, 2021

mheon commented Apr 29, 2021

openshift-ci-robot commented Apr 29, 2021