From 47e0557d57dfe65552954339cf5a55eca1f1b5b1 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 19 Jun 2023 08:52:55 +0200 Subject: [PATCH] auto update: restart instead of stop+start Commit f131eaa74aa6 changed restart to a stop+start motivated by comments in the systemd man pages that restart behaves different than stop+start, for instance, that it keeps certain resources open and treats timers differently. Yet, the actually fix for #17607 in the very same commit was dealing with an ENOENT of the CID file on container removal. As it turns out in in #18926, changing to stop+start regressed on restarting dependencies when auto updating a systemd unit. Hence, move back to using restart to make sure that dependent systemd units are restarted as well. An alternative could be recommending to use `BindsTo=` in Quadlet files but this seems less common than `Requires=` and hence more risky to cause issues on user sites. Fixes: #18926 Signed-off-by: Valentin Rothberg --- pkg/autoupdate/autoupdate.go | 33 ++----------------------------- test/system/255-auto-update.bats | 34 +++++++++++++++++++++++++++----- 2 files changed, 31 insertions(+), 36 deletions(-) diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go index 66752f193d..4f2a4a0770 100644 --- a/pkg/autoupdate/autoupdate.go +++ b/pkg/autoupdate/autoupdate.go @@ -325,16 +325,8 @@ func (t *task) rollbackImage() error { // restartSystemdUnit restarts the systemd unit the container is running in. func (u *updater) restartSystemdUnit(ctx context.Context, unit string) error { - if err := u.stopSystemdUnit(ctx, unit); err != nil { - return err - } - return u.startSystemdUnit(ctx, unit) -} - -// startSystemdUnit starts the systemd unit the container is running in. -func (u *updater) startSystemdUnit(ctx context.Context, unit string) error { restartChan := make(chan string) - if _, err := u.conn.StartUnitContext(ctx, unit, "replace", restartChan); err != nil { + if _, err := u.conn.RestartUnitContext(ctx, unit, "replace", restartChan); err != nil { return err } @@ -348,28 +340,7 @@ func (u *updater) startSystemdUnit(ctx context.Context, unit string) error { return nil default: - return fmt.Errorf("error starting systemd unit %q expected %q but received %q", unit, "done", result) - } -} - -// stopSystemdUnit stop the systemd unit the container is running in. -func (u *updater) stopSystemdUnit(ctx context.Context, unit string) error { - restartChan := make(chan string) - if _, err := u.conn.StopUnitContext(ctx, unit, "replace", restartChan); err != nil { - return err - } - - // Wait for the restart to finish and actually check if it was - // successful or not. - result := <-restartChan - - switch result { - case "done": - logrus.Infof("Successfully stopped systemd unit %q", unit) - return nil - - default: - return fmt.Errorf("error stopping systemd unit %q expected %q but received %q", unit, "done", result) + return fmt.Errorf("error restarting systemd unit %q expected %q but received %q", unit, "done", result) } } diff --git a/test/system/255-auto-update.bats b/test/system/255-auto-update.bats index b9ba7f2ca5..08dfa03243 100644 --- a/test/system/255-auto-update.bats +++ b/test/system/255-auto-update.bats @@ -53,6 +53,7 @@ function generate_service() { local command=$3 local extraArgs=$4 local noTag=$5 + local requires=$6 # Unless specified, set a default command. if [[ -z "$command" ]]; then @@ -73,9 +74,14 @@ function generate_service() { else label="" fi + + if [[ -n "$requires" ]]; then + requires="--requires=$requires" + fi + run_podman create $extraArgs --name $cname $label $target_img $command - (cd $UNIT_DIR; run_podman generate systemd --new --files --name $cname) + (cd $UNIT_DIR; run_podman generate systemd --new --files --name $requires $cname) echo "container-$cname" >> $SNAME_FILE run_podman rm -t 0 -f $cname @@ -161,23 +167,41 @@ function _confirm_update() { run_podman events --filter type=system --since $since --stream=false is "$output" "" + # Generate two units. The first "parent" to be auto updated, the second + # "child" depends on/requires the "parent" and is expected to get restarted + # as well on auto updates (regression test for #18926). generate_service alpine image + ctr_parent=$cname + _wait_service_ready container-$ctr_parent.service + + generate_service alpine image "" "" "" "container-$ctr_parent.service" + ctr_child=$cname + _wait_service_ready container-$ctr_child.service + run_podman container inspect --format "{{.ID}}" $ctr_child + old_child_id=$output - _wait_service_ready container-$cname.service since=$(date --iso-8601=seconds) run_podman auto-update --dry-run --format "{{.Unit}},{{.Image}},{{.Updated}},{{.Policy}}" - is "$output" ".*container-$cname.service,quay.io/libpod/alpine:latest,pending,registry.*" "Image update is pending." + is "$output" ".*container-$ctr_parent.service,quay.io/libpod/alpine:latest,pending,registry.*" "Image update is pending." run_podman events --filter type=system --since $since --stream=false is "$output" ".* system auto-update" since=$(date --iso-8601=seconds) run_podman auto-update --rollback=false --format "{{.Unit}},{{.Image}},{{.Updated}},{{.Policy}}" is "$output" "Trying to pull.*" "Image is updated." - is "$output" ".*container-$cname.service,quay.io/libpod/alpine:latest,true,registry.*" "Image is updated." + is "$output" ".*container-$ctr_parent.service,quay.io/libpod/alpine:latest,true,registry.*" "Image is updated." run_podman events --filter type=system --since $since --stream=false is "$output" ".* system auto-update" - _confirm_update $cname $ori_image + # Confirm that the update was successful and that the child container/unit + # has been restarted as well. + _confirm_update $ctr_parent $ori_image + run_podman container inspect --format "{{.ID}}" $ctr_child + assert "$output" != "$old_child_id" \ + "child container/unit has not been restarted during update" + run_podman container inspect --format "{{.ID}}" $ctr_child + run_podman container inspect --format "{{.State.Status}}" $ctr_child + is "$output" "running" "child container is in running state" } @test "podman auto-update - label io.containers.autoupdate=image with rollback" {