issue-20372 - Initial impl for restartable init-container

[vincentywdeng]

Implementing native sidecar as kubernetes
https://kubernetes.io/blog/2023/08/25/native-sidecar-containers/

Does this PR introduce a user-facing change?

[openshift-ci]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rhatdan]

Please sign your commits.

git commit -a --amend -s
git push --force

[ygalblum]

The code looks good. Please make sure to add test(s)

[packit-as-a-service]

Cockpit tests failed for commit 5fb6d9f8a862ec72232ec33793b3696f03d2009b. @martinpitt, @jelly, @mvollmer please check.

[ygalblum]

In addition, there is no test for the actual restarting of an init container

[martinpitt]

The cockpit test failure detected a crun regression in podman-next, see containers/crun#1367 Unrelated to this PR.

[vincentywdeng]

Thanks @ygalblum for the comment, I had updated my test.
On the other hand, for actual restarting of an init container, do you mean when sidecar container failed, it should be automatic restarted by Pod?
I haven't implement that yet as I don't know how to do it yet. Could I get some hint from expert for how to do that?

[ygalblum]

Thanks @ygalblum for the comment, I had updated my test.

Great, thanks

On the other hand, for actual restarting of an init container, do you mean when sidecar container failed, it should be automatic restarted by Pod? I haven't implement that yet as I don't know how to do it yet. Could I get some hint from expert for how to do that?

Yes, this is what I meant. I thought that once the container pass the check, podman will monitor and restart them. Have you tried it our manually?

[vincentywdeng]

@ygalblum
In my manual test, the restartable init container won't get restarted when exit. I tried

1. podman kill
2. Defining this command for init container, and then delete /shared/test to make it fail

      command:
        - sh
        - -c  
        - "while [ -e '/shared/test' ]; do echo 'File exists!'; sleep 5; done && exit 1"  

I assume podman didn't support it because podman won't continue until old init container exit. There is not such requirement to restart old init container.

[vincentywdeng]

Hi @ygalblum and @rhatdan
As mentioned above, it seems that podman currently didn't support re-starting init container when it fail and the requirement is not in the original description of the issue. Is it reasonable to create another enhancement for it and have current implementations merged?

[ygalblum]

I fear that merging this code without the code to restart the init-container might mislead the users as they will expect that the init-container is restarted while in fact in won't.
Having said that, does Podman restart a init-container when the policy is on-failure? If not, then I think we can merge this PR while making sure to document that init-containers are not restarted at all. But, if yes, then we're back to my initial response.
@rhatdan WDYT?

[rhatdan]

No idea on the second question about the init container.

[vincentywdeng]

According to https://github.com/containers/podman/blame/7dc7cbfd9b0440ddc86e210a2272fdaccd6376bb/pkg/domain/infra/abi/play.go#L803, the original init-container can not be restarted. So maybe we can just merge this PR with documentation?

Regarding to restarting, do you know how podman restart regular container. I didn't see how it is done in Podman code

[rhatdan]

Conmon restarts the container.

[ygalblum]

According to https://github.com/containers/podman/blame/7dc7cbfd9b0440ddc86e210a2272fdaccd6376bb/pkg/domain/infra/abi/play.go#L803, the original init-container can not be restarted. So maybe we can just merge this PR with documentation?

Fine by me. @containers/podman-maintainers Are you OK with this approach?

[rhatdan]

Makes sense from my understanding of a pod, that an init container only happens once during creation of the pod.

[ygalblum]

Makes sense from my understanding of a pod, that an init container only happens once during creation of the pod.

K8S is kinda piggybacking on init-containers to define side-car containers that must start successfully before the main container however unlike the conventional init-containers these ones are expected to keep running as long as the pod is running and therefore have a restartPolicy.
Without this change, Podman returns an error when trying to start a pod with an init-container that has a restartPolicy of always. This change allows Podman to consume such pods. However, it does not handle the actual restarting according to the policy.

The question was whether we should merge this change alone. The problem is that with this change, Podman ignores a configuration it accepts. However, from https://github.com/containers/podman/blame/7dc7cbfd9b0440ddc86e210a2272fdaccd6376bb/pkg/domain/infra/abi/play.go#L803 we see that even today Podman does not restart the init-container if the restartPolicy is on-failure.

So, I think we it's OK to merge this change with the current functionality. But, make sure the documentation is clear about it.

[vincentywdeng]

Thanks, I updated docs/source/markdown/podman-kube-play.1.md.in for the restartable init container.

[rhatdan]

I am fine with merging, since we are already broken, and this makes us less broken. But we should fix the handling of init containers with restart policy in a separate PR.

[ygalblum]

The LGTM. @vincentywdeng do you mind squashing the commits?

[vincentywdeng]

Thanks, I squashed my commits. How can I proceed with the the PR? There are some failed tests which I belive is unrelated to my change.

[ygalblum]

Thanks, I squashed my commits. How can I proceed with the the PR? There are some failed tests which I belive is unrelated to my change.

@vincentywdeng It seems that there is a failing check on the commit subject length: FAIL - commit subject exceeds 90 characters

[vincentywdeng]

Thanks, I updated the commit title. The rest failures are all

[+1957s] Summarizing 2 Failures:
[+1957s]   [FAIL] Podman search [It] podman search image with --compatible
[+1957s]   /var/tmp/go/src/github.com/containers/podman/test/e2e/search_test.go:74
[+1957s]   [FAIL] Podman search [It] podman search image with description
[+1957s]   /var/tmp/go/src/github.com/containers/podman/test/e2e/search_test.go:65

[ygalblum]

The code LGTM and it seems like the test failures are unrelated.

[vincentywdeng]

@rhatdan and @ygalblum
Should I merge with latest master to see if it fixes the failed test?
If yes, is the practice to merge or rebase?

[ygalblum]

Should I merge with latest master to see if it fixes the failed test?
If yes, is the practice to merge or rebase?

Yes. Just rebase on top of the current main

[ygalblum]

Sorry for the last minute nit. But, I think it can be beneficial

[ygalblum]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincentywdeng, ygalblum

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 [ygalblum]

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

[rhatdan]

@ashley-cui @umohnani8 PTAL

[ygalblum]

@containers/podman-maintainers PTAL. This will probably affect the discussion #22496

[rhatdan]

Lets bring this back alive.

@baude @Luap99 WDYT?