Initial podman support #2794

towe75 · 2021-01-31T16:06:02Z

This PR adds simple support for podman based containers.

I tested it only with rootful containers, also there is no filesystem watcher yet. It simply uses the docker API client. A more sophisticated version would have to use the native podman API to access i.e. the storage graph details etc.

It's, however, a good starting point and might be useful for some people, see #2424

google-cla · 2021-01-31T16:06:07Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

k8s-ci-robot · 2021-01-31T16:06:11Z

Hi @towe75. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

towe75 · 2021-01-31T16:07:53Z

@googlebot I signed it!

Creatone

Could you provide tests?

container/podman/client.go

container/podman/factory.go

container/podman/handler.go

container/podman/podman.go

info/v1/podman.go

container/podman/handler.go

container/podman/factory.go

Creatone · 2021-02-01T11:45:52Z

/ok-to-test

towe75 · 2021-02-01T12:29:04Z

@Creatone : thank you for having a look. I resolved most issues. Regarding tests: i will have a look but i wanted to learn if there is a general interest on a podman related PR before i put to much effort into into it.

bobbypage · 2021-02-03T06:20:28Z

/cc @mrunalp @haircommander since they may have some more experience with Podman.

haircommander · 2021-02-03T14:31:58Z

code LGTM, as a note, you can probably scrape FS metrics the same way the crio handler does, as we share image and container storage.

@containers/podman-maintainers PTAL

towe75 · 2021-02-03T15:29:53Z

@haircommander : thank you for the FS hint! I will have a look but it will go into another PR.

soinu · 2021-02-21T20:36:08Z

@containers/podman-maintainers PTAL

I hope it is okay to ping you, @mheon @rhatdan

rhatdan · 2021-02-22T18:28:47Z

@soinu No problem, I love to see more users of Podman.

rhatdan · 2021-02-22T18:32:14Z

@baude FYI

giuseppe

just some nits but LGTM

container/podman/factory.go

giuseppe · 2021-03-24T16:49:10Z

container/podman/podman.go

+	for index, versionStr := range versionStringArray {
+		version, err := strconv.Atoi(versionStr)
+		if err != nil {
+			return nil, fmt.Errorf("error while parsing \"%v\" in \"%v\"", versionStr, versionString)


could it be %q instead of "%v"?

It's copied from docker/docker.go and boils IMHO down to a "\d+" match and thus could be multi-char string in case of a version segment greater "9". docker_test.go checks for this case and thus i think it's ok.

Refactoring this version parser stuff into a separate driver independent package would be a good idea.

iwankgb

Take a look at my comments, please :)

container/podman/client.go

container/podman/factory.go

container/podman/podman.go

container/podman/client.go

iwankgb · 2021-03-25T17:42:06Z

container/podman/plugin.go

+		case context.DeadlineExceeded:
+			klog.Warningf("Timeout trying to communicate with podman during initialization, will retry")
+		default:
+			klog.V(5).Infof("Podman not connected: %v", err)


This is were you should call CancelFunc, I believe.

Oh, use %w, please :)

iwankgb · 2021-03-25T17:44:33Z

container/podman/plugin.go

+
+		switch err {
+		case context.DeadlineExceeded:
+			klog.Warningf("Timeout trying to communicate with podman during initialization, will retry")


It seems to me that if we keep getting context timeout then the for loop will become an infinite one. You must define a condition that will break it at some point.

iwankgb · 2021-03-25T17:49:12Z

info/v1/podman.go

+	NumContainers int               `json:"num_containers"`
+}
+
+type PodmanImage struct {


I might have missed it but I can't recall this struct being used. Is it needed?

iwankgb · 2021-03-25T17:51:50Z

container/podman/podman.go

+	return StatusWithContext(defaultContext())
+}
+
+func StatusWithContext(ctx context.Context) (v1.PodmanStatus, error) {


Does this function must be exported? This is more general remark: there is substantial amount of exported function in podman package that do not seem to implement an interface and are not used anywhere outside of the package. Can you unexplored them? I would rather expect them to be exported on case-by-case basis when we clearly see a use case for them.

iwankgb · 2021-03-25T17:52:56Z

container/podman/podman.go

+
+// Checks whether the podmanInfo reflects a valid docker setup, and returns it if it does, or an
+// error otherwise.
+func ValidateInfo() (*dockertypes.Info, error) {


Where is this function used?

iwankgb · 2021-03-25T17:54:06Z

Oh, there are no tests in this PR at all. What is your testing strategy for this code?

towe75 · 2021-04-03T09:08:54Z

@iwankgb : thank you for the detailed review.

I am a bit surprised about the effort that is put into reviewing this code. But up to now nobody recognized that most of the plumbing/boilerplate code is copied from the docker support. Working on cadvisor is completely new to me but i did of course not start from scratch. I copied the docker package and ripped out all what i did not need for a very basic, barebones, podman support. So i guessed that this might be the kind of code/style that you want to have in this project.

Anyhow. I will certainly improve the code and i can add some tests, too. But i strongly recommend a re-review of the docker and maybe other packages if you want to enforce a certain coding style. Otherwise it's somewhat hard for contributors to learn about your teams expectations.

iwankgb · 2021-04-03T09:19:17Z

@towe75 I hope that one day cAdvisor will be more consistent but it's next to impossible to achieve this when Kubernetes depends on us (imagine what happens when you want to make exported field names to match Go style). I don't think that we should not try to improve the project by scrutinising new code even if it's based on existing parts of the project :)

towe75 · 2021-04-03T09:29:22Z

@iwankgb : no worries. So maybe the podman package can become "the goto example" of how to support new container types ;-)

grouped some const() and var() renamed Client() to NewClient()

iwankgb · 2021-05-08T07:56:48Z

/retest

giuseppe · 2021-11-29T08:26:10Z

@towe75 are you still working on this PR?

towe75 · 2021-11-29T13:10:29Z

@giuseppe : TBH no. I lost track of it and i don't have enough spare time recently. It was obviously a bad idea to clone older code from the docker support. We can as well close it and start from scratch on another day if there is some demand for it. Sorry.

giuseppe · 2021-11-29T13:51:12Z

@giuseppe : TBH no. I lost track of it and i don't have enough spare time recently. It was obviously a bad idea to clone older code from the docker support. We can as well close it and start from scratch on another day if there is some demand for it. Sorry.

thanks for the update! I think cAdvisor support for Podman is still needed

Creatone · 2021-11-29T13:54:21Z

@giuseppe : TBH no. I lost track of it and i don't have enough spare time recently. It was obviously a bad idea to clone older code from the docker support. We can as well close it and start from scratch on another day if there is some demand for it. Sorry.

thanks for the update! I think cAdvisor support for Podman is still needed

So I would like to implement that!

rhatdan · 2021-11-29T15:06:27Z

Go for it.

Creatone · 2021-12-01T11:32:01Z

Moved to #3021.

Initial podman support

99129cd

google-cla bot added the cla: no label Jan 31, 2021

k8s-ci-robot added the needs-ok-to-test label Jan 31, 2021

google-cla bot added cla: yes and removed cla: no labels Jan 31, 2021

Make vet/linter happy

98c6d73

towe75 mentioned this pull request Jan 31, 2021

Add support for Podman containers #2424

Closed

Creatone self-requested a review February 1, 2021 08:30

Creatone reviewed Feb 1, 2021

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Feb 1, 2021

towe75 added 2 commits February 1, 2021 13:22

Review

3a98eeb

Comment

7fe4cd1

Review

cf729fc

giuseppe reviewed Mar 24, 2021

View reviewed changes

iwankgb requested changes Mar 25, 2021

View reviewed changes

Review

4f2957b

Review

586ceee

grouped some const() and var() renamed Client() to NewClient()

Creatone closed this Dec 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial podman support #2794

Initial podman support #2794

towe75 commented Jan 31, 2021

google-cla bot commented Jan 31, 2021

k8s-ci-robot commented Jan 31, 2021

towe75 commented Jan 31, 2021

Creatone left a comment

Creatone commented Feb 1, 2021

towe75 commented Feb 1, 2021

bobbypage commented Feb 3, 2021

haircommander commented Feb 3, 2021

towe75 commented Feb 3, 2021

soinu commented Feb 21, 2021

rhatdan commented Feb 22, 2021

rhatdan commented Feb 22, 2021

giuseppe left a comment

giuseppe Mar 24, 2021

towe75 Apr 3, 2021

iwankgb left a comment

iwankgb Mar 25, 2021

iwankgb Mar 25, 2021

iwankgb Mar 25, 2021

iwankgb Mar 25, 2021

iwankgb Mar 25, 2021

iwankgb Mar 25, 2021

iwankgb commented Mar 25, 2021

towe75 commented Apr 3, 2021

iwankgb commented Apr 3, 2021

towe75 commented Apr 3, 2021

iwankgb commented May 8, 2021

giuseppe commented Nov 29, 2021

towe75 commented Nov 29, 2021

giuseppe commented Nov 29, 2021

Creatone commented Nov 29, 2021

rhatdan commented Nov 29, 2021

Creatone commented Dec 1, 2021

Initial podman support #2794

Initial podman support #2794

Conversation

towe75 commented Jan 31, 2021

google-cla bot commented Jan 31, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

k8s-ci-robot commented Jan 31, 2021

towe75 commented Jan 31, 2021

Creatone left a comment

Choose a reason for hiding this comment

Creatone commented Feb 1, 2021

towe75 commented Feb 1, 2021

bobbypage commented Feb 3, 2021

haircommander commented Feb 3, 2021

towe75 commented Feb 3, 2021

soinu commented Feb 21, 2021

rhatdan commented Feb 22, 2021

rhatdan commented Feb 22, 2021

giuseppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwankgb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwankgb commented Mar 25, 2021

towe75 commented Apr 3, 2021

iwankgb commented Apr 3, 2021

towe75 commented Apr 3, 2021

iwankgb commented May 8, 2021

giuseppe commented Nov 29, 2021

towe75 commented Nov 29, 2021

giuseppe commented Nov 29, 2021

Creatone commented Nov 29, 2021

rhatdan commented Nov 29, 2021

Creatone commented Dec 1, 2021