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

WIP: containerd: Support for kubenet-style networking modes #10586

Closed
wants to merge 1 commit into from

Conversation

justinsb
Copy link
Member

By configuration conf_template, we can continue to support these
networking modes.

By configuration conf_template, we can continue to support these
networking modes.
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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

@k8s-ci-robot k8s-ci-robot added area/api area/nodeup approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 15, 2021
@k8s-ci-robot
Copy link
Contributor

@justinsb: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kops-verify-bazel 3a83587 link /test pull-kops-verify-bazel

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@hakman
Copy link
Member

hakman commented Jan 15, 2021

@justinsb I removed something similar in c02e5a2.
The problem with it isn't that it wouldn't work at all, but that is just unsupported and should not be used as default or in production.

@justinsb
Copy link
Member Author

@hakman that's surprising; this is how all the CNI plugins work :-) Do you have a link to what's deprecated?

I can imagine that maybe conf_template is deprecated, in which case each provider (e.g. kopeio networking) might have to implement this behaviour.

We can also change the default to a different CNI provider, though then we'd have to agree what the new default should be, which will be tricky...

@justinsb
Copy link
Member Author

Ah - I bet the confusion is passing --networking=kubenet to kubelet, vs using CNI in a kubenet-style. By kubenet-style I mean that nodes are configured externally with routes (either by the cloudprovider or by a CNI provider), and that we don't need to do anything per-pod.

Maybe we should come up with a better name for "kubenet-style" to avoid the confusion!

The reason I pursued this was because otherwise we have to figure out a migration strategy for clusters using kubenet/kopeio/gce-networking.

@hakman
Copy link
Member

hakman commented Jan 15, 2021

Yes, I meant the template from containerd conf and what it does behind the scenes.
Let's make this the main topic of today's call

@hakman
Copy link
Member

hakman commented Jan 25, 2021

@justinsb I added #10651 back to the master branch and cherry-picked for 1.19.
I see kopeio networking tests working again:
https://testgrid.k8s.io/kops-network-plugins#kops-aws-cni-kopeio

@k8s-ci-robot
Copy link
Contributor

@justinsb: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2021
@justinsb
Copy link
Member Author

Thanks for fixing @hakman - this is now unneeded

/close

@k8s-ci-robot
Copy link
Contributor

@justinsb: Closed this PR.

In response to this:

Thanks for fixing @hakman - this is now unneeded

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/nodeup cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants