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

Specify Pod default network in Annotations #174

Merged
merged 3 commits into from
Nov 30, 2018

Conversation

pliurh
Copy link
Contributor

@pliurh pliurh commented Oct 22, 2018

Allow pod to specify cluster default network in annotations, When more than one cluster network is available.
Create a new field 'k8s.v1.cni.cncf.io/default-network' in pod annotations, which is used to overwrite the cluster default network defined in Multus config file.

Fix #158

@pliurh pliurh closed this Oct 22, 2018
@pliurh pliurh reopened this Oct 22, 2018
@coveralls
Copy link

coveralls commented Oct 22, 2018

Pull Request Test Coverage Report for Build 524

  • 29 of 45 (64.44%) changed or added relevant lines in 1 file are covered.
  • 109 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+2.7%) to 52.152%

Changes Missing Coverage Covered Lines Changed/Added Lines %
k8sclient/k8sclient.go 29 45 64.44%
Files with Coverage Reduction New Missed Lines %
types/conf.go 32 27.93%
multus/multus.go 77 54.36%
Totals Coverage Status
Change from base Build 495: 2.7%
Covered Lines: 630
Relevant Lines: 1208

💛 - Coveralls

@kannanvr
Copy link

@rkamudhan @dougbtv Request you to merge this PR as early as possible. This PR would solve our use case. FOllowing is our Use case.
Multiple tenants in networks - where every tenants / NS will be configured with a different subnet and any pod in a NS will have the IPAddr from that configured subnet and pods across Namespaces should not connect to each other. There is no management IP / subnet concept

k8sclient/k8sclient.go Outdated Show resolved Hide resolved
Signed-off-by: Peng Liu <pliu@redhat.com>
@pliurh pliurh changed the title [WIP]Specify Pod default network in Annotations Specify Pod default network in Annotations Nov 19, 2018
@pliurh
Copy link
Contributor Author

pliurh commented Nov 19, 2018

@s1061123 @rkamudhan @dougbtv could you please review it? Thanks.

k8sclient/k8sclient.go Outdated Show resolved Hide resolved
k8sclient/k8sclient.go Outdated Show resolved Hide resolved
k8sclient/k8sclient_test.go Show resolved Hide resolved
k8sclient/k8sclient.go Outdated Show resolved Hide resolved
k8sclient/k8sclient.go Outdated Show resolved Hide resolved
k8sclient/k8sclient.go Show resolved Hide resolved
k8sclient/k8sclient.go Outdated Show resolved Hide resolved
greg-bock pushed a commit to greg-bock/kubevirt that referenced this pull request Nov 28, 2018
Copy link
Member

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

Could you change "multus-cni.io/default-network" to "multus-cni.io/v1/default-network"?
Once it fixed we merge it! Thank you for your cooperation!

Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

I'm OK with moving forward on this one, there's no rush and it shouldn't block the merge, but... Let's follow up with some documentation -- one that includes a big warning for usage. We can also understate it in the documentation -- as to not encourage people to use this functionality for non-standard use-cases. I created the issue in #200. Again, no big priority on the documentation, but, should be a follow up at some point. Thanks!

@joshlreese
Copy link

We've been keeping up with this PR and have been testing the changes as they've come through. The recommendation of using multus-cni.io/v1/default-network makes sense, but is not a qualified name, which k8s requires here. Perhaps something along the lines of v1.multus-cni.io/default-network (similar to k8s.v1.cni.cncf.io) would work.

Below is the error encountered when attempting to use the recommended annotation name:

Error creating pod: Pod "test" is invalid: metadata.annotations: Invalid value: "multus-cni.io/v1/default-network": a qualified name must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')

@pliurh
Copy link
Contributor Author

pliurh commented Nov 30, 2018

@dougbtv, thanks for reminding. I'll prepare the document in another patch.

Copy link
Member

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for your work!

@s1061123 s1061123 merged commit 96217dd into k8snetworkplumbingwg:master Nov 30, 2018
greg-bock pushed a commit to greg-bock/kubevirt that referenced this pull request Nov 30, 2018
greg-bock pushed a commit to greg-bock/kubevirt that referenced this pull request Dec 3, 2018
greg-bock pushed a commit to StackPathABC/kubevirt2 that referenced this pull request Dec 4, 2018
Signed-off-by: Greg Bock <greg.bock@stackpath.com>
Co-Authored-By: Joshua Reese <joshua.reese@stackpath.com>
@dougbtv dougbtv added the release-v3 PRs to make it in release branch label Dec 19, 2018
greg-bock pushed a commit to StackPathABC/kubevirt2 that referenced this pull request Jan 10, 2019
Signed-off-by: Greg Bock <greg.bock@stackpath.com>
Co-Authored-By: Joshua Reese <joshua.reese@stackpath.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog release-v3 PRs to make it in release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants