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

Infra Container Enhancement #10933

Closed
cdoern opened this issue Jul 14, 2021 · 5 comments · Fixed by #11102
Closed

Infra Container Enhancement #10933

cdoern opened this issue Jul 14, 2021 · 5 comments · Fixed by #11102
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@cdoern
Copy link
Contributor

cdoern commented Jul 14, 2021

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind feature

Description

InfraContainerConfig is not as parallel as it should be to ContainerConfig and therefore container creation seems to be split into two different paths. There should be clear differences as certain attributes do not apply. However, for simplicity and readability I believe we should begin a slight rework of what InfraContainerConfig looks like.

The codepath for a container is as follows

cmd => filloutspecgen => containercreate => completespec => makecontainer => newContainer()

while infra is...

cmd => podcreate => makepod => createpodoptions => newPod => makeinfracontainer => createinfracontainer => newContainer()

They end up at the same place and require most of the same options therefore should be combined into one codepath further up in the list.

Ran into this while working on pod create flags. Certain things that can be done while creating a regular container cannot be done while creating infra due to a lot of missing attributes. This is a good foundational step before building up podman pod create. A good specific example is while implementing the --volume flag, I could not use overlayVolumes because overlay.Mount() requires the usage of the containers StaticDir which is where persistent libpod data is stored. Infra does not have this. This change led me down a rabbit hole of other changes that could be made which leads me to think a general rework is called for.

Will link commits I make to my infraEnhance branch so others can see what I am suggesting.

Open for suggestions and discussions!

@cdoern cdoern self-assigned this Jul 14, 2021
@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 14, 2021
@mheon
Copy link
Member

mheon commented Jul 15, 2021

The StaticDir isn't something we know before the container is created - it's configured by c/storage as part of container creation?

@cdoern
Copy link
Contributor Author

cdoern commented Jul 15, 2021

hmmm okay @mheon I guess the issue is there is not equiv place as there is w/ regular containers to do specific mount such as overlay which uses the StaticDir

@rhatdan
Copy link
Member

rhatdan commented Jul 15, 2021

Doesn't the infra container get a staticdir?

@cdoern
Copy link
Contributor Author

cdoern commented Jul 15, 2021

Yes @rhatdan it does but not it cannot be explicitly set until infra goes through the normal container creation process. I am brainstorming on how to consolidate infra and regular container creation since infra ends up getting mostly written by regular container creation anyway. I feel like they should be more comparable and easy to track. Gonna look into this more later today and tomorrow.

cdoern added a commit to cdoern/podman that referenced this issue Aug 10, 2021
InfraContainer should go through the same creation process as regular containers. This change was from the cmd level
down, involving new container CLI opts and specgen creating functions. What now happens is that both container and pod
cli options are populated in cmd and used to create a podSpecgen and a containerSpecgen. The process then goes as follows

FillOutSpecGen (infra) -> MapSpec (podOpts -> infraOpts) -> PodCreate -> MakePod -> createPodOptions -> NewPod -> CompleteSpec (infra) -> MakeContainer -> NewContainer -> newContainer -> AddInfra (to pod state)

removed runtime_pod_infra_linux.go, infra now is on its way to following basic container creation path.

resolves containers#10933

Signed-off-by: cdoern <cdoern@redhat.com>
@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@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 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 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. 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.

3 participants