stop podman kube play --wait from using 100% CPU

[t4chib4ne]

When running podman kube play --wait pod.yaml the podman process invoked on the commandline will use 100% of a single CPU waiting for the pod to exit.
This can be reproduced with the following file and then opening another terminal and observing the CPU load of the process.

# https://kubernetes.io/docs/concepts/workloads/pods/
apiVersion: v1
kind: Pod
metadata:
  name: "high-cpu-load"
  namespace: default
  labels:
    app: "high-cpu-load"
spec:
  containers:
  - name: high-cpu-load
    image: docker.io/library/debian:bookworm-slim
    command: ["bash"]
    args: ["-c", "handler() { exit 0; } ; trap handler TERM; sleep infinity& wait"]
  restartPolicy: Never

This is fixed by adding a proper Interval value to the options given to ContainerWait. Previously the default value of time.Duration was used which is 0 thus creating a busy-wait.

I have set it to 250ms similarly how it was done here #18149
Likewise I don't know if there are additional tests needed for this PR.

Does this PR introduce a user-facing change?

podman kube play --wait will no longer busy poll which resulted excessive cpu usage.

[Luap99]

I feel like in order to prevent more of such bugs it would be much better to set a default pollInterval in func (c *Container) WaitForExit(), i.e.

if pollInterval <= 0 {
	pollInterval = DefaultWaitInterval
}

[t4chib4ne]

Yes, I agree.
As a side note: I checked other functions calling Container.WaitForExit and it looks like (no 100% sure) there are no more bugs like this one except for the one from this PR. Other functions supply their own default value of 250ms.

I would like to add your suggestion by reverting my commit, adding the code and adding a comment to Container.WaitForExit pointing out the default value. Is that OK? Do I need to add tests or are the current ones sufficient?
(Sorry for asking but I am pretty new to doing PRs)

[Luap99]

I would like to add your suggestion by reverting my commit, adding the code and adding a comment to Container.WaitForExit pointing out the default value. Is that OK? Do I need to add tests or are the current ones sufficient?

Yes you can drop your commit and then add a new one with the default logic. Reverting (in terms of git revert command) makes no sense in a PR context because that adds a second commit which undo the changes so they still end up being in the git history.

I mark this as no tests needed, given the current functionality is already in some tests and pass it will be pretty hard to add a solid automatic test into our CI system. We would need to track CPU usage of the process which will be unreliable in CI systems and not something we have done before in CI tests AFAIK.

[rhatdan]

/approve
LGTM
Thanks @t4chib4ne

[Luap99]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, rhatdan, t4chib4ne

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 [Luap99,rhatdan]

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