From d4760977392028f0228ba76e9c09575c97a0111e Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 28 Jul 2022 14:20:50 +0200 Subject: [PATCH 01/17] pkg/autoupdate: introduce `updater` for shared state Introduce an `updater` type to allow for sharing state. This will be more useful for future changes. [NO NEW TESTS NEEDED] as it does not change behavior. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 60 ++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 8d99916227..c792ee4e91 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -145,9 +145,15 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A runtime.NewSystemEvent(events.AutoUpdate) + auto := updater{ + conn: conn, + options: &options, + runtime: runtime, + updatedRawImages: make(map[string]bool), + } + // Update all images/container according to their auto-update policy. var allReports []*entities.AutoUpdateReport - updatedRawImages := make(map[string]bool) for imageID, policyMapper := range containerMap { image, exists := imageMap[imageID] if !exists { @@ -156,7 +162,7 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A } for _, ctr := range policyMapper[PolicyRegistryImage] { - report, err := autoUpdateRegistry(ctx, image, ctr, updatedRawImages, &options, conn, runtime) + report, err := auto.updateRegistry(ctx, image, ctr) if err != nil { errs = append(errs, err) } @@ -166,7 +172,7 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A } for _, ctr := range policyMapper[PolicyLocalImage] { - report, err := autoUpdateLocally(ctx, image, ctr, &options, conn, runtime) + report, err := auto.updateLocally(ctx, image, ctr) if err != nil { errs = append(errs, err) } @@ -179,8 +185,16 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A return allReports, errs } -// autoUpdateRegistry updates the image/container according to the "registry" policy. -func autoUpdateRegistry(ctx context.Context, image *libimage.Image, ctr *libpod.Container, updatedRawImages map[string]bool, options *entities.AutoUpdateOptions, conn *dbus.Conn, runtime *libpod.Runtime) (*entities.AutoUpdateReport, error) { +// updater includes shared state for auto-updating one or more containers. +type updater struct { + conn *dbus.Conn + options *entities.AutoUpdateOptions + updatedRawImages map[string]bool + runtime *libpod.Runtime +} + +// updateRegistry updates the image/container according to the "registry" policy. +func (u *updater) updateRegistry(ctx context.Context, image *libimage.Image, ctr *libpod.Container) (*entities.AutoUpdateReport, error) { cid := ctr.ID() rawImageName := ctr.RawImageName() if rawImageName == "" { @@ -202,16 +216,16 @@ func autoUpdateRegistry(ctx context.Context, image *libimage.Image, ctr *libpod. Updated: "failed", } - if _, updated := updatedRawImages[rawImageName]; updated { + if _, updated := u.updatedRawImages[rawImageName]; updated { logrus.Infof("Auto-updating container %q using registry image %q", cid, rawImageName) - if err := restartSystemdUnit(ctx, ctr, unit, conn); err != nil { + if err := u.restartSystemdUnit(ctx, ctr, unit); err != nil { return report, err } report.Updated = "true" return report, nil } - authfile := getAuthfilePath(ctr, options) + authfile := getAuthfilePath(ctr, u.options) needsUpdate, err := newerRemoteImageAvailable(ctx, image, rawImageName, authfile) if err != nil { return report, fmt.Errorf("registry auto-updating container %q: image check for %q failed: %w", cid, rawImageName, err) @@ -222,24 +236,24 @@ func autoUpdateRegistry(ctx context.Context, image *libimage.Image, ctr *libpod. return report, nil } - if options.DryRun { + if u.options.DryRun { report.Updated = "pending" return report, nil } - if _, err := updateImage(ctx, runtime, rawImageName, authfile); err != nil { + if _, err := updateImage(ctx, u.runtime, rawImageName, authfile); err != nil { return report, fmt.Errorf("registry auto-updating container %q: image update for %q failed: %w", cid, rawImageName, err) } - updatedRawImages[rawImageName] = true + u.updatedRawImages[rawImageName] = true logrus.Infof("Auto-updating container %q using registry image %q", cid, rawImageName) - updateErr := restartSystemdUnit(ctx, ctr, unit, conn) + updateErr := u.restartSystemdUnit(ctx, ctr, unit) if updateErr == nil { report.Updated = "true" return report, nil } - if !options.Rollback { + if !u.options.Rollback { return report, updateErr } @@ -247,7 +261,7 @@ func autoUpdateRegistry(ctx context.Context, image *libimage.Image, ctr *libpod. if err := image.Tag(rawImageName); err != nil { return report, fmt.Errorf("falling back to previous image: %w", err) } - if err := restartSystemdUnit(ctx, ctr, unit, conn); err != nil { + if err := u.restartSystemdUnit(ctx, ctr, unit); err != nil { return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) } @@ -255,8 +269,8 @@ func autoUpdateRegistry(ctx context.Context, image *libimage.Image, ctr *libpod. return report, nil } -// autoUpdateRegistry updates the image/container according to the "local" policy. -func autoUpdateLocally(ctx context.Context, image *libimage.Image, ctr *libpod.Container, options *entities.AutoUpdateOptions, conn *dbus.Conn, runtime *libpod.Runtime) (*entities.AutoUpdateReport, error) { +// updateRegistry updates the image/container according to the "local" policy. +func (u *updater) updateLocally(ctx context.Context, image *libimage.Image, ctr *libpod.Container) (*entities.AutoUpdateReport, error) { cid := ctr.ID() rawImageName := ctr.RawImageName() if rawImageName == "" { @@ -278,7 +292,7 @@ func autoUpdateLocally(ctx context.Context, image *libimage.Image, ctr *libpod.C Updated: "failed", } - needsUpdate, err := newerLocalImageAvailable(runtime, image, rawImageName) + needsUpdate, err := newerLocalImageAvailable(u.runtime, image, rawImageName) if err != nil { return report, fmt.Errorf("locally auto-updating container %q: image check for %q failed: %w", cid, rawImageName, err) } @@ -288,19 +302,19 @@ func autoUpdateLocally(ctx context.Context, image *libimage.Image, ctr *libpod.C return report, nil } - if options.DryRun { + if u.options.DryRun { report.Updated = "pending" return report, nil } logrus.Infof("Auto-updating container %q using local image %q", cid, rawImageName) - updateErr := restartSystemdUnit(ctx, ctr, unit, conn) + updateErr := u.restartSystemdUnit(ctx, ctr, unit) if updateErr == nil { report.Updated = "true" return report, nil } - if !options.Rollback { + if !u.options.Rollback { return report, updateErr } @@ -308,7 +322,7 @@ func autoUpdateLocally(ctx context.Context, image *libimage.Image, ctr *libpod.C if err := image.Tag(rawImageName); err != nil { return report, fmt.Errorf("falling back to previous image: %w", err) } - if err := restartSystemdUnit(ctx, ctr, unit, conn); err != nil { + if err := u.restartSystemdUnit(ctx, ctr, unit); err != nil { return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) } @@ -317,9 +331,9 @@ func autoUpdateLocally(ctx context.Context, image *libimage.Image, ctr *libpod.C } // restartSystemdUnit restarts the systemd unit the container is running in. -func restartSystemdUnit(ctx context.Context, ctr *libpod.Container, unit string, conn *dbus.Conn) error { +func (u *updater) restartSystemdUnit(ctx context.Context, ctr *libpod.Container, unit string) error { restartChan := make(chan string) - if _, err := conn.RestartUnitContext(ctx, unit, "replace", restartChan); err != nil { + if _, err := u.conn.RestartUnitContext(ctx, unit, "replace", restartChan); err != nil { return fmt.Errorf("auto-updating container %q: restarting systemd unit %q failed: %w", ctr.ID(), unit, err) } From 328c8ba7b4567f69602cf6bc71f7a2f0efb5c969 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 1 Aug 2022 14:22:37 +0200 Subject: [PATCH 02/17] pkg/autoupdate: move policy map into updater [NO NEW TESTS NEEDED] - should not change behavior. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 57 ++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index c792ee4e91..7bda0ac27d 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -53,6 +53,15 @@ var supportedPolicies = map[string]Policy{ // policyMapper is used for tying a container to it's autoupdate policy type policyMapper map[Policy][]*libpod.Container +// updater includes shared state for auto-updating one or more containers. +type updater struct { + conn *dbus.Conn + imageToPolicyMapper map[string]policyMapper + options *entities.AutoUpdateOptions + updatedRawImages map[string]bool + runtime *libpod.Runtime +} + // LookupPolicy looks up the corresponding Policy for the specified // string. If none is found, an errors is returned including the list of // supported policies. @@ -116,12 +125,22 @@ func ValidateImageReference(imageName string) error { // It returns a slice of successfully restarted systemd units and a slice of // errors encountered during auto update. func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.AutoUpdateOptions) ([]*entities.AutoUpdateReport, []error) { + auto := updater{ + options: &options, + runtime: runtime, + updatedRawImages: make(map[string]bool), + } + // Create a map from `image ID -> []*Container`. - containerMap, errs := imageContainersMap(runtime) - if len(containerMap) == 0 { + if errs := auto.imageContainersMap(); len(errs) > 0 { return nil, errs } + // Nothing to do. + if len(auto.imageToPolicyMapper) == 0 { + return nil, nil + } + // Create a map from `image ID -> *libimage.Image` for image lookups. listOptions := &libimage.ListImagesOptions{ Filters: []string{"readonly=false"}, @@ -142,19 +161,14 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A return nil, []error{err} } defer conn.Close() + auto.conn = conn runtime.NewSystemEvent(events.AutoUpdate) - auto := updater{ - conn: conn, - options: &options, - runtime: runtime, - updatedRawImages: make(map[string]bool), - } - // Update all images/container according to their auto-update policy. var allReports []*entities.AutoUpdateReport - for imageID, policyMapper := range containerMap { + var errs []error + for imageID, policyMapper := range auto.imageToPolicyMapper { image, exists := imageMap[imageID] if !exists { errs = append(errs, fmt.Errorf("container image ID %q not found in local storage", imageID)) @@ -185,14 +199,6 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A return allReports, errs } -// updater includes shared state for auto-updating one or more containers. -type updater struct { - conn *dbus.Conn - options *entities.AutoUpdateOptions - updatedRawImages map[string]bool - runtime *libpod.Runtime -} - // updateRegistry updates the image/container according to the "registry" policy. func (u *updater) updateRegistry(ctx context.Context, image *libimage.Image, ctr *libpod.Container) (*entities.AutoUpdateReport, error) { cid := ctr.ID() @@ -353,14 +359,15 @@ func (u *updater) restartSystemdUnit(ctx context.Context, ctr *libpod.Container, // 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]policyMapper, []error) { - allContainers, err := runtime.GetAllContainers() +func (u *updater) imageContainersMap() []error { + allContainers, err := u.runtime.GetAllContainers() if err != nil { - return nil, []error{err} + return []error{err} } + u.imageToPolicyMapper = make(map[string]policyMapper) + errors := []error{} - containerMap := make(map[string]policyMapper) for _, ctr := range allContainers { state, err := ctr.State() if err != nil { @@ -390,17 +397,17 @@ func imageContainersMap(runtime *libpod.Runtime) (map[string]policyMapper, []err continue } else { id, _ := ctr.Image() - policyMap, exists := containerMap[id] + policyMap, exists := u.imageToPolicyMapper[id] if !exists { policyMap = make(map[Policy][]*libpod.Container) } policyMap[policy] = append(policyMap[policy], ctr) - containerMap[id] = policyMap + u.imageToPolicyMapper[id] = policyMap // Now we know that `ctr` is configured for auto updates. } } - return containerMap, errors + return errors } // getAuthfilePath returns an authfile path, if set. The authfile label in the From 033cc059fa712d930d631471b91f713a08ed4e45 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 1 Aug 2022 14:23:20 +0200 Subject: [PATCH 03/17] pkg/autoupdate: remove redundant branch [NO NEW TESTS NEEDED] - should not change behavior. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 7bda0ac27d..7586468a01 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -395,16 +395,16 @@ func (u *updater) imageContainersMap() []error { // Skip labels not related to autoupdate if policy == PolicyDefault { continue - } else { - id, _ := ctr.Image() - policyMap, exists := u.imageToPolicyMapper[id] - if !exists { - policyMap = make(map[Policy][]*libpod.Container) - } - policyMap[policy] = append(policyMap[policy], ctr) - u.imageToPolicyMapper[id] = policyMap - // Now we know that `ctr` is configured for auto updates. } + + id, _ := ctr.Image() + policyMap, exists := u.imageToPolicyMapper[id] + if !exists { + policyMap = make(map[Policy][]*libpod.Container) + } + policyMap[policy] = append(policyMap[policy], ctr) + u.imageToPolicyMapper[id] = policyMap + // Now we know that `ctr` is configured for auto updates. } return errors From 87c0c760ece41ae56ec3d99f3938a84b11366521 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 2 Aug 2022 14:28:18 +0200 Subject: [PATCH 04/17] pkg/autoupdate: introduce the notion of a `task` A `task` includes data and state for updating a given container image. It will come in handy in future changes, but we are going there in baby steps to have smaller incremental changes. [NO NEW TESTS NEEDED] - should not change behaviour. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 139 +++++++++++++++++++++-------------- 1 file changed, 84 insertions(+), 55 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 7586468a01..58e9194935 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -50,18 +50,27 @@ var supportedPolicies = map[string]Policy{ "local": PolicyLocalImage, } -// policyMapper is used for tying a container to it's autoupdate policy -type policyMapper map[Policy][]*libpod.Container +// policyMappers assembles update tasks by policy +type policyMapper map[Policy][]*task // updater includes shared state for auto-updating one or more containers. type updater struct { conn *dbus.Conn + idToImage map[string]*libimage.Image imageToPolicyMapper map[string]policyMapper options *entities.AutoUpdateOptions updatedRawImages map[string]bool runtime *libpod.Runtime } +// task includes data and state for updating a container +type task struct { + container *libpod.Container // Container to update + policy Policy // Update policy + image *libimage.Image // Original image before the update + unit string // Name of the systemd unit +} + // LookupPolicy looks up the corresponding Policy for the specified // string. If none is found, an errors is returned including the list of // supported policies. @@ -110,6 +119,24 @@ func ValidateImageReference(imageName string) error { return nil } +func (u *updater) assembleImageMap(ctx context.Context) error { + // Create a map from `image ID -> *libimage.Image` for image lookups. + listOptions := &libimage.ListImagesOptions{ + Filters: []string{"readonly=false"}, + } + imagesSlice, err := u.runtime.LibimageRuntime().ListImages(ctx, nil, listOptions) + if err != nil { + return err + } + imageMap := make(map[string]*libimage.Image) + for i := range imagesSlice { + imageMap[imagesSlice[i].ID()] = imagesSlice[i] + } + + u.idToImage = imageMap + return nil +} + // AutoUpdate looks up containers with a specified auto-update policy and acts // accordingly. // @@ -131,6 +158,12 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A updatedRawImages: make(map[string]bool), } + // Assemble a map `image ID -> *libimage.Image` that we can consult + // later on for lookups. + if err := auto.assembleImageMap(ctx); err != nil { + return nil, []error{err} + } + // Create a map from `image ID -> []*Container`. if errs := auto.imageContainersMap(); len(errs) > 0 { return nil, errs @@ -141,19 +174,6 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A return nil, nil } - // Create a map from `image ID -> *libimage.Image` for image lookups. - listOptions := &libimage.ListImagesOptions{ - Filters: []string{"readonly=false"}, - } - imagesSlice, err := runtime.LibimageRuntime().ListImages(ctx, nil, listOptions) - if err != nil { - return nil, []error{err} - } - imageMap := make(map[string]*libimage.Image) - for i := range imagesSlice { - imageMap[imagesSlice[i].ID()] = imagesSlice[i] - } - // Connect to DBUS. conn, err := systemd.ConnectToDBUS() if err != nil { @@ -169,14 +189,13 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A var allReports []*entities.AutoUpdateReport var errs []error for imageID, policyMapper := range auto.imageToPolicyMapper { - image, exists := imageMap[imageID] - if !exists { + if _, exists := auto.idToImage[imageID]; !exists { errs = append(errs, fmt.Errorf("container image ID %q not found in local storage", imageID)) return nil, errs } - for _, ctr := range policyMapper[PolicyRegistryImage] { - report, err := auto.updateRegistry(ctx, image, ctr) + for _, task := range policyMapper[PolicyRegistryImage] { + report, err := auto.updateRegistry(ctx, task) if err != nil { errs = append(errs, err) } @@ -185,8 +204,8 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A } } - for _, ctr := range policyMapper[PolicyLocalImage] { - report, err := auto.updateLocally(ctx, image, ctr) + for _, task := range policyMapper[PolicyLocalImage] { + report, err := auto.updateLocally(ctx, task) if err != nil { errs = append(errs, err) } @@ -200,39 +219,37 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A } // updateRegistry updates the image/container according to the "registry" policy. -func (u *updater) updateRegistry(ctx context.Context, image *libimage.Image, ctr *libpod.Container) (*entities.AutoUpdateReport, error) { - cid := ctr.ID() - rawImageName := ctr.RawImageName() +func (u *updater) updateRegistry(ctx context.Context, task *task) (*entities.AutoUpdateReport, error) { + cid := task.container.ID() + rawImageName := task.container.RawImageName() if rawImageName == "" { return nil, fmt.Errorf("registry auto-updating container %q: raw-image name is empty", cid) } - labels := ctr.Labels() - unit, exists := labels[systemdDefine.EnvVariable] - if !exists { - return nil, fmt.Errorf("auto-updating container %q: no %s label found", ctr.ID(), systemdDefine.EnvVariable) + if task.unit == "" { + return nil, fmt.Errorf("auto-updating container %q: no %s label found", task.container.ID(), systemdDefine.EnvVariable) } report := &entities.AutoUpdateReport{ ContainerID: cid, - ContainerName: ctr.Name(), + ContainerName: task.container.Name(), ImageName: rawImageName, Policy: PolicyRegistryImage, - SystemdUnit: unit, + SystemdUnit: task.unit, Updated: "failed", } if _, updated := u.updatedRawImages[rawImageName]; updated { logrus.Infof("Auto-updating container %q using registry image %q", cid, rawImageName) - if err := u.restartSystemdUnit(ctx, ctr, unit); err != nil { + if err := u.restartSystemdUnit(ctx, task.container, task.unit); err != nil { return report, err } report.Updated = "true" return report, nil } - authfile := getAuthfilePath(ctr, u.options) - needsUpdate, err := newerRemoteImageAvailable(ctx, image, rawImageName, authfile) + authfile := getAuthfilePath(task.container, u.options) + needsUpdate, err := newerRemoteImageAvailable(ctx, task.image, rawImageName, authfile) if err != nil { return report, fmt.Errorf("registry auto-updating container %q: image check for %q failed: %w", cid, rawImageName, err) } @@ -253,7 +270,7 @@ func (u *updater) updateRegistry(ctx context.Context, image *libimage.Image, ctr u.updatedRawImages[rawImageName] = true logrus.Infof("Auto-updating container %q using registry image %q", cid, rawImageName) - updateErr := u.restartSystemdUnit(ctx, ctr, unit) + updateErr := u.restartSystemdUnit(ctx, task.container, task.unit) if updateErr == nil { report.Updated = "true" return report, nil @@ -264,10 +281,10 @@ func (u *updater) updateRegistry(ctx context.Context, image *libimage.Image, ctr } // To fallback, simply retag the old image and restart the service. - if err := image.Tag(rawImageName); err != nil { + if err := task.image.Tag(rawImageName); err != nil { return report, fmt.Errorf("falling back to previous image: %w", err) } - if err := u.restartSystemdUnit(ctx, ctr, unit); err != nil { + if err := u.restartSystemdUnit(ctx, task.container, task.unit); err != nil { return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) } @@ -276,29 +293,27 @@ func (u *updater) updateRegistry(ctx context.Context, image *libimage.Image, ctr } // updateRegistry updates the image/container according to the "local" policy. -func (u *updater) updateLocally(ctx context.Context, image *libimage.Image, ctr *libpod.Container) (*entities.AutoUpdateReport, error) { - cid := ctr.ID() - rawImageName := ctr.RawImageName() +func (u *updater) updateLocally(ctx context.Context, task *task) (*entities.AutoUpdateReport, error) { + cid := task.container.ID() + rawImageName := task.container.RawImageName() if rawImageName == "" { return nil, fmt.Errorf("locally auto-updating container %q: raw-image name is empty", cid) } - labels := ctr.Labels() - unit, exists := labels[systemdDefine.EnvVariable] - if !exists { - return nil, fmt.Errorf("auto-updating container %q: no %s label found", ctr.ID(), systemdDefine.EnvVariable) + if task.unit == "" { + return nil, fmt.Errorf("auto-updating container %q: no %s label found", task.container.ID(), systemdDefine.EnvVariable) } report := &entities.AutoUpdateReport{ ContainerID: cid, - ContainerName: ctr.Name(), + ContainerName: task.container.Name(), ImageName: rawImageName, Policy: PolicyLocalImage, - SystemdUnit: unit, + SystemdUnit: task.unit, Updated: "failed", } - needsUpdate, err := newerLocalImageAvailable(u.runtime, image, rawImageName) + needsUpdate, err := newerLocalImageAvailable(u.runtime, task.image, rawImageName) if err != nil { return report, fmt.Errorf("locally auto-updating container %q: image check for %q failed: %w", cid, rawImageName, err) } @@ -314,7 +329,7 @@ func (u *updater) updateLocally(ctx context.Context, image *libimage.Image, ctr } logrus.Infof("Auto-updating container %q using local image %q", cid, rawImageName) - updateErr := u.restartSystemdUnit(ctx, ctr, unit) + updateErr := u.restartSystemdUnit(ctx, task.container, task.unit) if updateErr == nil { report.Updated = "true" return report, nil @@ -325,10 +340,10 @@ func (u *updater) updateLocally(ctx context.Context, image *libimage.Image, ctr } // To fallback, simply retag the old image and restart the service. - if err := image.Tag(rawImageName); err != nil { + if err := task.image.Tag(rawImageName); err != nil { return report, fmt.Errorf("falling back to previous image: %w", err) } - if err := u.restartSystemdUnit(ctx, ctr, unit); err != nil { + if err := u.restartSystemdUnit(ctx, task.container, task.unit); err != nil { return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) } @@ -368,7 +383,8 @@ func (u *updater) imageContainersMap() []error { u.imageToPolicyMapper = make(map[string]policyMapper) errors := []error{} - for _, ctr := range allContainers { + for _, c := range allContainers { + ctr := c state, err := ctr.State() if err != nil { errors = append(errors, err) @@ -379,13 +395,11 @@ func (u *updater) imageContainersMap() []error { continue } - // Only update containers with the specific label/policy set. labels := ctr.Labels() value, exists := labels[Label] if !exists { continue } - policy, err := LookupPolicy(value) if err != nil { errors = append(errors, err) @@ -400,11 +414,26 @@ func (u *updater) imageContainersMap() []error { id, _ := ctr.Image() policyMap, exists := u.imageToPolicyMapper[id] if !exists { - policyMap = make(map[Policy][]*libpod.Container) + policyMap = make(map[Policy][]*task) } - policyMap[policy] = append(policyMap[policy], ctr) + + image, exists := u.idToImage[id] + if !exists { + err := fmt.Errorf("internal error: no image found for ID %s", id) + errors = append(errors, err) + continue + } + + unit, _ := labels[systemdDefine.EnvVariable] + + t := task{ + container: ctr, + policy: policy, + image: image, + unit: unit, + } + policyMap[policy] = append(policyMap[policy], &t) u.imageToPolicyMapper[id] = policyMap - // Now we know that `ctr` is configured for auto updates. } return errors From f8b6a81ae4762e6ed67af254611b0a32a7d69ea9 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 2 Aug 2022 15:37:34 +0200 Subject: [PATCH 05/17] test/system/255-auto-update.bats: add an SELinux comment Drop a comment on using `chcon` to let the local rollback test pass. It took me a while to understand why the test failed and future souls may appreciated the extra breadcrumb. Signed-off-by: Valentin Rothberg --- test/system/255-auto-update.bats | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/system/255-auto-update.bats b/test/system/255-auto-update.bats index 6cee939fbc..c6f9600b6a 100644 --- a/test/system/255-auto-update.bats +++ b/test/system/255-auto-update.bats @@ -234,6 +234,8 @@ function _confirm_update() { _confirm_update $cname $ori_image } +# This test can fail in dev. environment because of SELinux. +# quick fix: chcon -t container_runtime_exec_t ./bin/podman @test "podman auto-update - label io.containers.autoupdate=local with rollback" { # sdnotify fails with runc 1.0.0-3-dev2 on Ubuntu. Let's just # assume that we work only with crun, nothing else. From 2c999f1ecb87a6688c19502f0f594e002104e951 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 2 Aug 2022 16:18:57 +0200 Subject: [PATCH 06/17] pkg/autoupdate: update unit-by-unit Change the auto-update logic to update unit-by-unit rather by policy. This allows for, in theory now and in practice later, to have mutliple containers run in a single systemd unit and update them in sequence before restarting the unit. [NO NEW TESTS NEEDED] - should not change behavior. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 108 ++++++++++++++++++----------------- 1 file changed, 55 insertions(+), 53 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 58e9194935..0f9df93270 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -50,17 +50,14 @@ var supportedPolicies = map[string]Policy{ "local": PolicyLocalImage, } -// policyMappers assembles update tasks by policy -type policyMapper map[Policy][]*task - // updater includes shared state for auto-updating one or more containers. type updater struct { - conn *dbus.Conn - idToImage map[string]*libimage.Image - imageToPolicyMapper map[string]policyMapper - options *entities.AutoUpdateOptions - updatedRawImages map[string]bool - runtime *libpod.Runtime + conn *dbus.Conn + idToImage map[string]*libimage.Image + unitToTasks map[string][]*task + options *entities.AutoUpdateOptions + updatedRawImages map[string]bool + runtime *libpod.Runtime } // task includes data and state for updating a container @@ -152,26 +149,22 @@ func (u *updater) assembleImageMap(ctx context.Context) error { // It returns a slice of successfully restarted systemd units and a slice of // errors encountered during auto update. func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.AutoUpdateOptions) ([]*entities.AutoUpdateReport, []error) { + // Note that (most) errors are non-fatal such that a single + // misconfigured container does not prevent others from being updated + // (which could be a security threat). + auto := updater{ options: &options, runtime: runtime, updatedRawImages: make(map[string]bool), } - // Assemble a map `image ID -> *libimage.Image` that we can consult - // later on for lookups. - if err := auto.assembleImageMap(ctx); err != nil { - return nil, []error{err} - } - - // Create a map from `image ID -> []*Container`. - if errs := auto.imageContainersMap(); len(errs) > 0 { - return nil, errs - } + // Find auto-update tasks and assemble them by unit. + errors := auto.assembleTasks(ctx) // Nothing to do. - if len(auto.imageToPolicyMapper) == 0 { - return nil, nil + if len(auto.unitToTasks) == 0 { + return nil, errors } // Connect to DBUS. @@ -187,27 +180,28 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A // Update all images/container according to their auto-update policy. var allReports []*entities.AutoUpdateReport - var errs []error - for imageID, policyMapper := range auto.imageToPolicyMapper { - if _, exists := auto.idToImage[imageID]; !exists { - errs = append(errs, fmt.Errorf("container image ID %q not found in local storage", imageID)) - return nil, errs + for unit, tasks := range auto.unitToTasks { + // Sanity check: we'll support that in the future. + if len(tasks) != 1 { + errors = append(errors, fmt.Errorf("only 1 task per unit supported but unit %s has %d", unit, len(tasks))) + return nil, errors } - for _, task := range policyMapper[PolicyRegistryImage] { - report, err := auto.updateRegistry(ctx, task) - if err != nil { - errs = append(errs, err) - } - if report != nil { - allReports = append(allReports, report) - } - } + for _, task := range tasks { + var report *entities.AutoUpdateReport + var reportError error - for _, task := range policyMapper[PolicyLocalImage] { - report, err := auto.updateLocally(ctx, task) - if err != nil { - errs = append(errs, err) + switch task.policy { + case PolicyRegistryImage: + report, reportError = auto.updateRegistry(ctx, task) + case PolicyLocalImage: + report, reportError = auto.updateLocally(ctx, task) + default: + reportError = fmt.Errorf("unexpected auto-update policy %s for container %s", task.policy, task.container.ID()) + } + + if reportError != nil { + errors = append(errors, reportError) } if report != nil { allReports = append(allReports, report) @@ -215,7 +209,7 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A } } - return allReports, errs + return allReports, errors } // updateRegistry updates the image/container according to the "registry" policy. @@ -372,15 +366,21 @@ func (u *updater) restartSystemdUnit(ctx context.Context, ctr *libpod.Container, } } -// imageContainersMap generates a map[image ID] -> [containers using the image] -// of all containers with a valid auto-update policy. -func (u *updater) imageContainersMap() []error { +// assembleTasks assembles update tasks per unit and populates a mapping from +// `unit -> []*task` such that multiple containers _can_ run in a single unit. +func (u *updater) assembleTasks(ctx context.Context) []error { + // Assemble a map `image ID -> *libimage.Image` that we can consult + // later on for lookups. + if err := u.assembleImageMap(ctx); err != nil { + return []error{err} + } + allContainers, err := u.runtime.GetAllContainers() if err != nil { return []error{err} } - u.imageToPolicyMapper = make(map[string]policyMapper) + u.unitToTasks = make(map[string][]*task) errors := []error{} for _, c := range allContainers { @@ -395,6 +395,8 @@ func (u *updater) imageContainersMap() []error { continue } + // Check the container's auto-update policy which is configured + // as a label. labels := ctr.Labels() value, exists := labels[Label] if !exists { @@ -405,18 +407,19 @@ func (u *updater) imageContainersMap() []error { errors = append(errors, err) continue } - - // Skip labels not related to autoupdate if policy == PolicyDefault { continue } - id, _ := ctr.Image() - policyMap, exists := u.imageToPolicyMapper[id] + // Make sure the container runs in a systemd unit which is + // stored as a label at container creation. + unit, exists := labels[systemdDefine.EnvVariable] if !exists { - policyMap = make(map[Policy][]*task) + errors = append(errors, fmt.Errorf("auto-updating container %q: no %s label found", ctr.ID(), systemdDefine.EnvVariable)) + continue } + id, _ := ctr.Image() image, exists := u.idToImage[id] if !exists { err := fmt.Errorf("internal error: no image found for ID %s", id) @@ -424,16 +427,15 @@ func (u *updater) imageContainersMap() []error { continue } - unit, _ := labels[systemdDefine.EnvVariable] - t := task{ container: ctr, policy: policy, image: image, unit: unit, } - policyMap[policy] = append(policyMap[policy], &t) - u.imageToPolicyMapper[id] = policyMap + + // Add the task to the unit. + u.unitToTasks[unit] = append(u.unitToTasks[unit], &t) } return errors From 13a8ebd09f7d76d43f1b8af86c7ca2bbc1acfbca Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 2 Aug 2022 17:32:38 +0200 Subject: [PATCH 07/17] pkg/autoupdate: repull image if other containers failed If two containers use the same image and one rolled back (i.e., tagged the old image again), make sure to repull the image for the other container. Once an image has caused a rollback, it may be worth marking this image as broken and not update any other container using it but that is outside of the scope. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 0f9df93270..dfdbcc9569 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -54,8 +54,8 @@ var supportedPolicies = map[string]Policy{ type updater struct { conn *dbus.Conn idToImage map[string]*libimage.Image - unitToTasks map[string][]*task options *entities.AutoUpdateOptions + unitToTasks map[string][]*task updatedRawImages map[string]bool runtime *libpod.Runtime } @@ -278,6 +278,8 @@ func (u *updater) updateRegistry(ctx context.Context, task *task) (*entities.Aut if err := task.image.Tag(rawImageName); err != nil { return report, fmt.Errorf("falling back to previous image: %w", err) } + u.updatedRawImages[rawImageName] = false + if err := u.restartSystemdUnit(ctx, task.container, task.unit); err != nil { return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) } From cfa089c361c697932f0cd5506a1a43077253d6c8 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 3 Aug 2022 13:00:27 +0200 Subject: [PATCH 08/17] pkg/autoupdate: s/updateImage/pullImage/ "pull" is more expressive. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index dfdbcc9569..d01695912a 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -258,7 +258,7 @@ func (u *updater) updateRegistry(ctx context.Context, task *task) (*entities.Aut return report, nil } - if _, err := updateImage(ctx, u.runtime, rawImageName, authfile); err != nil { + if _, err := pullImage(ctx, u.runtime, rawImageName, authfile); err != nil { return report, fmt.Errorf("registry auto-updating container %q: image update for %q failed: %w", cid, rawImageName, err) } u.updatedRawImages[rawImageName] = true @@ -474,8 +474,8 @@ func newerLocalImageAvailable(runtime *libpod.Runtime, img *libimage.Image, rawI return localImg.Digest().String() != img.Digest().String(), nil } -// updateImage pulls the specified image. -func updateImage(ctx context.Context, runtime *libpod.Runtime, name, authfile string) (*libimage.Image, error) { +// pullImage pulls the specified image. +func pullImage(ctx context.Context, runtime *libpod.Runtime, name, authfile string) (*libimage.Image, error) { pullOptions := &libimage.PullOptions{} pullOptions.AuthFilePath = authfile pullOptions.Writer = os.Stderr From 3f1928d76714e6cd11a4036b2b99b897937e0586 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 3 Aug 2022 13:30:07 +0200 Subject: [PATCH 09/17] pkg/autoupdate: move more logic under `task` This will simplify the logic and pave the way for abstracting the auto-update policies to a certain degree that allows us to better control _when_ the updates and rollbacks happen and will ultimately reduce redundant code. [NO NEW TESTS NEEDED] - should not change behavior. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 95 +++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 46 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index d01695912a..e892ad3010 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -62,6 +62,7 @@ type updater struct { // task includes data and state for updating a container type task struct { + auto *updater // Reverse pointer to the updater container *libpod.Container // Container to update policy Policy // Update policy image *libimage.Image // Original image before the update @@ -188,20 +189,9 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A } for _, task := range tasks { - var report *entities.AutoUpdateReport - var reportError error - - switch task.policy { - case PolicyRegistryImage: - report, reportError = auto.updateRegistry(ctx, task) - case PolicyLocalImage: - report, reportError = auto.updateLocally(ctx, task) - default: - reportError = fmt.Errorf("unexpected auto-update policy %s for container %s", task.policy, task.container.ID()) - } - - if reportError != nil { - errors = append(errors, reportError) + report, err := task.update(ctx) + if err != nil { + errors = append(errors, err) } if report != nil { allReports = append(allReports, report) @@ -212,38 +202,50 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A return allReports, errors } +// update the task according to its auto-update policy. +func (t *task) update(ctx context.Context) (*entities.AutoUpdateReport, error) { + switch t.policy { + case PolicyRegistryImage: + return t.updateRegistry(ctx) + case PolicyLocalImage: + return t.updateLocally(ctx) + default: + return nil, fmt.Errorf("unexpected auto-update policy %s for container %s", t.policy, t.container.ID()) + } +} + // updateRegistry updates the image/container according to the "registry" policy. -func (u *updater) updateRegistry(ctx context.Context, task *task) (*entities.AutoUpdateReport, error) { - cid := task.container.ID() - rawImageName := task.container.RawImageName() +func (t *task) updateRegistry(ctx context.Context) (*entities.AutoUpdateReport, error) { + cid := t.container.ID() + rawImageName := t.container.RawImageName() if rawImageName == "" { return nil, fmt.Errorf("registry auto-updating container %q: raw-image name is empty", cid) } - if task.unit == "" { - return nil, fmt.Errorf("auto-updating container %q: no %s label found", task.container.ID(), systemdDefine.EnvVariable) + if t.unit == "" { + return nil, fmt.Errorf("auto-updating container %q: no %s label found", t.container.ID(), systemdDefine.EnvVariable) } report := &entities.AutoUpdateReport{ ContainerID: cid, - ContainerName: task.container.Name(), + ContainerName: t.container.Name(), ImageName: rawImageName, Policy: PolicyRegistryImage, - SystemdUnit: task.unit, + SystemdUnit: t.unit, Updated: "failed", } - if _, updated := u.updatedRawImages[rawImageName]; updated { + if _, updated := t.auto.updatedRawImages[rawImageName]; updated { logrus.Infof("Auto-updating container %q using registry image %q", cid, rawImageName) - if err := u.restartSystemdUnit(ctx, task.container, task.unit); err != nil { + if err := t.auto.restartSystemdUnit(ctx, t.container, t.unit); err != nil { return report, err } report.Updated = "true" return report, nil } - authfile := getAuthfilePath(task.container, u.options) - needsUpdate, err := newerRemoteImageAvailable(ctx, task.image, rawImageName, authfile) + authfile := getAuthfilePath(t.container, t.auto.options) + needsUpdate, err := newerRemoteImageAvailable(ctx, t.image, rawImageName, authfile) if err != nil { return report, fmt.Errorf("registry auto-updating container %q: image check for %q failed: %w", cid, rawImageName, err) } @@ -253,34 +255,34 @@ func (u *updater) updateRegistry(ctx context.Context, task *task) (*entities.Aut return report, nil } - if u.options.DryRun { + if t.auto.options.DryRun { report.Updated = "pending" return report, nil } - if _, err := pullImage(ctx, u.runtime, rawImageName, authfile); err != nil { + if _, err := pullImage(ctx, t.auto.runtime, rawImageName, authfile); err != nil { return report, fmt.Errorf("registry auto-updating container %q: image update for %q failed: %w", cid, rawImageName, err) } - u.updatedRawImages[rawImageName] = true + t.auto.updatedRawImages[rawImageName] = true logrus.Infof("Auto-updating container %q using registry image %q", cid, rawImageName) - updateErr := u.restartSystemdUnit(ctx, task.container, task.unit) + updateErr := t.auto.restartSystemdUnit(ctx, t.container, t.unit) if updateErr == nil { report.Updated = "true" return report, nil } - if !u.options.Rollback { + if !t.auto.options.Rollback { return report, updateErr } // To fallback, simply retag the old image and restart the service. - if err := task.image.Tag(rawImageName); err != nil { + if err := t.image.Tag(rawImageName); err != nil { return report, fmt.Errorf("falling back to previous image: %w", err) } - u.updatedRawImages[rawImageName] = false + t.auto.updatedRawImages[rawImageName] = false - if err := u.restartSystemdUnit(ctx, task.container, task.unit); err != nil { + if err := t.auto.restartSystemdUnit(ctx, t.container, t.unit); err != nil { return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) } @@ -289,27 +291,27 @@ func (u *updater) updateRegistry(ctx context.Context, task *task) (*entities.Aut } // updateRegistry updates the image/container according to the "local" policy. -func (u *updater) updateLocally(ctx context.Context, task *task) (*entities.AutoUpdateReport, error) { - cid := task.container.ID() - rawImageName := task.container.RawImageName() +func (t *task) updateLocally(ctx context.Context) (*entities.AutoUpdateReport, error) { + cid := t.container.ID() + rawImageName := t.container.RawImageName() if rawImageName == "" { return nil, fmt.Errorf("locally auto-updating container %q: raw-image name is empty", cid) } - if task.unit == "" { - return nil, fmt.Errorf("auto-updating container %q: no %s label found", task.container.ID(), systemdDefine.EnvVariable) + if t.unit == "" { + return nil, fmt.Errorf("auto-updating container %q: no %s label found", t.container.ID(), systemdDefine.EnvVariable) } report := &entities.AutoUpdateReport{ ContainerID: cid, - ContainerName: task.container.Name(), + ContainerName: t.container.Name(), ImageName: rawImageName, Policy: PolicyLocalImage, - SystemdUnit: task.unit, + SystemdUnit: t.unit, Updated: "failed", } - needsUpdate, err := newerLocalImageAvailable(u.runtime, task.image, rawImageName) + needsUpdate, err := newerLocalImageAvailable(t.auto.runtime, t.image, rawImageName) if err != nil { return report, fmt.Errorf("locally auto-updating container %q: image check for %q failed: %w", cid, rawImageName, err) } @@ -319,27 +321,27 @@ func (u *updater) updateLocally(ctx context.Context, task *task) (*entities.Auto return report, nil } - if u.options.DryRun { + if t.auto.options.DryRun { report.Updated = "pending" return report, nil } logrus.Infof("Auto-updating container %q using local image %q", cid, rawImageName) - updateErr := u.restartSystemdUnit(ctx, task.container, task.unit) + updateErr := t.auto.restartSystemdUnit(ctx, t.container, t.unit) if updateErr == nil { report.Updated = "true" return report, nil } - if !u.options.Rollback { + if !t.auto.options.Rollback { return report, updateErr } // To fallback, simply retag the old image and restart the service. - if err := task.image.Tag(rawImageName); err != nil { + if err := t.image.Tag(rawImageName); err != nil { return report, fmt.Errorf("falling back to previous image: %w", err) } - if err := u.restartSystemdUnit(ctx, task.container, task.unit); err != nil { + if err := t.auto.restartSystemdUnit(ctx, t.container, t.unit); err != nil { return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) } @@ -430,6 +432,7 @@ func (u *updater) assembleTasks(ctx context.Context) []error { } t := task{ + auto: u, container: ctr, policy: policy, image: image, From 42c4c17c01af746cd1424878a397cb6eeb9c65dd Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 3 Aug 2022 13:37:06 +0200 Subject: [PATCH 10/17] pkg/autoupdate: move authfile into `tasks` Will simplify the code and speed up things as we do not consult a container's labels multiple times. [NO NEW TESTS NEEDED] - should not change behavior. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index e892ad3010..89d0a1ac35 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -62,6 +62,7 @@ type updater struct { // task includes data and state for updating a container type task struct { + authfile string // Container-specific authfile auto *updater // Reverse pointer to the updater container *libpod.Container // Container to update policy Policy // Update policy @@ -244,8 +245,7 @@ func (t *task) updateRegistry(ctx context.Context) (*entities.AutoUpdateReport, return report, nil } - authfile := getAuthfilePath(t.container, t.auto.options) - needsUpdate, err := newerRemoteImageAvailable(ctx, t.image, rawImageName, authfile) + needsUpdate, err := newerRemoteImageAvailable(ctx, t.image, rawImageName, t.authfile) if err != nil { return report, fmt.Errorf("registry auto-updating container %q: image check for %q failed: %w", cid, rawImageName, err) } @@ -260,7 +260,7 @@ func (t *task) updateRegistry(ctx context.Context) (*entities.AutoUpdateReport, return report, nil } - if _, err := pullImage(ctx, t.auto.runtime, rawImageName, authfile); err != nil { + if _, err := pullImage(ctx, t.auto.runtime, rawImageName, t.authfile); err != nil { return report, fmt.Errorf("registry auto-updating container %q: image update for %q failed: %w", cid, rawImageName, err) } t.auto.updatedRawImages[rawImageName] = true @@ -432,6 +432,7 @@ func (u *updater) assembleTasks(ctx context.Context) []error { } t := task{ + authfile: labels[AuthfileLabel], auto: u, container: ctr, policy: policy, @@ -446,17 +447,6 @@ func (u *updater) assembleTasks(ctx context.Context) []error { return errors } -// getAuthfilePath returns an authfile path, if set. The authfile label in the -// container, if set, as precedence over the one set in the options. -func getAuthfilePath(ctr *libpod.Container, options *entities.AutoUpdateOptions) string { - labels := ctr.Labels() - authFilePath, exists := labels[AuthfileLabel] - if exists { - return authFilePath - } - return options.Authfile -} - // newerRemoteImageAvailable returns true if there corresponding image on the remote // registry is newer. func newerRemoteImageAvailable(ctx context.Context, img *libimage.Image, origName string, authfile string) (bool, error) { From 1cc933c6bb1dbbab56bc1918db4ae358c56bba8b Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 3 Aug 2022 13:49:04 +0200 Subject: [PATCH 11/17] pkg/autoupdate: introduce status constants To replace redundant string scattered across the code with proper constants. The "status" will further be useful in a future change as it can be moved into a `task`. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 89d0a1ac35..2db85df5d3 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -60,6 +60,14 @@ type updater struct { runtime *libpod.Runtime } +const ( + statusFailed = "failed" // The update has failed + statusUpdated = "true" // The update succeeded + statusNotUpdated = "false" // No update was needed + statusPending = "pending" // The update is pending (see options.DryRun) + statusRolledBack = "rolled back" // Rollback after a failed update +) + // task includes data and state for updating a container type task struct { authfile string // Container-specific authfile @@ -233,7 +241,7 @@ func (t *task) updateRegistry(ctx context.Context) (*entities.AutoUpdateReport, ImageName: rawImageName, Policy: PolicyRegistryImage, SystemdUnit: t.unit, - Updated: "failed", + Updated: statusFailed, } if _, updated := t.auto.updatedRawImages[rawImageName]; updated { @@ -241,7 +249,7 @@ func (t *task) updateRegistry(ctx context.Context) (*entities.AutoUpdateReport, if err := t.auto.restartSystemdUnit(ctx, t.container, t.unit); err != nil { return report, err } - report.Updated = "true" + report.Updated = statusUpdated return report, nil } @@ -251,12 +259,12 @@ func (t *task) updateRegistry(ctx context.Context) (*entities.AutoUpdateReport, } if !needsUpdate { - report.Updated = "false" + report.Updated = statusNotUpdated return report, nil } if t.auto.options.DryRun { - report.Updated = "pending" + report.Updated = statusPending return report, nil } @@ -268,7 +276,7 @@ func (t *task) updateRegistry(ctx context.Context) (*entities.AutoUpdateReport, logrus.Infof("Auto-updating container %q using registry image %q", cid, rawImageName) updateErr := t.auto.restartSystemdUnit(ctx, t.container, t.unit) if updateErr == nil { - report.Updated = "true" + report.Updated = statusUpdated return report, nil } @@ -286,7 +294,7 @@ func (t *task) updateRegistry(ctx context.Context) (*entities.AutoUpdateReport, return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) } - report.Updated = "rolled back" + report.Updated = statusRolledBack return report, nil } @@ -308,7 +316,7 @@ func (t *task) updateLocally(ctx context.Context) (*entities.AutoUpdateReport, e ImageName: rawImageName, Policy: PolicyLocalImage, SystemdUnit: t.unit, - Updated: "failed", + Updated: statusFailed, } needsUpdate, err := newerLocalImageAvailable(t.auto.runtime, t.image, rawImageName) @@ -317,19 +325,19 @@ func (t *task) updateLocally(ctx context.Context) (*entities.AutoUpdateReport, e } if !needsUpdate { - report.Updated = "false" + report.Updated = statusNotUpdated return report, nil } if t.auto.options.DryRun { - report.Updated = "pending" + report.Updated = statusPending return report, nil } logrus.Infof("Auto-updating container %q using local image %q", cid, rawImageName) updateErr := t.auto.restartSystemdUnit(ctx, t.container, t.unit) if updateErr == nil { - report.Updated = "true" + report.Updated = statusUpdated return report, nil } @@ -345,7 +353,7 @@ func (t *task) updateLocally(ctx context.Context) (*entities.AutoUpdateReport, e return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) } - report.Updated = "rolled back" + report.Updated = statusRolledBack return report, nil } From 82d18a86f395657424f24e86140bb1ed15229141 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 3 Aug 2022 14:52:38 +0200 Subject: [PATCH 12/17] pkg/autoupdate: use policy consts were possible Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 2db85df5d3..6748ca3516 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -43,11 +43,11 @@ const ( // Map for easy lookups of supported policies. var supportedPolicies = map[string]Policy{ - "": PolicyDefault, - "disabled": PolicyDefault, - "image": PolicyRegistryImage, - "registry": PolicyRegistryImage, - "local": PolicyLocalImage, + "": PolicyDefault, + string(PolicyDefault): PolicyDefault, + "image": PolicyRegistryImage, + string(PolicyRegistryImage): PolicyRegistryImage, + string(PolicyLocalImage): PolicyLocalImage, } // updater includes shared state for auto-updating one or more containers. From 3fdd3b1ae332a26e6bba696d8bd49d29a0299b3b Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 3 Aug 2022 14:56:01 +0200 Subject: [PATCH 13/17] pkg/autoupdate: remove image map from updater It is not state needed after assembling the tasks, so remove it to keep the task struct simpler. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 41 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 6748ca3516..794f31d1f5 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -53,7 +53,6 @@ var supportedPolicies = map[string]Policy{ // updater includes shared state for auto-updating one or more containers. type updater struct { conn *dbus.Conn - idToImage map[string]*libimage.Image options *entities.AutoUpdateOptions unitToTasks map[string][]*task updatedRawImages map[string]bool @@ -126,24 +125,6 @@ func ValidateImageReference(imageName string) error { return nil } -func (u *updater) assembleImageMap(ctx context.Context) error { - // Create a map from `image ID -> *libimage.Image` for image lookups. - listOptions := &libimage.ListImagesOptions{ - Filters: []string{"readonly=false"}, - } - imagesSlice, err := u.runtime.LibimageRuntime().ListImages(ctx, nil, listOptions) - if err != nil { - return err - } - imageMap := make(map[string]*libimage.Image) - for i := range imagesSlice { - imageMap[imagesSlice[i].ID()] = imagesSlice[i] - } - - u.idToImage = imageMap - return nil -} - // AutoUpdate looks up containers with a specified auto-update policy and acts // accordingly. // @@ -383,7 +364,8 @@ func (u *updater) restartSystemdUnit(ctx context.Context, ctr *libpod.Container, func (u *updater) assembleTasks(ctx context.Context) []error { // Assemble a map `image ID -> *libimage.Image` that we can consult // later on for lookups. - if err := u.assembleImageMap(ctx); err != nil { + imageMap, err := u.assembleImageMap(ctx) + if err != nil { return []error{err} } @@ -432,7 +414,7 @@ func (u *updater) assembleTasks(ctx context.Context) []error { } id, _ := ctr.Image() - image, exists := u.idToImage[id] + image, exists := imageMap[id] if !exists { err := fmt.Errorf("internal error: no image found for ID %s", id) errors = append(errors, err) @@ -455,6 +437,23 @@ func (u *updater) assembleTasks(ctx context.Context) []error { return errors } +// assembleImageMap creates a map from `image ID -> *libimage.Image` for image lookups. +func (u *updater) assembleImageMap(ctx context.Context) (map[string]*libimage.Image, error) { + listOptions := &libimage.ListImagesOptions{ + Filters: []string{"readonly=false"}, + } + imagesSlice, err := u.runtime.LibimageRuntime().ListImages(ctx, nil, listOptions) + if err != nil { + return nil, err + } + imageMap := make(map[string]*libimage.Image) + for i := range imagesSlice { + imageMap[imagesSlice[i].ID()] = imagesSlice[i] + } + + return imageMap, nil +} + // newerRemoteImageAvailable returns true if there corresponding image on the remote // registry is newer. func newerRemoteImageAvailable(ctx context.Context, img *libimage.Image, origName string, authfile string) (bool, error) { From af3ce70844faaeece8d7a8dedab7065a13ceba2a Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 3 Aug 2022 14:57:51 +0200 Subject: [PATCH 14/17] pkg/autoupdate: document fields of `updater` Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 794f31d1f5..69a25cb3b0 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -52,11 +52,11 @@ var supportedPolicies = map[string]Policy{ // updater includes shared state for auto-updating one or more containers. type updater struct { - conn *dbus.Conn - options *entities.AutoUpdateOptions - unitToTasks map[string][]*task - updatedRawImages map[string]bool - runtime *libpod.Runtime + conn *dbus.Conn // DBUS connection + options *entities.AutoUpdateOptions // User-specified options + unitToTasks map[string][]*task // Keeps track of tasks per unit + updatedRawImages map[string]bool // Keeps track of updated images + runtime *libpod.Runtime // The libpod runtime } const ( From 0df51bb6bccc4e3598db59ea1bf389a97d7019e4 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 3 Aug 2022 15:07:08 +0200 Subject: [PATCH 15/17] pkg/autoupdate: move status into `task` As state should be kept in a single `task`. This will allow for separating updates from rollbacks which will be needed to support multiple containers/tasks in a single unit. [NO NEW TESTS NEEDED] - should not change behavior. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 308 ++++++++++++++++------------------- 1 file changed, 140 insertions(+), 168 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 69a25cb3b0..f450a42330 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -69,12 +69,14 @@ const ( // task includes data and state for updating a container type task struct { - authfile string // Container-specific authfile - auto *updater // Reverse pointer to the updater - container *libpod.Container // Container to update - policy Policy // Update policy - image *libimage.Image // Original image before the update - unit string // Name of the systemd unit + authfile string // Container-specific authfile + auto *updater // Reverse pointer to the updater + container *libpod.Container // Container to update + policy Policy // Update policy + image *libimage.Image // Original image before the update + rawImageName string // The container's raw image name + status string // Auto-update status + unit string // Name of the systemd unit } // LookupPolicy looks up the corresponding Policy for the specified @@ -179,170 +181,155 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A } for _, task := range tasks { - report, err := task.update(ctx) + err := func() error { + updateAvailable, err := task.updateAvailable(ctx) + if err != nil { + task.status = statusFailed + return fmt.Errorf("checking image updates for container %s: %w", task.container.ID(), err) + } + + if !updateAvailable { + task.status = statusNotUpdated + return nil + } + + if options.DryRun { + task.status = statusPending + return nil + } + + if err := task.update(ctx); err != nil { + task.status = statusFailed + return fmt.Errorf("updating image for container %s: %w", task.container.ID(), err) + } + + updateError := auto.restartSystemdUnit(ctx, unit) + if updateError == nil { + task.status = statusUpdated + return nil + } + + if !options.Rollback { + task.status = statusFailed + return fmt.Errorf("restarting unit %s for container %s: %w", task.unit, task.container.ID(), err) + } + + if err := task.rollbackImage(); err != nil { + task.status = statusFailed + return fmt.Errorf("rolling back image for container %s: %w", task.container.ID(), err) + } + + if err := auto.restartSystemdUnit(ctx, unit); err != nil { + task.status = statusFailed + return fmt.Errorf("restarting unit %s for container %s during rollback: %w", task.unit, task.container.ID(), err) + } + + task.status = statusRolledBack + return nil + }() + if err != nil { errors = append(errors, err) } - if report != nil { - allReports = append(allReports, report) - } + allReports = append(allReports, task.report()) } } return allReports, errors } -// update the task according to its auto-update policy. -func (t *task) update(ctx context.Context) (*entities.AutoUpdateReport, error) { +// report creates an auto-update report for the task. +func (t *task) report() *entities.AutoUpdateReport { + return &entities.AutoUpdateReport{ + ContainerID: t.container.ID(), + ContainerName: t.container.Name(), + ImageName: t.container.RawImageName(), + Policy: string(t.policy), + SystemdUnit: t.unit, + Updated: t.status, + } +} + +// updateAvailable returns whether an update for the task is available. +func (t *task) updateAvailable(ctx context.Context) (bool, error) { switch t.policy { case PolicyRegistryImage: - return t.updateRegistry(ctx) + return t.registryUpdateAvailable(ctx) case PolicyLocalImage: - return t.updateLocally(ctx) + return t.localUpdateAvailable() default: - return nil, fmt.Errorf("unexpected auto-update policy %s for container %s", t.policy, t.container.ID()) + return false, fmt.Errorf("unexpected auto-update policy %s for container %s", t.policy, t.container.ID()) } } -// updateRegistry updates the image/container according to the "registry" policy. -func (t *task) updateRegistry(ctx context.Context) (*entities.AutoUpdateReport, error) { - cid := t.container.ID() - rawImageName := t.container.RawImageName() - if rawImageName == "" { - return nil, fmt.Errorf("registry auto-updating container %q: raw-image name is empty", cid) +// update the task according to its auto-update policy. +func (t *task) update(ctx context.Context) error { + switch t.policy { + case PolicyRegistryImage: + return t.registryUpdate(ctx) + case PolicyLocalImage: + // Nothing to do as the image is already available in the local storage. + return nil + default: + return fmt.Errorf("unexpected auto-update policy %s for container %s", t.policy, t.container.ID()) } - - if t.unit == "" { - return nil, fmt.Errorf("auto-updating container %q: no %s label found", t.container.ID(), systemdDefine.EnvVariable) - } - - report := &entities.AutoUpdateReport{ - ContainerID: cid, - ContainerName: t.container.Name(), - ImageName: rawImageName, - Policy: PolicyRegistryImage, - SystemdUnit: t.unit, - Updated: statusFailed, - } - - if _, updated := t.auto.updatedRawImages[rawImageName]; updated { - logrus.Infof("Auto-updating container %q using registry image %q", cid, rawImageName) - if err := t.auto.restartSystemdUnit(ctx, t.container, t.unit); err != nil { - return report, err - } - report.Updated = statusUpdated - return report, nil - } - - needsUpdate, err := newerRemoteImageAvailable(ctx, t.image, rawImageName, t.authfile) - if err != nil { - return report, fmt.Errorf("registry auto-updating container %q: image check for %q failed: %w", cid, rawImageName, err) - } - - if !needsUpdate { - report.Updated = statusNotUpdated - return report, nil - } - - if t.auto.options.DryRun { - report.Updated = statusPending - return report, nil - } - - if _, err := pullImage(ctx, t.auto.runtime, rawImageName, t.authfile); err != nil { - return report, fmt.Errorf("registry auto-updating container %q: image update for %q failed: %w", cid, rawImageName, err) - } - t.auto.updatedRawImages[rawImageName] = true - - logrus.Infof("Auto-updating container %q using registry image %q", cid, rawImageName) - updateErr := t.auto.restartSystemdUnit(ctx, t.container, t.unit) - if updateErr == nil { - report.Updated = statusUpdated - return report, nil - } - - if !t.auto.options.Rollback { - return report, updateErr - } - - // To fallback, simply retag the old image and restart the service. - if err := t.image.Tag(rawImageName); err != nil { - return report, fmt.Errorf("falling back to previous image: %w", err) - } - t.auto.updatedRawImages[rawImageName] = false - - if err := t.auto.restartSystemdUnit(ctx, t.container, t.unit); err != nil { - return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) - } - - report.Updated = statusRolledBack - return report, nil } -// updateRegistry updates the image/container according to the "local" policy. -func (t *task) updateLocally(ctx context.Context) (*entities.AutoUpdateReport, error) { - cid := t.container.ID() - rawImageName := t.container.RawImageName() - if rawImageName == "" { - return nil, fmt.Errorf("locally auto-updating container %q: raw-image name is empty", cid) +// registryUpdateAvailable returns whether a new image on the registry is available. +func (t *task) registryUpdateAvailable(ctx context.Context) (bool, error) { + // The newer image has already been pulled for another task, so we know + // there's a newer one available. + if _, exists := t.auto.updatedRawImages[t.rawImageName]; exists { + return true, nil } - if t.unit == "" { - return nil, fmt.Errorf("auto-updating container %q: no %s label found", t.container.ID(), systemdDefine.EnvVariable) - } - - report := &entities.AutoUpdateReport{ - ContainerID: cid, - ContainerName: t.container.Name(), - ImageName: rawImageName, - Policy: PolicyLocalImage, - SystemdUnit: t.unit, - Updated: statusFailed, - } - - needsUpdate, err := newerLocalImageAvailable(t.auto.runtime, t.image, rawImageName) + remoteRef, err := docker.ParseReference("//" + t.rawImageName) if err != nil { - return report, fmt.Errorf("locally auto-updating container %q: image check for %q failed: %w", cid, rawImageName, err) + return false, err + } + options := &libimage.HasDifferentDigestOptions{AuthFilePath: t.authfile} + return t.image.HasDifferentDigest(ctx, remoteRef, options) +} + +// registryUpdate pulls down the image from the registry. +func (t *task) registryUpdate(ctx context.Context) error { + // The newer image has already been pulled for another task. + if _, exists := t.auto.updatedRawImages[t.rawImageName]; exists { + return nil } - if !needsUpdate { - report.Updated = statusNotUpdated - return report, nil + if err := pullImage(ctx, t.auto.runtime, t.rawImageName, t.authfile); err != nil { + return fmt.Errorf("registry auto-updating container %q: image update for %q failed: %w", t.container.ID(), t.rawImageName, err) } - if t.auto.options.DryRun { - report.Updated = statusPending - return report, nil - } + t.auto.updatedRawImages[t.rawImageName] = true + return nil +} - logrus.Infof("Auto-updating container %q using local image %q", cid, rawImageName) - updateErr := t.auto.restartSystemdUnit(ctx, t.container, t.unit) - if updateErr == nil { - report.Updated = statusUpdated - return report, nil - } - - if !t.auto.options.Rollback { - return report, updateErr +// localUpdateAvailable returns whether a new image in the local storage is available. +func (t *task) localUpdateAvailable() (bool, error) { + localImg, _, err := t.auto.runtime.LibimageRuntime().LookupImage(t.rawImageName, nil) + if err != nil { + return false, err } + return localImg.Digest().String() != t.image.Digest().String(), nil +} +// rollbackImage rolls back the task's image to the previous version before the update. +func (t *task) rollbackImage() error { // To fallback, simply retag the old image and restart the service. - if err := t.image.Tag(rawImageName); err != nil { - return report, fmt.Errorf("falling back to previous image: %w", err) + if err := t.image.Tag(t.rawImageName); err != nil { + return err } - if err := t.auto.restartSystemdUnit(ctx, t.container, t.unit); err != nil { - return report, fmt.Errorf("restarting unit with old image during fallback: %w", err) - } - - report.Updated = statusRolledBack - return report, nil + t.auto.updatedRawImages[t.rawImageName] = false + return nil } // restartSystemdUnit restarts the systemd unit the container is running in. -func (u *updater) restartSystemdUnit(ctx context.Context, ctr *libpod.Container, unit string) error { +func (u *updater) restartSystemdUnit(ctx context.Context, unit string) error { restartChan := make(chan string) if _, err := u.conn.RestartUnitContext(ctx, unit, "replace", restartChan); err != nil { - return fmt.Errorf("auto-updating container %q: restarting systemd unit %q failed: %w", ctr.ID(), unit, err) + return err } // Wait for the restart to finish and actually check if it was @@ -351,11 +338,11 @@ func (u *updater) restartSystemdUnit(ctx context.Context, ctr *libpod.Container, switch result { case "done": - logrus.Infof("Successfully restarted systemd unit %q of container %q", unit, ctr.ID()) + logrus.Infof("Successfully restarted systemd unit %q", unit) return nil default: - return fmt.Errorf("auto-updating container %q: restarting systemd unit %q failed: expected %q but received %q", ctr.ID(), unit, "done", result) + return fmt.Errorf("expected %q but received %q", "done", result) } } @@ -421,13 +408,21 @@ func (u *updater) assembleTasks(ctx context.Context) []error { continue } + rawImageName := ctr.RawImageName() + if rawImageName == "" { + errors = append(errors, fmt.Errorf("locally auto-updating container %q: raw-image name is empty", ctr.ID())) + continue + } + t := task{ - authfile: labels[AuthfileLabel], - auto: u, - container: ctr, - policy: policy, - image: image, - unit: unit, + authfile: labels[AuthfileLabel], + auto: u, + container: ctr, + policy: policy, + image: image, + unit: unit, + rawImageName: rawImageName, + status: statusFailed, // must be updated later on } // Add the task to the unit. @@ -454,35 +449,12 @@ func (u *updater) assembleImageMap(ctx context.Context) (map[string]*libimage.Im return imageMap, nil } -// newerRemoteImageAvailable returns true if there corresponding image on the remote -// registry is newer. -func newerRemoteImageAvailable(ctx context.Context, img *libimage.Image, origName string, authfile string) (bool, error) { - remoteRef, err := docker.ParseReference("//" + origName) - if err != nil { - return false, err - } - options := &libimage.HasDifferentDigestOptions{AuthFilePath: authfile} - return img.HasDifferentDigest(ctx, remoteRef, options) -} - -// newerLocalImageAvailable returns true if the container and local image have different digests -func newerLocalImageAvailable(runtime *libpod.Runtime, img *libimage.Image, rawImageName string) (bool, error) { - localImg, _, err := runtime.LibimageRuntime().LookupImage(rawImageName, nil) - if err != nil { - return false, err - } - return localImg.Digest().String() != img.Digest().String(), nil -} - // pullImage pulls the specified image. -func pullImage(ctx context.Context, runtime *libpod.Runtime, name, authfile string) (*libimage.Image, error) { +func pullImage(ctx context.Context, runtime *libpod.Runtime, name, authfile string) error { pullOptions := &libimage.PullOptions{} pullOptions.AuthFilePath = authfile pullOptions.Writer = os.Stderr - pulledImages, err := runtime.LibimageRuntime().Pull(ctx, name, config.PullPolicyAlways, pullOptions) - if err != nil { - return nil, err - } - return pulledImages[0], nil + _, err := runtime.LibimageRuntime().Pull(ctx, name, config.PullPolicyAlways, pullOptions) + return err } From 43cca5d97a2c971f3386099d6b66250a1ebad96f Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 3 Aug 2022 16:12:48 +0200 Subject: [PATCH 16/17] pkg/autoupdate: decompose the update logic Decompose the update logic into smaller steps (update check, update, rollback, etc.) and move the implementation into the `task` API. This allows to transition a task from state to state, independent of its underlying auto-update policy. Supporting more than one container per unit is now really close. [NO NEW TESTS NEEDED] - should not change behavior. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index f450a42330..b3f0c53eb7 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -182,6 +182,10 @@ func AutoUpdate(ctx context.Context, runtime *libpod.Runtime, options entities.A for _, task := range tasks { err := func() error { + // Transition from state to state. Will be + // split into multiple loops in the future to + // support more than one container/task per + // unit. updateAvailable, err := task.updateAvailable(ctx) if err != nil { task.status = statusFailed @@ -298,8 +302,11 @@ func (t *task) registryUpdate(ctx context.Context) error { return nil } - if err := pullImage(ctx, t.auto.runtime, t.rawImageName, t.authfile); err != nil { - return fmt.Errorf("registry auto-updating container %q: image update for %q failed: %w", t.container.ID(), t.rawImageName, err) + pullOptions := &libimage.PullOptions{} + pullOptions.AuthFilePath = t.authfile + pullOptions.Writer = os.Stderr + if _, err := t.auto.runtime.LibimageRuntime().Pull(ctx, t.rawImageName, config.PullPolicyAlways, pullOptions); err != nil { + return err } t.auto.updatedRawImages[t.rawImageName] = true @@ -448,13 +455,3 @@ func (u *updater) assembleImageMap(ctx context.Context) (map[string]*libimage.Im return imageMap, nil } - -// pullImage pulls the specified image. -func pullImage(ctx context.Context, runtime *libpod.Runtime, name, authfile string) error { - pullOptions := &libimage.PullOptions{} - pullOptions.AuthFilePath = authfile - pullOptions.Writer = os.Stderr - - _, err := runtime.LibimageRuntime().Pull(ctx, name, config.PullPolicyAlways, pullOptions) - return err -} From 81a1ea18c8426134b9985469fc02092078912cfe Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 4 Aug 2022 08:22:37 +0200 Subject: [PATCH 17/17] pkg/autoupdate: "image" policy: add deprecation comment The "image" policy has been deprecated in favor of the more precise "registry" policy. Add a code comment to leave some breadcrumbs for future generations. Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index b3f0c53eb7..297d6640e1 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -45,7 +45,7 @@ const ( var supportedPolicies = map[string]Policy{ "": PolicyDefault, string(PolicyDefault): PolicyDefault, - "image": PolicyRegistryImage, + "image": PolicyRegistryImage, // Deprecated in favor of PolicyRegistryImage string(PolicyRegistryImage): PolicyRegistryImage, string(PolicyLocalImage): PolicyLocalImage, }