-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Saw |
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 |
@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. |
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. |
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{ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
@crosbymichael I've switched the ns creation implementation over to go:linkname from reexec. PTAL |
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 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vim
?
There was a problem hiding this comment.
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
Ping @AkihiroSuda |
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