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.

Sign up for GitHub

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

Jump to bottom

network/cni: Bring up the lo interface for rkt #29310

Merged
merged 2 commits into from
Jul 25, 2016
Merged

network/cni: Bring up the lo interface for rkt #29310

merged 2 commits into from
Jul 25, 2016

Conversation

euank
Copy link
Contributor

@euank euank commented Jul 20, 2016

This is already done in kubenet. This specifically fixes an issue where a kubelet-managed network for the rkt runtime does not have an "UP" lo interface.

Fixes #28561

If this fix doesn't seem right, it could also be implemented by rkt effectively managing two "cni" network plugins, one for the user requested network, one for lo.

Followup CRs can improve unit testing further and then possibly remove the vendor directory logic (which seems like dead code)

cc @kubernetes/sig-rktnetes @kubernetes/sig-network @dcbw

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jul 20, 2016
@euank
Copy link
Contributor Author

euank commented Jul 20, 2016

I'd also like to suggest this as a cherry-pick candidate and possibly re-title to clarify that it ends up impacting rkt+cni and making it a release note / bugfix.

@euank euank mentioned this pull request Jul 20, 2016
Merged
This was referenced Jul 21, 2016
Closed
Closed
@dcbw
Copy link
Member

dcbw commented Jul 22, 2016

LGTM. Would be useful for openshift/origin#9981 too since I hard-coded the same thing there (though not with the CNI 'lo' plugin).

yifan-gu
yifan-gu reviewed Jul 22, 2016
View reviewed changes
pkg/kubelet/network/cni/cni.go
"name": "cni-loopback",
"type": "loopback"
}`))
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would the error be caused by other things like oom? Though it should be pretty rare.

Copy link
Member

Choose a reason for hiding this comment

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

OOm should not manifest as an error ever.

@yifan-gu yifan-gu added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 22, 2016
@yifan-gu yifan-gu assigned yifan-gu and unassigned vishh Jul 22, 2016
@yifan-gu
Copy link
Contributor

LGTM

euank added 2 commits July 22, 2016 14:40
@euank
network/cni: Always setup 'lo'
8f7b5bc 
See issue kubernetes#28561
@euank
network/cni: Search vendorDir for lo plugin too
6aed2a0 
Though frankly the vendor dir thing should probably be removed in a
followup PR.
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@yifan-gu yifan-gu added this to the v1.3 milestone Jul 23, 2016
@thockin
Copy link
Member

thockin commented Jul 24, 2016

@dcbw you should have enough authority to apply the lgtm label here. You are fully authorized to approve changes like this - you are as much an owner of this code as anyone.

@yifan-gu yifan-gu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2016
@k8s-bot
Copy link

k8s-bot commented Jul 25, 2016

GCE e2e build/test passed for commit 6aed2a0.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jul 25, 2016

GCE e2e build/test passed for commit 6aed2a0.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 4251ebd into kubernetes:master Jul 25, 2016
@euank euank deleted the cni-lo branch July 25, 2016 22:45
peebs pushed a commit to peebs/kubernetes that referenced this pull request Jul 26, 2016
@euank
network/cni: Always setup 'lo'
58903a6 
backport of kubernetes#29310

See issue kubernetes#28561
peebs pushed a commit to peebs/kubernetes that referenced this pull request Jul 26, 2016
@euank
network/cni: Search vendorDir for lo plugin too
285c743 
backport of: kubernetes#29310

Though frankly the vendor dir thing should probably be removed in a
followup PR.
@fabioy fabioy added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 29, 2016
@fabioy
Copy link
Contributor

fabioy commented Jul 29, 2016

@euank Cherrypick approved. Please create a cherrypick PR. Thanks.

@euank euank changed the title network/cni: Unconditionally bring up lo interface network/cni: Bring up the lo interface for rkt Aug 1, 2016
@euank euank mentioned this pull request Aug 1, 2016
Merged
@euank
Copy link
Contributor Author

euank commented Aug 1, 2016 •
edited
Loading

Can we swap the label here to "release-note"? I re-titled it to something that I think accurately conveys the bugfix.

@yifan-gu yifan-gu added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 1, 2016
aaronlevy pushed a commit to aaronlevy/kubernetes that referenced this pull request Aug 1, 2016
@euank @aaronlevy
network/cni: Always setup 'lo'
5bc4bf9 
backport of kubernetes#29310

See issue kubernetes#28561
aaronlevy pushed a commit to aaronlevy/kubernetes that referenced this pull request Aug 1, 2016
@euank @aaronlevy
network/cni: Search vendorDir for lo plugin too
29c3026 
backport of: kubernetes#29310

Though frankly the vendor dir thing should probably be removed in a
followup PR.
k8s-github-robot pushed a commit that referenced this pull request Aug 2, 2016
Merge pull request #29870 from euank/automated-cherry-pick-of-#29310-…
898c648 
…upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of #29310

Cherry pick of #29310 on release-1.3.
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.3" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@hanikesn hanikesn mentioned this pull request Aug 16, 2016
Closed
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#29870 from euank/automated-cherry-pick-…
b319877 
…of-#29310-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of kubernetes#29310

Cherry pick of kubernetes#29310 on release-1.3.
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
Merge pull request kubernetes#29870 from euank/automated-cherry-pick-…
4803711 
…of-#29310-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of kubernetes#29310

Cherry pick of kubernetes#29310 on release-1.3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@yifan-gu yifan-gu

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

rkt: When using network-plugin=cni, loopback interface is not up