[wip] podman: add support for specifying MAC

[filak-sap]

Signed-off-by: Jakub Filak [email protected]

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: filak-sap
To complete the pull request process, please assign umohnani8
You can assign the PR to them by writing /assign @umohnani8 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci-robot]

Hi @filak-sap. Thanks for your PR.

I'm waiting for a containers or openshift 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.

Details

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.

[filak-sap]

Depends on cri-o/ocicni#61

[adrianreber]

@filak-sap Thanks for including the MAC support also in the checkpoint/restore code paths. From the checkpoint/restore part the newly added flags would need to be added to the bash completion and man-pages. Also a checkpoint/restore test using a static MAC would be helpful.

[rhatdan]

/ok-to-test

[filak-sap]

/retest

[filak-sap]

D'oh, sorry, the build is failing because of the missing MAC:
_build/src/github.com/containers/libpod/libpod/networking_linux.go:50:6: rt.MAC undefined (type ocicni.RuntimeConfig has no field or method MAC)

[mheon]

Yeah, I don't think the tests will pass until OCICNI changes merge and get vendored here

[filak-sap]

Amended with bashcompletions and man:

diff --git a/completions/bash/podman b/completions/bash/podman
index 4bc38787..77f45619 100644
--- a/completions/bash/podman
+++ b/completions/bash/podman
@@ -877,6 +877,7 @@ _podman_container_restore() {
          --tcp-established
          --ignore-rootfs
          --ignore-static-ip
+         --ignore-static-mac
      "
      case "$prev" in
         -i|--import)
diff --git a/docs/podman-container-restore.1.md b/docs/podman-container-restore.1.md
index 1d2cf0b3..d71daf4a 100644
--- a/docs/podman-container-restore.1.md
+++ b/docs/podman-container-restore.1.md
@@ -76,6 +76,15 @@ a container is restored multiple times from an exported checkpoint with **--name
 Using **--ignore-static-ip** tells Podman to ignore the IP address if it was configured
 with **--ip** during container creation.
 
+**--ignore-static-mac**
+
+If the container was started with **--mac-address** the restored container also
+tries to use that MAC address and restore fails if that MAC address is already
+in use. This can happen, if a container is restored multiple times from an
+exported checkpoint with **--name, -n**.
+
+Using **--ignore-static-mac** tells Podman to ignore the MAC address if it was
+configured with **--mac-address** during container creation.
 ## EXAMPLE
 
 podman container restore mywebserver

I found only 1 occurrence of --ignore-static-ip in the tests (grep -r ignore-static-ip) and it is not clear to me which case is verified because I cannot see any asserts for IP:
https://github.com/containers/libpod/blob/master/test/e2e/checkpoint_test.go#L392

[rhatdan]

You need to run all of your code through gofmt.

[filak-sap]

Updated tests:

diff --git a/test/e2e/checkpoint_test.go b/test/e2e/checkpoint_test.go
index 1caefd29..90c6fc29 100644
--- a/test/e2e/checkpoint_test.go
+++ b/test/e2e/checkpoint_test.go
@@ -334,6 +334,10 @@ var _ = Describe("Podman checkpoint", func() {
                IPBefore.WaitWithDefaultTimeout()
                Expect(IPBefore.ExitCode()).To(Equal(0))
 
+               MACBefore := podmanTest.Podman([]string{"inspect", "-l", "--format={{.NetworkSettings.MacAddress}}"})
+               MACBefore.WaitWithDefaultTimeout()
+               Expect(MACBefore.ExitCode()).To(Equal(0))
+
                result := podmanTest.Podman([]string{"container", "checkpoint", "test_name"})
                result.WaitWithDefaultTimeout()
 
@@ -348,9 +352,16 @@ var _ = Describe("Podman checkpoint", func() {
                IPAfter.WaitWithDefaultTimeout()
                Expect(IPAfter.ExitCode()).To(Equal(0))
 
+               MACAfter := podmanTest.Podman([]string{"inspect", "-l", "--format={{.NetworkSettings.MacAddress}}"})
+               MACAfter.WaitWithDefaultTimeout()
+               Expect(MACAfter.ExitCode()).To(Equal(0))
+
                // Check that IP address did not change between checkpointing and restoring
                Expect(IPBefore.OutputToString()).To(Equal(IPAfter.OutputToString()))
 
+               // Check that MAC address did not change between checkpointing and restoring
+               Expect(MACBefore.OutputToString()).To(Equal(MACAfter.OutputToString()))
+
                Expect(result.ExitCode()).To(Equal(0))
                Expect(podmanTest.NumberOfContainersRunning()).To(Equal(1))
                Expect(podmanTest.GetContainerStatus()).To(ContainSubstring("Up"))

[filak-sap]

Amended with changes made by make gofmt.

I am sorry but I am not able to make make localunit working - it dies for missing dependencies while I can build the repo.

[mheon]

OCICNI changes are landed, so vendoring the new version should get support in the library.

[filak-sap]

• I am not sure how to update the directory vendor and I am not even sure you want me to do that. I can compile podmam if I cherry pick the ocicni patch but if I update all ocicni source files, I get strange errors - probably dependency problems.
• I added Dan's version of error handler.

[mheon]

I think the update process for vendor/ is:

export GO111MODULE=on
go get github.com/cri-o/ocicni [revision to get]
make vendor
export GO111MODULE=""

@vrothberg Concur?

[vrothberg]

After a go get github.com/cri-o/ocicni@deac903fd99b6c52d781c9f42b8db3af7dcfd00a and make vendor I get the following:

libpod/networking_linux.go:37:3: cannot use networks (type []string) as type []ocicni.NetAttachment in field value
libpod/networking_linux.go:44:20: cannot use []string literal (type []string) as type []ocicni.NetAttachment in assignment
libpod/networking_linux.go:96:46: r.String undefined (type ocicni.NetResult has no field or method String)
libpod/networking_linux.go:97:43: cannot use r (type ocicni.NetResult) as type "github.com/containernetworking/cni/pkg/types".Result in argument to current.GetResult:
        ocicni.NetResult does not implement "github.com/containernetworking/cni/pkg/types".Result (missing GetAsVersion method)
libpod/networking_linux.go:99:77: r.String undefined (type ocicni.NetResult has no field or method String)
make: *** [Makefile:162: podman] Error 2

Looks like ocicni had quite some breaking changes. If this commit needs a new version of ocicni, the compilation issues must be addressed as well.

[vrothberg]

I restarted the failed tests - looked like flakes as the tests pass locally on my machine with this PR.

[vrothberg]

The checkpoint tests fail reproducibly in the CI:

[+0718s] • Failure [4.585 seconds]
[+0718s] Podman checkpoint
[+0718s] /var/tmp/go/src/github.com/containers/libpod/test/e2e/checkpoint_test.go:22
[+0718s]   podman checkpoint and restore container with same IP [It]
[+0718s]   /var/tmp/go/src/github.com/containers/libpod/test/e2e/checkpoint_test.go:327
[+0718s] 
[+0718s]   Expected
[+0718s]       <string>: e6:74:2a:e5:17:c6
[+0718s]   to equal
[+0718s]       <string>: 66:db:1c:ac:0c:d4
[+0718s] 
[+0718s]   /var/tmp/go/src/github.com/containers/libpod/test/e2e/checkpoint_test.go:363

[mheon]

I'll take a look at the checkpoint failures on Monday

[filak-sap]

Unfortunately, I am not sure what code actually sets MAC address to the given value. I found only tuning plugin at https://github.com/containernetworking/plugins

I would love to fix the problem but I am not sure how to run end-2-end tests.

[filak-sap]

Amended with:

diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index e9848e34..d53a74aa 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -1356,6 +1356,6 @@ func (c *Container) copyOwnerAndPerms(source, dest string) error {
 // Teardown CNI config on refresh
 func (c *Container) refreshCNI() error {
        // Let's try and delete any lingering network config...
-       podNetwork := c.runtime.getPodNetwork(c.ID(), c.config.Name, "", c.config.Networks, c.config.PortMappings, c.config.StaticIP)
+       podNetwork := c.runtime.getPodNetwork(c.ID(), c.config.Name, "", c.config.Networks, c.config.PortMappings, c.config.StaticIP, c.config.StaticMAC)
        return c.runtime.netPlugin.TearDownPod(podNetwork)
 }

in order to fix build after commit b57d2f4

[mheon]

I don't know if we actually expect these to be the same, at present. podman checkpoint seems to have special code for restoring IP address, which I don't think is duplicated for MAC yet.

I don't know if we care, at this point - it might be safe to remove this test, which is the only thing not passing?

[filak-sap]

Well, I am afraid that my changes pass the requested MAC to container networks but there is nothing reading it and changing network interface.

Can you give me a hint where I need to add the missing MAC handler, please?

[mheon]

The changes you made to pod network get it into OCICNI, so I think this should work from our end. You might want to add some debug to make absolutely sure the settings are getting to getPodNetwork(). It they are, it's probably somewhere in ocicni.

[mheon]

Ahhh - I think you're missing the actual configuration of static IP from Podman.

Look at pkg/spec/createconfig.go, function getContainerCreateOptions() - you need to generate a WithStaticMac() there if the CreateConfig field MacAddress is set.

[filak-sap]

Ooops, I ignore errors (_) but I test err to nil on the very next line.

[filak-sap]

@mheon I at least try to add MAC to the network options.

[mheon]

@adrianreber Mind looking at the checkpoint/restore bits here? The rest seems solid to me.

[adrianreber]

@filak-sap About the checkpoint/restore failures: I think I am missing some context here. As you added code to handle static MAC addresses during restore it seems that this is something you tested and care about. That was my assumption. Seeing that the test cases fail contradicts this assumption. So it would be good to understand if checkpoint/restore with the same MAC is important to you. I would be very happy to have this feature, but if it is not something you actually care right now, I would say to either always restore with a random MAC (as it was before) and ignore the statically set MAC or just fail if the checkpointed container contains a static MAC setting.

This way you can get your MAC feature into Podman and the checkpoint/restore with static MAC addresses can be done later. Again, I would be happy if this PR would add the MAC feature for checkpoint/restore, but if this is not important for you I would just skip it like described above. Ignore the static MAC on restore, fail on restore with a static MAC or early abort the restore if a static MAC is detected.

[rh-atomic-bot]

☔ The latest upstream changes (presumably #4151) made this pull request unmergeable. Please resolve the merge conflicts.

[github-actions]

This pull request had no activity for 30 days. In the absence of activity or the "do-not-close" label, the pull request will be automatically closed within 7 days.

[rhatdan]

We definitely still want this. @filak-sap Are you still working on this?

[rhatdan]

@mheon is this something you could take over?

[filak-sap]

@rhatdan I am a bit busy these days. I might have find free time next month :(

[rhatdan]

Ok we will see if @mheon can take it over the finish line.

[rhatdan]

@giuseppe You have time for this one?

[giuseppe]

I've rebased the PR and opened a new one: #4451