Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to specify init container type when using play kube #14877

Closed
fugkco opened this issue Jul 8, 2022 · 10 comments · Fixed by #14977
Closed

Allow to specify init container type when using play kube #14877

fugkco opened this issue Jul 8, 2022 · 10 comments · Fixed by #14977
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. kube locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@fugkco
Copy link

fugkco commented Jul 8, 2022

/kind feature

Description

It seems init container type (podman create ... --init-ctr=type) cannot be specified in the kubernetes manifest when using play kube (see https://github.com/containers/podman/blob/main/pkg/domain/infra/abi/play.go#L516).

This also doesn't match Kubernetes' default initContainers behaviour, which is closer to setting the type to once. In addition, when an init container fails, it repeatedly restarts the init container until it passes. The exception to this is when the pod's restartPolicy is set to Never, in which case on failure, the overall pod is failed.

It would be nice to either change the behaviour of this to be closer to Kubernetes, or allow for this to be changed so an init container can be set to run once within a manifest.

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 8, 2022
@vrothberg
Copy link
Member

Thanks for reaching out, @fugkco!

@baude @rhatdan something to groom

@rhatdan
Copy link
Member

rhatdan commented Jul 11, 2022

I think it should follow the kubernetes behaviour.
@umohnani8 WDYT?

@rhatdan rhatdan added the kube label Jul 11, 2022
@umohnani8
Copy link
Member

I agree, we should follow k8s behavior for type and restarts. We can add a flag --init-ctr to the play kube command so users can specify the type. If none is specified, we can default to once.

I can work on this.

@umohnani8 umohnani8 self-assigned this Jul 11, 2022
@vrothberg
Copy link
Member

@umohnani8 can you elaborate on why we need a flag? If specified in the YAML, I'd expect play kube to just use them as K8s does.

@fugkco
Copy link
Author

fugkco commented Jul 12, 2022

Agreed. I think if anything this option should be managable on a per init container basis, not globally. But I don't see an obvious way of doing this based on K8s Container definition. Perhaps a label on the pod spec but that seems like a lot.

@rhatdan
Copy link
Member

rhatdan commented Jul 12, 2022

I agree no reason for a flag, we should work like Kubernetes does with init containers.

@umohnani8
Copy link
Member

Kuberentes doesn't have a field to set the type of init container in the yaml, there is no once or always in kubernetes land. We can change the default to be once for play kube if that is what kubernetes does.

If we want to give the user the choice to decided if they want the init containers to be always or once, that would be a flag in play kube or we parse a label added to the podspec (this label will be only specific to podman). In both cases, the type will be set for all the init containers in the yaml, I don't think it makes sense to set it per init container - the type should be conisistent throughout all init containers imo.

@vrothberg
Copy link
Member

We can change the default to be once for play kube if that is what kubernetes does.

That sounds like a good first step.

If we want to give the user the choice to decided if they want the init containers to be always or once, that would be a flag in play kube or we parse a label added to the podspec (this label will be only specific to podman). In both cases, the type will be set for all the init containers in the yaml, I don't think it makes sense to set it per init container - the type should be conisistent throughout all init containers imo.

A label/annotation sounds good. We do that for a number of Podman-specific features already and I see beauty in all these things being part of the YAML such that it works everywhere the same way.

@Syquel
Copy link
Contributor

Syquel commented Dec 24, 2022

In my opinion the init container type "always" was the correct default.

Excerpt from the Kubernetes documentation:

If the Pod restarts, or is restarted, all init containers must execute again.

Excerpt from the podman-create documentation:

The always value means the container will run with each and every pod start, whereas the once value means the container will only run once when the pod is started and then the container is removed.

Although "always" does not replicate the Kubernetes init container behavior 100% because Podman init containers are not run when the pod is restarted, in my opinion it is more correct because the init container should run when a stopped pod is started.

@fugkco
Copy link
Author

fugkco commented Jan 2, 2023

@Syquel I believe the issue that was fixed is that the initContainer kept restarting even when it ran successfully (without the pod restarting that is), which is not the correct behaviour compared to Kubernetes. If the issue is that restarting the pod doesn't rerun the initContainer, that's a different issue, and you should raise that as a separate issue for it to be triaged.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Categorizes issue or PR as related to a new feature. kube locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants