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

Wire network interface into libpod #11322

Merged
merged 7 commits into from
Sep 15, 2021

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Aug 25, 2021

Wire network interface into libpod

Make use of the new network interface in libpod.

This commit contains several breaking changes:

  • podman network create only outputs the new network name and not file
    path.
  • podman network ls shows the network driver instead of the cni version
    and plugins.
  • podman network inspect outputs the new network struct and not the cni
    conflist.
  • The bindings and libpod api endpoints have been changed to use the new
    network structure.

The container network status is stored in a new field in the state. The
status should be received with the new c.getNetworkStatus. This will
migrate the old status to the new format. Therefore old containers should
continue to work correctly in all cases even when network connect/
disconnect is used.

New features:

  • podman network reload keeps the ip and mac for more than one network.
  • podman container restore keeps the ip and mac for more than one
    network.
  • The network create compat endpoint can now use more than one ipam
    config.

The man pages and the swagger doc are updated to reflect the latest
changes.

Drop OCICNI dependency

We do not use the ocicni code anymore so let's get rid of it. Only the
port struct is used but we can copy this into libpod network types so
we can debloat the binary.

The next step is to remove the OCICNI port mapping form the container
config and use the better PortMapping struct everywhere.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 25, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2021
@Luap99 Luap99 force-pushed the network-libpod branch 2 times, most recently from fa4b6f5 to 7d4bd3c Compare August 25, 2021 12:58
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2021
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2021
@Luap99 Luap99 force-pushed the network-libpod branch 3 times, most recently from c00232c to 3624b31 Compare August 27, 2021 13:15
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2021
@Luap99
Copy link
Member Author

Luap99 commented Aug 30, 2021

@edsantiago Is there something like skip_if_not_fedora for the system tests? One new test needs containernetworking-plugins 1.0 or newer. This is only available in fedora at the moment. I would like to do a version check but /usr/libexec/cni/bridge just prints version unknown. So I guess I have to use skip_if_ubuntu/skip_if_not_fedora until the package is updated. Or do you know a better way?

@edsantiago
Copy link
Member

There are no such skip helpers, because I consider such usage (checking for OS/distro) improper: the correct way is to test for functionality. ISTM that you actually have that?

    cni=/usr/libexec/cni/bridge       ! FIXME: there should be a better way to find this, but `podman info` does not help
    test -x $cni || skip "CNI executable $cni not installed"
    run $cni --version
    cni_version=$(expr "$output" : ".* plugin version \(.*\)")
    [[ $cni_version -ge 1.0 ]] || skip "Test requires CNI >= 1.0 (found: $cni_version)"

Using [[ will treat unknown as < 1.0 without emitting a warning. Using test will also work, but with "integer expression expected" warning.

@Luap99
Copy link
Member Author

Luap99 commented Aug 30, 2021

/usr/libexec/cni/bridge always prints unknown even for 1.0 so this does not work.

@Luap99
Copy link
Member Author

Luap99 commented Aug 30, 2021

Also it looks like the path is /usr/lib/cni/bridge on debian/ubuntu

@edsantiago
Copy link
Member

/usr/libexec/cni/bridge always prints unknown even for 1.0 so this does not work.

Well, that's obviously a bug and it must be fixed; but equally obviously, it can't get fixed in time for your PR.

What is the shortest, simplest way to behavior-check and identify good-new-cni vs old-bad-cni? Is there a podman network foo bar that will exit error with old-cni and success on new-cni? If so, you could add such a test to setup() in the system test file that requires it. (If multiple test files, then it's better to add require_cni_with_FEATURENAME to helpers.bash).

@Luap99
Copy link
Member Author

Luap99 commented Aug 30, 2021

For now this is the only test which requires the new functionality so it doesn't make sense to use the same test to check if the test should be skipped so I will remove the parts which require v1.0. In future PRs I will add functionality to test this in a reasonable way.

@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2021
@Luap99 Luap99 marked this pull request as ready for review September 1, 2021 11:20
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2021
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2021
@TomSweeneyRedHat
Copy link
Member

Manpage changes LGTM.
I did a very high-level skim review and it LGTM and we've happy green test buttons.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2021
@Luap99
Copy link
Member Author

Luap99 commented Sep 14, 2021

/hold cancel
we have a 3.4 branch so this can merge

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2021
@Luap99
Copy link
Member Author

Luap99 commented Sep 14, 2021

@mheon @baude PTAL

@mheon
Copy link
Member

mheon commented Sep 14, 2021

You have a vendor conflict. LGTM once that's resolved.

Check that the given subnet does not conflict with existing ones (other
configs or host interfaces).

Signed-off-by: Paul Holzinger <[email protected]>
The default network should not be validated against used subnets, we have to ensure
that this network can always be created even when a subnet is already used on the host.
This could happen if you run a container on this net, then the cni interface will be
created on the host and "block" this subnet from being used again.
Therefore the next podman command tries to create the default net again and it would
fail because it thinks the network is used on the host.

Signed-off-by: Paul Holzinger <[email protected]>
When configs are loaded from disk we need to check if they contain a
ipv6 subnet and set ipv6 enables to true in this case.

Signed-off-by: Paul Holzinger <[email protected]>
Make use of the new network interface in libpod.

This commit contains several breaking changes:
- podman network create only outputs the new network name and not file
  path.
- podman network ls shows the network driver instead of the cni version
  and plugins.
- podman network inspect outputs the new network struct and not the cni
  conflist.
- The bindings and libpod api endpoints have been changed to use the new
  network structure.

The container network status is stored in a new field in the state. The
status should be received with the new `c.getNetworkStatus`. This will
migrate the old status to the new format. Therefore old containers should
contine to work correctly in all cases even when network connect/
disconnect is used.

New features:
- podman network reload keeps the ip and mac for more than one network.
- podman container restore keeps the ip and mac for more than one
  network.
- The network create compat endpoint can now use more than one ipam
  config.

The man pages and the swagger doc are updated to reflect the latest
changes.

Signed-off-by: Paul Holzinger <[email protected]>
We do not use the ocicni code anymore so let's get rid of it. Only the
port struct is used but we can copy this into libpod network types so
we can debloat the binary.

The next step is to remove the OCICNI port mapping form the container
config and use the better PortMapping struct everywhere.

Signed-off-by: Paul Holzinger <[email protected]>
Rootless cni with ipv6 needs the `ip6_tables` module loaded, normally
the cni plugins will load this module but as rootless it does not have
the necessary permission to do so. Therefore we load it manually.

Signed-off-by: Paul Holzinger <[email protected]>
Drivers should return the list of supported network drivers by this
plugin. This is useful for podman info.

Signed-off-by: Paul Holzinger <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Sep 15, 2021

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 15, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2021
@rhatdan
Copy link
Member

rhatdan commented Sep 15, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 15, 2021
@openshift-merge-robot openshift-merge-robot merged commit 5f41ffd into containers:main Sep 15, 2021
@Luap99 Luap99 deleted the network-libpod branch September 15, 2021 20:12
@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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 this pull request may close these issues.

6 participants