From 04e8cd1eb101a135d976f799040aa51c136504cc Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 4 Mar 2025 18:20:04 +0100 Subject: [PATCH 1/3] kube play: don't print start errors twice It is very bad practise to print to stdout in our backend code without nay real context. The exact same error message is returned to the caller and printed in the cli frontend hwere it should be. Therefore drop this print as it is redundant. Signed-off-by: Paul Holzinger --- pkg/domain/infra/abi/play.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/domain/infra/abi/play.go b/pkg/domain/infra/abi/play.go index 2d9b7807167..4876974a1ef 100644 --- a/pkg/domain/infra/abi/play.go +++ b/pkg/domain/infra/abi/play.go @@ -1143,7 +1143,6 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY } for id, err := range podStartErrors { playKubePod.ContainerErrors = append(playKubePod.ContainerErrors, fmt.Errorf("starting container %s: %w", id, err).Error()) - fmt.Println(playKubePod.ContainerErrors) } // Wait for each proxy to receive a READY message. Use a wait From 518773a616ea6e29b3f7481564bb69d8ec63268c Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 5 Mar 2025 14:47:26 +0100 Subject: [PATCH 2/3] pkg/domain/infra/abi/play.go: fix two nilness issues The first condition is checking an error where no error is returned and the second is checking even though err == nil was matched above already so we know the error is not nil here. Then also replace os.IsNotExist(err) with errors.Is(err, os.ErrNotExist) as that should be used for new code. This should not change behavior in any way. Signed-off-by: Paul Holzinger --- pkg/domain/infra/abi/play.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg/domain/infra/abi/play.go b/pkg/domain/infra/abi/play.go index 4876974a1ef..7aa636792b2 100644 --- a/pkg/domain/infra/abi/play.go +++ b/pkg/domain/infra/abi/play.go @@ -715,9 +715,6 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY } p := specgen.NewPodSpecGenerator() - if err != nil { - return nil, nil, err - } p, err = entities.ToPodSpecGen(*p, &podOpt) if err != nil { @@ -1632,7 +1629,7 @@ func getBuildFile(imageName string, cwd string) (string, error) { // If the error is not because the file does not exist, take // a mulligan and try Dockerfile. If that also fails, return that // error - if err != nil && !os.IsNotExist(err) { + if !errors.Is(err, os.ErrNotExist) { logrus.Error(err.Error()) } @@ -1642,7 +1639,7 @@ func getBuildFile(imageName string, cwd string) (string, error) { return dockerfilePath, nil } // Strike two - if os.IsNotExist(err) { + if errors.Is(err, os.ErrNotExist) { return "", nil } return "", err From 945aade38b3d323803270276413dabd717d5cf89 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 5 Mar 2025 14:24:40 +0100 Subject: [PATCH 3/3] quadlet kube: correctly mark unit as failed When no containers could be started we need to make sure the unit status reflects this. This means we should not send the READ=1 message and not keep the service container running when we were unable to start any container. There is the question what should happen when only a subset was started. For systemd we can only be either running or failed. And as podman kube play also just keeps the partial started pods running I opted to let systemd keep considering this as success. Fixes #20667 Fixes https://issues.redhat.com/browse/RHEL-80471 Signed-off-by: Paul Holzinger --- pkg/domain/infra/abi/play.go | 33 ++++++++++++++++++---- test/system/252-quadlet.bats | 53 ++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/pkg/domain/infra/abi/play.go b/pkg/domain/infra/abi/play.go index 7aa636792b2..6ffbf4cf54e 100644 --- a/pkg/domain/infra/abi/play.go +++ b/pkg/domain/infra/abi/play.go @@ -283,6 +283,19 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, options var configMaps []v1.ConfigMap ranContainers := false + // set the ranContainers bool to true if at least one container was successfully started. + setRanContainers := func(r *entities.PlayKubeReport) { + if !ranContainers { + for _, p := range r.Pods { + // If the list of container errors is less then the total number of pod containers then we know it didn't start. + if len(p.ContainerErrors) < len(p.Containers)+len(p.InitContainers) { + ranContainers = true + break + } + } + } + } + // FIXME: both, the service container and the proxies, should ideally // be _state_ of an object. The Kube code below is quite Spaghetti-code // which we should refactor at some point to make it easier to extend @@ -364,7 +377,7 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, options report.Pods = append(report.Pods, r.Pods...) validKinds++ - ranContainers = true + setRanContainers(r) case "DaemonSet": var daemonSetYAML v1apps.DaemonSet @@ -380,7 +393,7 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, options report.Pods = append(report.Pods, r.Pods...) validKinds++ - ranContainers = true + setRanContainers(r) case "Deployment": var deploymentYAML v1apps.Deployment @@ -396,7 +409,7 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, options report.Pods = append(report.Pods, r.Pods...) validKinds++ - ranContainers = true + setRanContainers(r) case "Job": var jobYAML v1.Job @@ -412,7 +425,7 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, options report.Pods = append(report.Pods, r.Pods...) validKinds++ - ranContainers = true + setRanContainers(r) case "PersistentVolumeClaim": var pvcYAML v1.PersistentVolumeClaim @@ -473,10 +486,14 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, options return nil, fmt.Errorf("YAML document does not contain any supported kube kind") } + if !options.ServiceContainer { + return report, nil + } + // If we started containers along with a service container, we are // running inside a systemd unit and need to set the main PID. - if options.ServiceContainer && ranContainers { + if ranContainers { switch len(notifyProxies) { case 0: // Optimization for containers/podman/issues/17345 // No container needs sdnotify, so we can mark the @@ -509,6 +526,12 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, options } report.ServiceContainerID = serviceContainer.ID() + } else if serviceContainer != nil { + // No containers started, make sure to stop the service container. + // Note because the pods still do exists and are not removed by default we cannot remove it. + if err := serviceContainer.StopWithTimeout(0); err != nil { + logrus.Errorf("Failed to stop service container: %v", err) + } } return report, nil diff --git a/test/system/252-quadlet.bats b/test/system/252-quadlet.bats index 264518b2695..df184333b77 100644 --- a/test/system/252-quadlet.bats +++ b/test/system/252-quadlet.bats @@ -1107,6 +1107,59 @@ EOF service_cleanup $QUADLET_SERVICE_NAME inactive } +# https://github.com/containers/podman/issues/20667 +@test "quadlet kube - start error" { + local port=$(random_free_port) + # Create the YAMl file + pod_name="p-$(safename)" + container_name="c-$(safename)" + yaml_source="$PODMAN_TMPDIR/start_err$(safename).yaml" + cat >$yaml_source < $quadlet_file <