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

Use ip address from CNI output #69821

Open
wants to merge 3 commits into
base: master
from

Conversation

@AlexeyPerevalov
Copy link
Contributor

AlexeyPerevalov commented Oct 15, 2018

Before this patch Kubernetes obtained ip address from container's eth
device. But in case when containers work only with loopback device and
with vhostuser (it's not device it's UNIX domain socket file) Kubernetes
can't rely on this approach, it should rely on CNI output. Most of plugins
provide IP address of created port.

This patch also touches unit test, CNI spec declares ip field as mandatory
field of ADD/GET command output. But some users may relay on current behaviour
so code where ip is empty and obtained from container's eth is preserved,
but it's not covered in unit test.

Signed-off-by: Alexey Perevalov a.perevalov@samsung.com
Fixes #65756

This is a third version of #65759 and #68876
I wasn't able to update my current PR.

Current implementation has functional drawback: if we restart kubelet when pod already started,
pod's IP becames none, after that pod will be restarted, and its IP obtained by CNI ADD method
is correct. But restarting of the pod is not desirable behaviour.

Update: after moving to libcni 0.7.0 we don't have drawback described above!

@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Oct 15, 2018

/sig network

@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Oct 15, 2018

/assign @dcbw

@idealhack

This comment has been minimized.

Copy link
Member

idealhack commented Oct 18, 2018

/kind bug
/release-note-none
/ok-to-test

@PatrickLang

This comment has been minimized.

Copy link
Contributor

PatrickLang commented Oct 18, 2018

@feiskyer will this affect Windows?

@caseydavenport

This comment has been minimized.

Copy link
Member

caseydavenport commented Oct 18, 2018

Current implementation has functional drawback: if we restart kubelet when pod already started,
pod's IP becames none, after that pod will be restarted, and its IP obtained by CNI ADD method
is correct. But restarting of the pod is not desirable behaviour.

I think this is really substantial, and we'll need a solution for this. It's pretty bad if restarting the kubelet means restarting all of the pods on that node.

One option might be to rely on the new CNI CHECK command that was recently introduced to retrieve info for a running pod. However, we'll need to make sure we're still compatible with plugins that implement older versions of the CNI spec for some time because I suspect it will be a while before CHECK implementations become standard.

@bowei

This comment has been minimized.

Copy link
Member

bowei commented Oct 18, 2018

cc: @freehan

@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Oct 19, 2018

Current implementation has functional drawback: if we restart kubelet when pod already started,
pod's IP becames none, after that pod will be restarted, and its IP obtained by CNI ADD method
is correct. But restarting of the pod is not desirable behaviour.

I think this is really substantial, and we'll need a solution for this. It's pretty bad if restarting the kubelet means restarting all of the pods on that node.

One option might be to rely on the new CNI CHECK command that was recently introduced to retrieve info for a running pod. However, we'll need to make sure we're still compatible with plugins that implement older versions of the CNI spec for some time because I suspect it will be a while before CHECK implementations become standard.

One clarification, not all pods are restarting, but only these that doesn't have eth0, because here still nsenter approach is on the stage.
As mentioned @dcbw in previous PR need to get rid of nsenter approach. But in this case pod's ip address should be stored somewhere, it already stored in etcd. So just need to get it from etcd, instead of trying to obtain it from pod at restart time.

@dcbw

This comment has been minimized.

Copy link
Member

dcbw commented Nov 15, 2018

As mentioned @dcbw in previous PR need to get rid of nsenter approach. But in this case pod's ip address should be stored somewhere, it already stored in etcd. So just need to get it from etcd, instead of trying to obtain it from pod at restart time

@AlexeyPerevalov latest libcni has caching ability to store the Result that gets passed back to the CRI on-disk somewhere. It also has a function to grab that back out of the cache, which we anticipated CRI would use on restart when it wanted to get the pod details. Additionally, we are adding a CHECK call that the CRI would call to ensure the container's network was still OK after a restart.

These two things combined should allow kubelet to get the pod network details and verify they are still valid on CRI/kubelet restart. These changes are anticipated to land "soon" (eg matter of weeks).

@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Nov 19, 2018

@dcbw

Oh, great ) I already though how to reuse checkpointing in kubernetes for kubelet restart case.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 17, 2019

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@AlexeyPerevalov AlexeyPerevalov force-pushed the AlexeyPerevalov:GetIPFromCNIOutputMaster branch 2 times, most recently from 23e38e4 to 02b5c48 Sep 6, 2019
@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Sep 7, 2019

/test pull-kubernetes-integration

1 similar comment
@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Sep 16, 2019

/test pull-kubernetes-integration

@AlexeyPerevalov AlexeyPerevalov force-pushed the AlexeyPerevalov:GetIPFromCNIOutputMaster branch from 02b5c48 to 409e579 Sep 20, 2019
@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Sep 20, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@AlexeyPerevalov AlexeyPerevalov force-pushed the AlexeyPerevalov:GetIPFromCNIOutputMaster branch from 409e579 to f0a04a8 Sep 30, 2019
@@ -345,8 +345,30 @@ func (plugin *cniNetworkPlugin) TearDownPod(namespace string, name string, id ku
return plugin.deleteFromNetwork(cniTimeoutCtx, plugin.getDefaultNetwork(), name, namespace, id, netnsPath, nil)
}

func podDesc(namespace, name string, id kubecontainer.ContainerID) string {
return fmt.Sprintf("%s_%s/%s", namespace, name, id.ID)
func getIpsFromResult(res *cnicurrent.Result, ifaceName string) []net.IP {

This comment has been minimized.

Copy link
@johscheuer

johscheuer Oct 1, 2019

Member

Any reason this method isn't exported ? This could be useful for kubenet too: https://github.com/kubernetes/kubernetes/pull/82844/files#r329884258

This comment has been minimized.

Copy link
@johscheuer

johscheuer Oct 1, 2019

Member

I can change this in my PR no need to do this in here. Just a question if there are any reasons not to use this method in kubenet.

This comment has been minimized.

Copy link
@AlexeyPerevalov

AlexeyPerevalov Oct 1, 2019

Author Contributor

@johscheuer, @dcbw I think if function is used by libraries which are on the same level (layer), like cni/kubenet we need to move that function to common library. Is k8s.io/kubernetes/pkg/kubelet/dockershim/network a common library for this case?

This comment has been minimized.

Copy link
@johscheuer

This comment has been minimized.

Copy link
@AlexeyPerevalov

AlexeyPerevalov Oct 3, 2019

Author Contributor

Ok, I can split this commit and add this method there, but tomorrow.

@AlexeyPerevalov AlexeyPerevalov force-pushed the AlexeyPerevalov:GetIPFromCNIOutputMaster branch 2 times, most recently from 7f56f32 to 18205b6 Oct 4, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 4, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AlexeyPerevalov
To complete the pull request process, please assign dcbw
You can assign the PR to them by writing /assign @dcbw in a comment when ready.

The full list of commands accepted by this bot can be found 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

@johscheuer

This comment has been minimized.

Copy link
Member

johscheuer commented Oct 7, 2019

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

@johscheuer

This comment has been minimized.

Copy link
Member

johscheuer commented Oct 7, 2019

/lgtm @dcbw could you also take a look ?

@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Oct 7, 2019

Now I think that containernetworking library is also proper place for GetIpsFromResult

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Oct 9, 2019

/remove-area kubeadm

@AlexeyPerevalov AlexeyPerevalov force-pushed the AlexeyPerevalov:GetIPFromCNIOutputMaster branch from 18205b6 to 4dc210e Oct 31, 2019
@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Nov 1, 2019

/test pull-kubernetes-e2e-gce-storage-slow

@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Nov 1, 2019

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Nov 6, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Nov 6, 2019

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

@danwinship

This comment has been minimized.

Copy link
Contributor

danwinship commented Dec 12, 2019

ping @dcbw

This function returns ip addresses from cni result.
It could be used in cni and kubenet packages.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
Before this patch Kubernetes obtained ip address from container's eth
device. But in case when containers work only with loopback device and
with vhostuser (it's not device it's UNIX domain socket file) Kubernetes
can't rely on this approach, it should rely on CNI output. Most of plugins
provide IP address of created port.

This patch also touches unit test, CNI spec declares ip field as mandatory
field of ADD/GET command output.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
This commit takes into account ipv6 dual stack feature.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
@AlexeyPerevalov AlexeyPerevalov force-pushed the AlexeyPerevalov:GetIPFromCNIOutputMaster branch from 4dc210e to 7de4c2a Dec 30, 2019
@AlexeyPerevalov

This comment has been minimized.

Copy link
Contributor Author

AlexeyPerevalov commented Dec 30, 2019

/test pull-kubernetes-verify

@Vladimare

This comment has been minimized.

Copy link

Vladimare commented Jan 14, 2020

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 14, 2020

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

Test name Commit Details Rerun command
pull-kubernetes-verify 7de4c2a link /test pull-kubernetes-verify

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.