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

add cni networking support #1073

Merged
merged 6 commits into from
Jul 19, 2019
Merged

add cni networking support #1073

merged 6 commits into from
Jul 19, 2019

Conversation

tonistiigi
Copy link
Member

@tonistiigi tonistiigi commented Jul 11, 2019

based on the @kunalkushwaha earlier implementation

The integration tests now use bridge networking by default. Plan is to make it default in the release image as well in follow-up.

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi
Copy link
Member Author

Saw CNI setup error: failed to set bridge addr: could not add IP address to "buildkit0": file exists in the CI. Does anyone have ideas? Should I guard the cni setup with a lockfile?

@crosbymichael
Copy link
Contributor

I guess the setup needs to be locked so that it does not try to create the bridge when two initialization are running. It maybe better to init the bridge separately at boot instead of on-demand when a container needs it. You could probably do this by creating a temp netns and initializing it

@tonistiigi
Copy link
Member Author

@crosbymichael Do I need to keep this ns running then or can I just setup and then remove it. First one would make it tricky to avoid leaks.

@tonistiigi
Copy link
Member Author

In the tests multiple daemons will use the same bridge so even this temp ns would probably need to be protected. So a lockfile still seems like a way to go.

@crosbymichael
Copy link
Contributor

You can just create an initial netns so that the bridge is initialized at boot then destroy it.

Usage: "path of cni config file",
Value: defaultConf.Workers.OCI.NetworkConfig.CNIConfigPath,
},
cli.StringFlag{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you making two different paths for OCI and containerd cni binaries? I think it would be safe to just have one dir

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from a data structure perspective workers should be isolated, eg. there should be a remote worker in the future that could run on a different machine (and different net config). So I want these values to be tied to the worker, not to the builder instance.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi
Copy link
Member Author

@crosbymichael I've switched the ns creation implementation over to go:linkname from reexec. PTAL

@crosbymichael
Copy link
Contributor

namespace changes LGTM

FROM buildkit-base AS integration-tests
ENV BUILDKIT_INTEGRATION_ROOTLESS_IDPAIR="1000:1000"
RUN apt-get install -y --no-install-recommends uidmap sudo \
RUN apt-get install -y --no-install-recommends uidmap sudo vim iptables \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vim ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a new dev target

@tiborvass
Copy link
Collaborator

Ping @AkihiroSuda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants