kubenet: replace gateway with cni result #85993

chendotjs · 2019-12-06T09:03:42Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Remove the tricky method that kubenet uses to calculate gateway from pod cidr. Instead，kubenet will fetch gateway from cni result since host-localhas the ability to conclude an appropriate gateway for a subnet as @mars1024 methioned in #84550.

Which issue(s) this PR fixes:

Fixes #84541

Special notes for your reviewer:

Thanks for review.

Does this PR introduce a user-facing change?:

fix a regression in kubenet that prevent pods to obtain ip addresses

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2019-12-06T09:03:51Z

Hi @chendotjs. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

chendotjs · 2019-12-06T09:11:50Z

/priority important-soon
/cc @khenidak @mars1024 @aojea

I'll keep on adding tests if removing gateway generation is accepted

chendotjs · 2019-12-10T09:16:55Z

ping @mars1024
ptal

mars1024

Sorry for the late reply, I think this almost LGTM except some tests :)

mars1024 · 2019-12-10T11:42:31Z

/ok-to-test

pkg/kubelet/dockershim/network/kubenet/kubenet_linux.go

@@ -336,6 +332,9 @@ func (plugin *kubenetNetworkPlugin) Capabilities() utilsets.Int {
 // setup sets up networking through CNI using the given ns/name and sandbox ID.
 func (plugin *kubenetNetworkPlugin) setup(namespace string, name string, id kubecontainer.ContainerID, annotations map[string]string) error {
 	var ipv4, ipv6 net.IP
+	var podGateways []net.IP
+	var podCIDRs []net.IPNet


chendotjs · 2019-12-19T09:28:33Z

@mars1024 @aojea
tests have been updated. would you please take another look? thanks for review :)

aojea · 2019-12-19T09:41:33Z

I suggest to squash the commits before merge, anyway it LGTM, but defer to @mars1024 the final lgtm

chendotjs · 2019-12-19T10:36:28Z

@aojea thanks for your relpy, the commits are squashed.

squeed · 2019-12-19T10:57:35Z

I think this is missing something: you still need to keep plugin.podGateways, because that's where we gather state (though it's still wrong, because it doesn't survive reboots... sigh.

chendotjs · 2019-12-19T13:22:08Z

I'm afraid I didn't catch your meaning of gathering state from plugin.podGateways, could you explain a little bit more? @squeed Thanks in advance.
PS: PR #84550 still keeps plugin.podGateways.

dcbw · 2020-01-09T21:40:06Z

I think this is missing something: you still need to keep plugin.podGateways, because that's where we gather state (though it's still wrong, because it doesn't survive reboots... sigh.

@squeed I don't know what state gets kept in there. podGateways doesn't seem to be used for anything other than creating the host-local ranges, which doesn't need state to be kept.

dcbw · 2020-01-09T21:40:12Z

/lgtm
/approve

k8s-ci-robot · 2020-01-09T21:40:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chendotjs, dcbw

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:

~~pkg/kubelet/dockershim/network/OWNERS~~ [dcbw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dcbw · 2020-01-09T21:55:12Z

/test pull-kubernetes-e2e-gce-100-performance

fejta-bot · 2020-01-10T01:01:25Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

baranov1ch · 2020-01-16T15:30:43Z

shouldn't it be cherry-picked to 1.16 and 1.17 ?

aojea · 2020-01-16T16:30:49Z

@chendotjs do you mind adding a release note?
I've added this to the cherry picks

fix a regression in kubenet  that  prevent pods to obtain ip addresses

chendotjs · 2020-01-18T03:22:48Z

@aojea thanks for reminding，release note updated 😃

…upstream-release-1.17 Automated cherry pick of #85993: kubenet: replace gateway with cni result

…upstream-release-1.16 Automated cherry pick of #85993: kubenet: replace gateway with cni result

k8s-ci-robot requested review from danwinship and freehan December 6, 2019 09:04

k8s-ci-robot added area/kubelet sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 6, 2019

k8s-ci-robot requested review from aojea, khenidak and mars1024 December 6, 2019 09:11

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 6, 2019

chendotjs changed the title ~~[WIP]: kubenet: replace gateway with cni result~~ kubenet: replace gateway with cni result Dec 6, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2019

mars1024 reviewed Dec 10, 2019

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 10, 2019

aojea mentioned this pull request Dec 10, 2019

kubenet: use subnet address for cni #84550

Closed

aojea reviewed Dec 10, 2019

View reviewed changes

thockin assigned dcbw Dec 17, 2019

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 19, 2019

kubenet: replace gateway with cni result

c5002a3

chendotjs force-pushed the fix-cidr branch from 44deecd to c5002a3 Compare December 19, 2019 10:34

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2020

k8s-ci-robot merged commit 7a50fdb into kubernetes:master Jan 10, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 10, 2020

chendotjs deleted the fix-cidr branch January 12, 2020 13:43

This was referenced Jan 16, 2020

Automated cherry pick of #85993: kubenet: replace gateway with cni result #87286

Merged

Automated cherry pick of #85993: kubenet: replace gateway with cni result #87287

Merged

k8s-ci-robot 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 Jan 18, 2020

MorrisLaw mentioned this pull request Jan 28, 2020

Document PR #85993 (kubenet: replace gateway with cni result) contributing-to-kubernetes/gnosis#21

Closed

k8s-ci-robot added a commit that referenced this pull request Feb 1, 2020

Merge pull request #87286 from aojea/automated-cherry-pick-of-#85993-…

d97211f

…upstream-release-1.17 Automated cherry pick of #85993: kubenet: replace gateway with cni result

k8s-ci-robot added a commit that referenced this pull request Feb 21, 2020

Merge pull request #87287 from aojea/automated-cherry-pick-of-#85993-…

32a8be1

…upstream-release-1.16 Automated cherry pick of #85993: kubenet: replace gateway with cni result

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubenet: replace gateway with cni result #85993

kubenet: replace gateway with cni result #85993

chendotjs commented Dec 6, 2019 •

edited

k8s-ci-robot commented Dec 6, 2019

chendotjs commented Dec 6, 2019

chendotjs commented Dec 10, 2019

mars1024 left a comment

mars1024 commented Dec 10, 2019

This comment was marked as resolved.

chendotjs commented Dec 19, 2019

aojea commented Dec 19, 2019

chendotjs commented Dec 19, 2019

squeed commented Dec 19, 2019

chendotjs commented Dec 19, 2019

dcbw commented Jan 9, 2020

dcbw commented Jan 9, 2020

k8s-ci-robot commented Jan 9, 2020

dcbw commented Jan 9, 2020

fejta-bot commented Jan 10, 2020

baranov1ch commented Jan 16, 2020

aojea commented Jan 16, 2020 •

edited

chendotjs commented Jan 18, 2020

kubenet: replace gateway with cni result #85993

kubenet: replace gateway with cni result #85993

Conversation

chendotjs commented Dec 6, 2019 • edited

k8s-ci-robot commented Dec 6, 2019

chendotjs commented Dec 6, 2019

chendotjs commented Dec 10, 2019

mars1024 left a comment

Choose a reason for hiding this comment

mars1024 commented Dec 10, 2019

This comment was marked as resolved.

chendotjs commented Dec 19, 2019

aojea commented Dec 19, 2019

chendotjs commented Dec 19, 2019

squeed commented Dec 19, 2019

chendotjs commented Dec 19, 2019

dcbw commented Jan 9, 2020

dcbw commented Jan 9, 2020

k8s-ci-robot commented Jan 9, 2020

dcbw commented Jan 9, 2020

fejta-bot commented Jan 10, 2020

baranov1ch commented Jan 16, 2020

aojea commented Jan 16, 2020 • edited

chendotjs commented Jan 18, 2020

chendotjs commented Dec 6, 2019 •

edited

aojea commented Jan 16, 2020 •

edited