Set interface name to the network_interface name for macvlan and ipvlan networks #21446

vikas-goel · 2024-01-30T20:41:32Z

When interface_name attribute in containers.conf file is set to "device", then set interface names inside containers same as the network_interface names of the respective network.

The change applies to macvlan and ipvlan networks only. The interface_name attribute value has no impact on any other types of networks.

If the interface name is set in the user request, then that takes precedence.

The interface_name attribute in the containers.conf file was added via containers/common#1814.

Fixes: #21313

Does this PR introduce a user-facing change? Yes

A new global attribute interface_name has been added to the containers.conf file. The default value of the attribute is "". The attribute value can be overridden to "device". This setting is effective for macvlan and ipvlan networks only. When set to "device", the network interface names inside containers will be set to the network_interface property value as seen in the podman inspect of the respective network, by default. If the network_interface property is not set or the global attribute is left unmodified, then the network interface name is set to the ethX pattern as usual, by default.

packit-as-a-service · 2024-01-30T22:34:26Z

Cockpit tests failed for commit a63870d820d721ead66bf64584227146f6869786. @martinpitt, @jelly, @mvollmer please check.

packit-as-a-service · 2024-01-31T04:55:22Z

Cockpit tests failed for commit 9350b183c51fbff305f4593011945fdcc84b7501. @martinpitt, @jelly, @mvollmer please check.

vikas-goel · 2024-01-31T07:03:50Z

@Luap99 , @TomSweeneyRedHat ,

The two machine related test case failures seem to be altogether in an independent area for unrelated reasons. For example, similar issue was seen in https://github.com/containers/podman/pull/21339/checks?check_run_id=21024221296.

What is the next step? Can we get it merged?

vikas-goel · 2024-01-31T07:08:47Z

/assign @Luap99 @TomSweeneyRedHat @rhatdan

vikas-goel · 2024-01-31T19:44:38Z

Pinging @TomSweeneyRedHat for review

vikas-goel · 2024-01-31T19:44:50Z

Pinging @rhatdan for review

libpod/runtime_ctr.go

libpod/networking_common.go

mheon · 2024-01-31T20:54:07Z

@edsantiago Mind taking a look at the tests?

TomSweeneyRedHat · 2024-01-31T20:56:38Z

@Luap99 is the SME here, and I'd like to get his eyeballs on this.
LGTM otherwise.

baude · 2024-01-31T20:57:41Z

general comments:

can you comment your tests more verbosely for the future
where is this documented
i understand this is paired with a containers-common pr, can you please cite it
can you please change the commit title to include something like macvlan only, in the theme of matt's feedback
please include macvlan only in the pr description as well

well done getting through CI! And appreciate the tests.

vikas-goel · 2024-01-31T23:14:08Z

general comments:

can you comment your tests more verbosely for the future

where is this documented

i understand this is paired with a containers-common pr, can you please cite it

can you please change the commit title to include something like macvlan only, in the theme of matt's feedback

please include macvlan only in the pr description as well

well done getting through CI! And appreciate the tests.

Incorporated all the comments. The attribute definition is documented in the c/common repo. The release notes in this PR describes the change.

libpod/networking_common.go

edsantiago · 2024-02-01T00:45:51Z

What bothers me a little about the tests is that, when run against main, all of them pass (or are skipped) rootless, and only one fails when run as root. If this is intentional -- like, good-citizen adding a whole bunch of tests unrelated to this PR -- yay and thank you! But if more than one of these tests are supposed to test the PR, there's something wrong.

I'll take a closer look tomorrow.

vikas-goel · 2024-02-01T00:52:07Z

What bothers me a little about the tests is that, when run against main, all of them pass (or are skipped) rootless, and only one fails when run as root. If this is intentional -- like, good-citizen adding a whole bunch of tests unrelated to this PR -- yay and thank you! But if more than one of these tests are supposed to test the PR, there's something wrong.

I'll take a closer look tomorrow.

There are 4/10 that skip in rootless, not all. They still make sense for root mode. If there is a better way, let me know.

edsantiago · 2024-02-01T00:59:39Z

What I'm trying to say is that most of these new tests do not test your PR. Are they supposed to?

vikas-goel · 2024-02-01T01:01:26Z

What I'm trying to say is that most of these new tests do not test your PR. Are they supposed to?

Not sure I am understanding. The PR CI result shows all of them are getting tested in root mode.

edsantiago · 2024-02-01T01:07:54Z

I write code.
I write a test for this code.
This new test must FAIL when run against the previous commit!

That is one of the most important and fundamental elements of testing.

Most of your new tests pass when run against main. this could be because
a. you are a kind person and have written a bunch of tests for functionality that your PR does not address, that has nothing whatsoever to do with your PR, but you found a hole in our testing and have corrected it. Or
b. your tests are not testing what you think you're testing

I do not have time or energy tonight to figure out which is the reason. So my question to you is, did you deliberately write a bunch of new tests that have nothing to do with your PR, simply to improve our coverage? Or were you expecting all of your new tests to be tests of your PR?

Thank you either way. We are all grateful for new code and new test coverage.

vikas-goel · 2024-02-01T01:15:06Z

I write code.

I write a test for this code.

This new test must FAIL when run against the previous commit!

That is one of the most important and fundamental elements of testing.

Most of your new tests pass when run against main. this could be because a. you are a kind person and have written a bunch of tests for functionality that your PR does not address, that has nothing whatsoever to do with your PR, but you found a hole in our testing and have corrected it. Or b. your tests are not testing what you think you're testing

I do not have time or energy tonight to figure out which is the reason. So my question to you is, did you deliberately write a bunch of new tests that have nothing to do with your PR, simply to improve our coverage? Or were you expecting all of your new tests to be tests of your PR?

Thank you either way. We are all grateful for new code and new test coverage.

I see what you are pointing to.

Most of the new test cases are related existing behavior. That is, checking the ethX interface naming inside container. That is why they are passing on the previous commit. So, what you said "written a bunch of tests for functionality that your PR does not address, that has nothing whatsoever to do with your PR, but you found a hole in our testing and have corrected it." is probably correct.

edsantiago

Feedback from a quick first pass. I need to switch context now, sorry.

test/e2e/container_iface_name_test.go

vikas-goel · 2024-02-02T18:56:26Z

@edsantiago , @mheon , @Luap99, @TomSweeneyRedHat , @rhatdan ,
How do we take it forward?

mheon · 2024-02-02T23:22:16Z

Once @Luap99 gets in next week and reviews, we can get through final review comments and merge.

Luap99

The tests seem to carry a ton of duplication which it makes it very hard to follow I suggest rewriting them in some form of loop to deduplicate most logic.

libpod/networking_common.go

test/e2e/container_iface_name_test.go

vikas-goel · 2024-02-05T20:27:22Z

The tests seem to carry a ton of duplication which it makes it very hard to follow I suggest rewriting them in some form of loop to deduplicate most logic.

Updated.

Luap99

Thanks, code looks good but we definitely need to have ipvlan tests working on netavark.

test/e2e/container_iface_name_test.go

vikas-goel · 2024-02-06T11:12:03Z

Thanks, code looks good but we definitely need to have ipvlan tests working on netavark.

It failed with a very clear statement that netavark does not support ipvlan.

Luap99 · 2024-02-06T12:59:55Z

It failed with a very clear statement that netavark does not support ipvlan.

What version are you using, the latest version support it and they should be in CI

Luap99 · 2024-02-06T13:06:15Z

Lets see if #21530 works, if it does then it should work here too.

vikas-goel · 2024-02-06T14:55:21Z

Lets see if #21530 works, if it does then it should work here too.

All ipvlan tests are failing in #21530. Will it make sense to get this PR in and enable ipvlan separately in #21530?

packit-as-a-service · 2024-02-06T18:44:37Z

Cockpit tests failed for commit c68f555b096d1046520343b2577c34f0402a59db. @martinpitt, @jelly, @mvollmer please check.

vikas-goel · 2024-02-06T19:57:35Z

After re-enabling ipvlan test for netavark in all debian tests, the same old issue happening.

# podman [options] run -d --network ipvlan5fd463d37d --name test quay.io/libpod/alpine:latest top
Error: netavark: unknown network driver ipvlan

Any clues? Is this a tested and supported combination for debian?

vikas-goel · 2024-02-06T22:10:18Z

System test failing randomly

sys remote fedora-38 root host boltdb

Failed tests (1):
 - 512 [500] podman run port forward range

edsantiago · 2024-02-06T22:59:59Z

Known flake, it's been hitting us very hard this past week (on f38 only). No point restarting it because the other failures are not flakes.

Looks like Debian has bad/old netavark (1.4). We don't have SkipIfDebian(), but we do have SkipIfRunc(). Would you mind adding that to the failing tests, with a loud explanation?

    SkipIfRunc("FIXME: Fails with netavark < 1.10. Reenable once Debian gets an update")

vikas-goel · 2024-02-06T23:37:23Z

Will the check for debian be info.Distribution == "debian"?

vikas-goel · 2024-02-07T02:15:30Z

Bypassing the test when all the following conditions are met

driver == "ipvlan"
info.Distribution == "debian"
pTest.OCIRuntime == "runc"

…an networks When interface_name attribute in containers.conf file is set to "device", then set interface names inside containers same as the network_interface names of the respective network. The change applies to macvlan and ipvlan networks only. The interface_name attribute value has no impact on any other types of networks. If the interface name is set in the user request, then that takes precedence. Fixes: #21313 Signed-off-by: Vikas Goel <[email protected]>

Luap99

/lgtm
/hold

openshift-ci · 2024-02-08T12:26:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, vikas-goel

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99]

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

mheon · 2024-02-08T13:30:12Z

/hold cancel

openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Jan 30, 2024

openshift-ci bot assigned Luap99, rhatdan and TomSweeneyRedHat Jan 31, 2024

openshift-ci bot added release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jan 31, 2024

mheon reviewed Jan 31, 2024

View reviewed changes

libpod/runtime_ctr.go Outdated Show resolved Hide resolved

mheon reviewed Jan 31, 2024

View reviewed changes

libpod/networking_common.go Outdated Show resolved Hide resolved

vikas-goel changed the title ~~Set interface name to the network_interface name~~ Set interface name to the network_interface name for macvlan and ipvlan networks Jan 31, 2024

mheon reviewed Feb 1, 2024

View reviewed changes

libpod/networking_common.go Outdated Show resolved Hide resolved

vikas-goel requested a review from mheon February 1, 2024 03:24

edsantiago reviewed Feb 1, 2024

View reviewed changes

test/e2e/container_iface_name_test.go Outdated Show resolved Hide resolved

test/e2e/container_iface_name_test.go Outdated Show resolved Hide resolved

test/e2e/container_iface_name_test.go Show resolved Hide resolved

test/e2e/container_iface_name_test.go Outdated Show resolved Hide resolved

vikas-goel requested a review from edsantiago February 1, 2024 15:21

Luap99 requested changes Feb 5, 2024

View reviewed changes

vikas-goel requested a review from Luap99 February 5, 2024 20:32

Luap99 reviewed Feb 6, 2024

View reviewed changes

test/e2e/container_iface_name_test.go Outdated Show resolved Hide resolved

test/e2e/container_iface_name_test.go Show resolved Hide resolved

vikas-goel requested a review from Luap99 February 7, 2024 21:22

Luap99 approved these changes Feb 8, 2024

View reviewed changes

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 8, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2024

openshift-merge-bot bot merged commit 9ad07d1 into containers:main Feb 8, 2024
88 of 89 checks passed

vikas-goel deleted the network branch February 8, 2024 20:19

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 9, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators May 9, 2024

Set interface name to the network_interface name for macvlan and ipvlan networks #21446

Set interface name to the network_interface name for macvlan and ipvlan networks #21446

Conversation

vikas-goel commented Jan 30, 2024 • edited Loading

Does this PR introduce a user-facing change? Yes

packit-as-a-service bot commented Jan 30, 2024

packit-as-a-service bot commented Jan 31, 2024

vikas-goel commented Jan 31, 2024

vikas-goel commented Jan 31, 2024

vikas-goel commented Jan 31, 2024

vikas-goel commented Jan 31, 2024

mheon commented Jan 31, 2024

TomSweeneyRedHat commented Jan 31, 2024

baude commented Jan 31, 2024

vikas-goel commented Jan 31, 2024

edsantiago commented Feb 1, 2024

vikas-goel commented Feb 1, 2024

edsantiago commented Feb 1, 2024

vikas-goel commented Feb 1, 2024

edsantiago commented Feb 1, 2024

vikas-goel commented Feb 1, 2024 • edited Loading

edsantiago left a comment

Choose a reason for hiding this comment

vikas-goel commented Feb 2, 2024

mheon commented Feb 2, 2024

Luap99 left a comment

Choose a reason for hiding this comment

vikas-goel commented Feb 5, 2024

Luap99 left a comment

Choose a reason for hiding this comment

vikas-goel commented Feb 6, 2024

Luap99 commented Feb 6, 2024

Luap99 commented Feb 6, 2024

vikas-goel commented Feb 6, 2024

packit-as-a-service bot commented Feb 6, 2024

vikas-goel commented Feb 6, 2024 • edited Loading

vikas-goel commented Feb 6, 2024

edsantiago commented Feb 6, 2024

vikas-goel commented Feb 6, 2024 • edited Loading

vikas-goel commented Feb 7, 2024

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 8, 2024

mheon commented Feb 8, 2024

vikas-goel commented Jan 30, 2024 •

edited

Loading

vikas-goel commented Feb 1, 2024 •

edited

Loading

vikas-goel commented Feb 6, 2024 •

edited

Loading

vikas-goel commented Feb 6, 2024 •

edited

Loading