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

Podman support. #3021

Merged
merged 3 commits into from
Jun 27, 2023
Merged

Podman support. #3021

merged 3 commits into from
Jun 27, 2023

Conversation

Creatone
Copy link
Collaborator

@Creatone Creatone commented Dec 1, 2021

Fixes #2424

Signed-off-by: Paweł Szulik pawel.szulik@intel.com

container/docker/handler.go Outdated Show resolved Hide resolved
container/podman/podman.go Outdated Show resolved Hide resolved
Copy link

@strideynet strideynet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey ! This PR is looking good and has some functionality we've been needing in a recent project. How's this going ? Still being worked on ?

container/podman/plugin.go Show resolved Hide resolved
@Creatone
Copy link
Collaborator Author

Creatone commented Jan 5, 2022

@strideynet I'm on holiday now, when I get back I'll try to end it :D

@rhatdan
Copy link

rhatdan commented Jan 25, 2022

Any udpate on this PR?

@Creatone
Copy link
Collaborator Author

Any udpate on this PR?

Still a work in progress, so many things simultaneously to do. I think that half work is done. Soon I will end this PR :)

@Creatone Creatone force-pushed the creatone/podman branch 15 times, most recently from 1beb52a to 82ae52c Compare January 31, 2022 20:26
@Crapshit
Copy link

@dims @bobbypage @iwankgb @mrunalp could you please move this forward? Thank you in advance

@dims
Copy link
Collaborator

dims commented May 16, 2023

@Crapshit as i am not a google employee, i consider myself as a guest here. I do not want to merge a feature without explicit ok from google folks.

@Crapshit
Copy link

Ok, understand @dims.
I thought you would merge it, because you are member of the core team

@dims
Copy link
Collaborator

dims commented May 16, 2023

@Crapshit thanks! (my mandate has been to make sure k8s usage of cadvisor is covered to be a bit more specific)

@Crapshit
Copy link

@Creatone do you think it could help if we assign this PR to @bobbypage ?

@Creatone
Copy link
Collaborator Author

@Crapshit I don't think so.

@xeizmendi
Copy link

@dims @bobbypage @iwankgb @mrunalp in order to add podman support to cadvisor is creating a fork the only solution?

To me it looks like this PR is being ignored on purpose.

@iwankgb
Copy link
Collaborator

iwankgb commented Jun 8, 2023

@xeizmendi I will speak for myself: I am an volunteer working on cAdvisor in my free time. At the moment I'm probably half done with review of this PR and I find your comments offensive: this is free software, what stops you from helping out and reviewing this code?

@xeizmendi
Copy link

@iwankgb I thought only one of the project maintainers has the right to review/approved the code. If anybody can do so, then I'm sorry. Can anybody review the PR? Is there any documentation on what are the code review requirements for this project?

The PR has been sitting unreviewed for over six months and there have been no comments from core maintainers, hence my frustration.

@iwankgb
Copy link
Collaborator

iwankgb commented Jun 8, 2023

@xeizmendi lack of comments usually boils down to lack of time. This is reality of FOSS.
I will finish reviewing this PR as soon as possible and I hope that second round will be just a formality :) keep in mind that further progress will depend on @Creatone availability and his current priorities (in his free time, AFAIK).

@iwankgb
Copy link
Collaborator

iwankgb commented Jun 8, 2023

Regarding reviewing: everyone can review, maintainers can approve and merge.

cmd/internal/pages/podman.go Outdated Show resolved Hide resolved
cmd/internal/pages/podman.go Outdated Show resolved Hide resolved
container/docker/fs.go Outdated Show resolved Hide resolved
container/docker/fs.go Outdated Show resolved Hide resolved
container/docker/fs.go Outdated Show resolved Hide resolved
container/podman/factory.go Show resolved Hide resolved
container/podman/utils.go Outdated Show resolved Hide resolved
container/podman/podman.go Outdated Show resolved Hide resolved
container/podman/podman_test.go Show resolved Hide resolved
container/podman/podman.go Outdated Show resolved Hide resolved
container/podman/podman.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@iwankgb iwankgb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two comments, @Creatone

Signed-off-by: Paweł Szulik <paul.szulik@gmail.com>
Signed-off-by: Paweł Szulik <paul.szulik@gmail.com>
Signed-off-by: Paweł Szulik <paul.szulik@gmail.com>
@google google deleted a comment from k8s-ci-robot Jun 27, 2023
@Creatone Creatone merged commit b9342aa into google:master Jun 27, 2023
13 checks passed
@xeizmendi
Copy link

Thank you very much @Creatone, all the people who reviewed the code and to @iwankgb who unblocked this PR!

@pixelsoccupied
Copy link

Hello! Really appreciate the podman support and I was able to clone and build from the main branch without any issue.

Was wondering if there's any release schedule I can keep an eye on? I'm guessing the next release will have whatever is currently on the master branch?

p.s not sure if it's being tracked already/looking contribution but the current implementation (from 2-ish weeks ago) is missing equivalent -docker_only or -docker_tls

@mailinglists35
Copy link

mailinglists35 commented Jan 9, 2024

@Creatone I run each rootless podman container in their own Linux user shell. Is there any chance you can support this scenario?

I see cadvisor container already maps /var/run/ , and on host the users containers are in /run/user/ (/var/run is link to /run)

@mailinglists35
Copy link

@Creatone it would need to walk through each readable /var/run/user/*/podman/podman.sock files, so instead of a single endpointFlag in

endpointFlag = flag.String("podman", "unix:///var/run/podman/podman.sock", "podman endpoint")
I guess you would need to find a way to process multiple file socket paths instead of one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Podman containers