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

use libnetwork from c/common for networking #3690

Merged
merged 2 commits into from
Jan 7, 2022

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Jan 6, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:

setup the netns in the buildah parent process

Do not configure the netns in the runtime child process, this removes
the need to send the network options to the child. This will be needed
for the new libnetwork network interface which cannot be transfered as
json.
To synchronize this between the child and parent we use two pipe pairs.

use libnetwork from c/common for networking

Podman uses the new netavark network stack. Buildah should be able to do
the same. Both projects should use the same networking code which was
move to c/common/libnetwork. The new network interface can use either
CNI or netvavark. Using the same code for podman and buildah is
important to ensure that both use the same backend. Mixing CNI and
netavark is not supported.
This also fixes some outstanding CNI issues, e.g. buildah trying to
connect all cni networks.

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?


@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 6, 2022
@Luap99 Luap99 force-pushed the network branch 2 times, most recently from de71103 to 79e9cbb Compare January 6, 2022 14:14
new.go Show resolved Hide resolved
new.go Outdated Show resolved Hide resolved
@nalind
Copy link
Member

nalind commented Jan 6, 2022

Assuming it passes tests, this looks reasonable.

@Luap99 Luap99 force-pushed the network branch 2 times, most recently from 22f32b2 to 3c2e420 Compare January 6, 2022 20:43
@TomSweeneyRedHat
Copy link
Member

Changes look reasonable to me, but the test system isn't at all happy and you need a rebase here.

@Luap99
Copy link
Member Author

Luap99 commented Jan 7, 2022

Yeah that is a regression in c/common, I opened containers/common#875 to revert the change for now.

Do not configure the netns in the runtime child process, this removes
the need to send the network options to the child. This will be needed
for the new libnetwork network interface which cannot be transfered as
json.

To synchronize this between the child and parent we use two pipe pairs.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Jan 7, 2022

/lgtm
/approve
/hold

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, rhatdan

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 label Jan 7, 2022
Podman uses the new netavark network stack. Buildah should be able to do
the same. Both projects should use the same networking code which was
move to c/common/libnetwork. The new network interface can use either
CNI or netvavark. Using the same code for podman and buildah is
important to ensure that both use the same backend. Mixing CNI and
netavark is not supported.

This also fixes some outstanding CNI issues, e.g. buildah trying to
connect all cni networks.

[NO NEW TESTS NEEDED]

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Jan 7, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 7, 2022
@rhatdan
Copy link
Member

rhatdan commented Jan 7, 2022

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit c03b997 into containers:main Jan 7, 2022
@Luap99 Luap99 deleted the network branch January 7, 2022 19:23
@flouthoc
Copy link
Collaborator

flouthoc commented Jan 7, 2022

@Luap99 After this PR My builds are failing with error creating build container: error opening "/etc/cni/net.d/cni.lock": permission denied

Edit: Rootless builds

@Luap99
Copy link
Member Author

Luap99 commented Jan 8, 2022

Ah sorry, is there no rootless testing in CI?
I will fix it on Monday.

@flouthoc
Copy link
Collaborator

flouthoc commented Jan 8, 2022

No issues not urgent i am still able to do work with root builds.

@Luap99 Luap99 restored the network branch January 25, 2022 14:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved kind/feature Categorizes issue or PR as related to a new feature. lgtm locked - please file new issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants