set secondary address on host-network pods #106715

aojea · 2021-11-29T14:24:42Z

host-network pods IPs are obtained from the reported kubelet nodeIPs.

Historically, host-network podIPs are immutable once set, but when
we've added dual-stack support, we didn't consider that the secondary
IP address may not be present at the same time that the primary nodeIP.

If a secondary IP address is added to a node after the host-network pods
IPs are set, we can add the secondary host-network pod IP address
maintaining the current behavior of not updating the current podIPs on
host-network pods.

This is an alternative to #105410 ,
without having to change the kubelet and host-network podIPs behavior
xref: kubernetes/enhancements#3000

/kind bug

CRI-API: IPs returned by PodSandboxNetworkStatus are ignored by the kubelet for host-network pods.

Fixes: #105320

k8s-ci-robot · 2021-11-29T14:24:49Z

@aojea: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

aojea · 2021-11-29T14:25:16Z

/assign @danwinship @khenidak @thockin
/cc @hanamantagoudvk

k8s-ci-robot · 2021-11-29T14:25:18Z

@aojea: GitHub didn't allow me to request PR reviews from the following users: hanamantagoudvk.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/assign @danwinship @khenidak @thockin
/cc @hanamantagoudvk

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.

aojea · 2021-11-29T15:09:54Z

pkg/kubelet/kubelet_pods_test.go

@@ -3284,6 +3284,7 @@ func TestGenerateAPIPodStatusHostNetworkPodIPs(t *testing.T) {
 			criPodIPs: []string{"192.168.0.1"},
 			podIPs: []v1.PodIP{
 				{IP: "192.168.0.1"},
+				{IP: "fd01::1234"},


Interesting corner case from DanWinships test cases

name: "CRI PodIPs override NodeAddresses", nodeAddresses: []v1.NodeAddress{ {Type: v1.NodeInternalIP, Address: "10.0.0.1"}, {Type: v1.NodeInternalIP, Address: "fd01::1234"}, }, criPodIPs: []string{"192.168.0.1"}, podIPs: []v1.PodIP{ {IP: "192.168.0.1"}, {IP: "fd01::1234"},

CRIO and containerd doesn't send IPs for host network pods
https://github.com/cri-o/cri-o/blob/8d4d158935929800c4300b82eb4b5a83ded400f4/server/sandbox_network.go#L34-L36

https://github.com/containerd/containerd/blob/4921fb6b63e5476e54fb7e2cf93c3b505f1c09cb/pkg/cri/store/sandbox/sandbox.go#L40-L41

but they can.
I think that the discussion here is if override means removing IPs or just updating the ones that it sets leaving the others as is

I think override means "ignore CRI".

Do we think it is EVER correct for CRI to return host-net IPs?

Do we think it is EVER correct for CRI to return host-net IPs?

I think we all agree that is a bad idea, but it seems that it is the way it was #106715 (comment)

@danwinship what do you think?

If nothing else we should specify what Kubernetes expects host-network pod IPs to be (order, number, type). I don't like leaving that an undefined wild-west.

And I think we should enforce that expectation. If there are corner cases that are currently possible that we break, I think a KEP should get written to implement that corner-case correctly.

Just for reference, the CRI IPs are obtained in kuberuntime in GetPodStatus()

kubernetes/pkg/kubelet/kuberuntime/kuberuntime_manager.go

Line 1064 in 571e067

podIPs = m.determinePodSandboxIPs(namespace, name, podSandboxStatus)

and it seems they are stored in the PLEG cache

kubernetes/pkg/kubelet/pleg/generic.go

Lines 407 to 419 in 571e067

status, err := g.runtime.GetPodStatus(pod.ID, pod.Name, pod.Namespace)

if klog.V(6).Enabled() {

klog.V(6).ErrorS(err, "PLEG: Write status", "pod", klog.KRef(pod.Namespace, pod.Name), "podStatus", status)

} else {

klog.V(4).ErrorS(err, "PLEG: Write status", "pod", klog.KRef(pod.Namespace, pod.Name))

}

if err == nil {

// Preserve the pod IP across cache updates if the new IP is empty.

// When a pod is torn down, kubelet may race with PLEG and retrieve

// a pod status after network teardown, but the kubernetes API expects

// the completed pod's IP to be available after the pod is dead.

status.IPs = g.getPodIPs(pid, status)

}

This cache is used to syncPod() from the PodWorker

kubernetes/pkg/kubelet/pod_workers.go

Line 904 in 571e067

err = p.syncPodFn(ctx, update.Options.UpdateType, pod, update.Options.MirrorPod, status)

and generateAPIPodStatus()

kubernetes/pkg/kubelet/kubelet.go

Line 1554 in 571e067

apiPodStatus := kl.generateAPIPodStatus(pod, podStatus)

it seems there is no way to differentiate between:

the CRI sets the IPs from CRI

CRI does NOT return IPs, but Pod status IPs are updated incrementally,

@danwinship what do you think?

I'm happy to declare that it was a bug that we ever let the CRI set hostNetwork pod IPs, but we should probably update the CRI docs to clarify this if so.

I've updated the commit with the CRI API change

Yes, this sounds good from the CRI API perspective 👍

aojea · 2021-11-29T18:22:10Z

Kubernetes e2e suite: [sig-node] Pods should delete a collection of pods [Conformance] expand_less

/test pull-kubernetes-e2e-kind

danwinship · 2021-12-04T01:48:40Z

we didn't consider that the secondary
IP address may not be present at the same time that the primary nodeIP.

why wouldn't it be? / why wouldn't we expect you to restart kubelet in that case?

CRIO and containerd doesn't send IPs for host network pods
...
but they can.

So, originally, cri-o did send IPs for host network pods, and kubelet just accepted them. But this was a bad idea, because it means that if you wanted to override kubelet's --node-ip, then you probably also had to override cri-o's idea of what the default host IP was or your host-network pods would end up with the wrong IP. Kubelet already had the fallback code to set host-network podIPs itself if the CRI didn't do it, so we just changed cri-o to leave the pod IPs blank for hostNetwork pods. (cri-o/cri-o#2944)

I'm not sure what the historical situation with containerd was.

The unit test is testing that if the CRI does set the pod IPs, that kubelet continues to behave like it did historically, so as to not break CRIs that still do that.

aojea · 2021-12-04T11:02:23Z

why wouldn't it be? / why wouldn't we expect you to restart kubelet in that case?

Because that is the issue reported, and kubelet will race again 😄 #105320

The unit test is testing that if the CRI does set the pod IPs, that kubelet continues to behave like it did historically, so as to not break CRIs that still do that.

agree, that is why I've commented here #106715 (comment)

I think that the discussion here is if override means removing IPs or just updating the ones that it sets leaving the others as is

thockin

overall, I think the simpler and more obvious this is, the better. I like this.

aojea · 2022-01-07T12:28:17Z

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

@@ -445,6 +445,7 @@ message PodIP{
    string ip = 1;
 }
 // PodSandboxNetworkStatus is the status of the network for a PodSandbox.
+// Ignored for pods sharing the host networking namespace.


@mrunalp you can find the context in this thread https://github.com/kubernetes/kubernetes/pull/106715/files#r758454031

aojea · 2022-01-13T08:05:40Z

@thockin @danwinship I've updated the CRI API , can you please take another look?

danwinship · 2022-01-17T14:38:52Z

pkg/kubelet/kubelet_pods.go

+			// If the Node changes its IPs the user has to restart the Kubelet
+			// so the HostNetwork pods update their IPs. We may change this in a future
+			// https://github.com/kubernetes/enhancements/pull/3000
+			// but since it changes the current behavior "here be dragons"


I'm not sure you need to explain that much. Also, you spend several sentences explaining that podIPs is immutable, and then add code that changes it.

// HostNetwork Pods inherit the node IPs as PodIPs. They are immutable once set, // other than that if the node becomes dual-stack, we add the secondary IP.

(though probably we should remove the second IP on dual-stack-to-single-stack downgrade too)

yeah, I see my gap on the language, I try to mean that once a PodIP exist that never changes, however, you can add a new IP is not the array that is immutable (it can grow), is the element in the array that can't change once set

(though probably we should remove the second IP on dual-stack-to-single-stack downgrade too)

this can open a new set of corner cases and we don't have a good history here, per example, I think that all the dual stack services that are stored will fail in validation on a migration like this.

danwinship · 2022-01-17T14:45:39Z

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

@@ -445,6 +445,7 @@ message PodIP{
    string ip = 1;
 }
 // PodSandboxNetworkStatus is the status of the network for a PodSandbox.
+// Ignored for pods sharing the host networking namespace.


Is there a chance we might add new fields in the future that it wouldn't make sense to ignore for hostNetwork pods? Maybe hedge a little bit here and prepend "Currently" ?

great point

host-network pods IPs are obtained from the reported kubelet nodeIPs. Historically, host-network podIPs are immutable once set, but when we've added dual-stack support, we didn't consider that the secondary IP address may not be present at the same time that the primary nodeIP. If a secondary IP address is added to a node after the host-network pods IPs are set, we can add the secondary host-network pod IP address maintaining the current behavior of not updating the current podIPs on host-network pods.

danwinship · 2022-01-17T20:57:52Z

/lgtm

thockin

/approve

thockin · 2022-01-25T19:39:03Z

pkg/kubelet/kubelet_pods.go

-				s.PodIPs = []v1.PodIP{{IP: s.PodIP}}
-				if len(hostIPs) == 2 {
+			// HostNetwork Pods inherit the node IPs as PodIPs. They are immutable once set,
+			// other than that if the node becomes dual-stack, we add the secondary IP.


Do we also needto downgrade? I feel like eventually we'll have to revisit this...

#106715 (comment)

aojea · 2022-01-25T22:26:22Z

/assign @mrunalp
for the CRI API doc update

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

aojea · 2022-02-04T17:44:40Z

updated the CRI-API godoc on v1 and v1alpha2 too

mrunalp · 2022-02-04T17:46:46Z

/approve

thockin · 2022-02-04T17:54:58Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2022-02-04T17:55:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, mrunalp, thockin

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/OWNERS~~ [mrunalp,thockin]
~~staging/src/k8s.io/cri-api/pkg/OWNERS~~ [mrunalp]

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

…-of-#106715-upstream-release-1.23 Automated cherry pick of #106715: set secondary address on host-network pods

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 29, 2021

k8s-ci-robot requested review from derekwaynecarr and ehashman November 29, 2021 14:25

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 29, 2021

k8s-ci-robot assigned danwinship, khenidak and thockin Nov 29, 2021

aojea commented Nov 29, 2021

View reviewed changes

ehashman added this to Triage in SIG Node PR Triage Nov 29, 2021

thockin reviewed Dec 17, 2021

View reviewed changes

aojea force-pushed the dual_hostnet_pods branch from a9c536e to ae8c0e3 Compare January 7, 2022 12:22

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 7, 2022

aojea force-pushed the dual_hostnet_pods branch from ae8c0e3 to 50af04d Compare January 7, 2022 12:26

aojea commented Jan 7, 2022

View reviewed changes

aojea force-pushed the dual_hostnet_pods branch from 50af04d to ae64f5f Compare January 7, 2022 21:13

danwinship reviewed Jan 17, 2022

View reviewed changes

aojea force-pushed the dual_hostnet_pods branch from ae64f5f to c516320 Compare January 17, 2022 17:25

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

aojea mentioned this pull request Jan 25, 2022

KEP 2999: adding new KEP for host network pod IPs kubernetes/enhancements#3000

Closed

thockin reviewed Jan 25, 2022

View reviewed changes

k8s-ci-robot assigned mrunalp Jan 25, 2022

mrunalp reviewed Feb 4, 2022

View reviewed changes

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto Show resolved Hide resolved

ignore CRI PodSandboxNetworkStatus for host network pods

bc8e7ac

aojea force-pushed the dual_hostnet_pods branch from c516320 to bc8e7ac Compare February 4, 2022 17:42

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 4, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2022

k8s-ci-robot merged commit 469c4c4 into kubernetes:master Feb 4, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 4, 2022

hanamantagoudvk mentioned this pull request Feb 7, 2022

Automated cherry pick of #106715: set secondary address on host-network pods #107988

Merged

k8s-ci-robot added a commit that referenced this pull request Feb 10, 2022

Merge pull request #107988 from hanamantagoudvk/automated-cherry-pick…

7ede5de

…-of-#106715-upstream-release-1.23 Automated cherry pick of #106715: set secondary address on host-network pods

danwinship mentioned this pull request Mar 27, 2023

[Dual-stack] Fix generateAPIPodStatus() of kubelet handling Secondary IP #116879

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set secondary address on host-network pods #106715

set secondary address on host-network pods #106715

aojea commented Nov 29, 2021 •

edited

k8s-ci-robot commented Nov 29, 2021

aojea commented Nov 29, 2021

k8s-ci-robot commented Nov 29, 2021

aojea Nov 29, 2021

thockin Dec 17, 2021

aojea Dec 17, 2021

aojea Dec 20, 2021

dcbw Dec 20, 2021

aojea Dec 20, 2021

aojea Dec 20, 2021 •

edited

danwinship Jan 5, 2022

aojea Jan 7, 2022

mrunalp Feb 4, 2022

aojea commented Nov 29, 2021

danwinship commented Dec 4, 2021 •

edited

aojea commented Dec 4, 2021

thockin left a comment

aojea Jan 7, 2022

aojea commented Jan 13, 2022

danwinship Jan 17, 2022

aojea Jan 17, 2022 •

edited

aojea Jan 17, 2022

danwinship Jan 17, 2022

aojea Jan 17, 2022

danwinship commented Jan 17, 2022

thockin left a comment

thockin Jan 25, 2022

aojea Jan 25, 2022

aojea commented Jan 25, 2022

aojea commented Feb 4, 2022

mrunalp commented Feb 4, 2022

thockin commented Feb 4, 2022

k8s-ci-robot commented Feb 4, 2022

	status, err := g.runtime.GetPodStatus(pod.ID, pod.Name, pod.Namespace)
	if klog.V(6).Enabled() {
	klog.V(6).ErrorS(err, "PLEG: Write status", "pod", klog.KRef(pod.Namespace, pod.Name), "podStatus", status)
	} else {
	klog.V(4).ErrorS(err, "PLEG: Write status", "pod", klog.KRef(pod.Namespace, pod.Name))
	}
	if err == nil {
	// Preserve the pod IP across cache updates if the new IP is empty.
	// When a pod is torn down, kubelet may race with PLEG and retrieve
	// a pod status after network teardown, but the kubernetes API expects
	// the completed pod's IP to be available after the pod is dead.
	status.IPs = g.getPodIPs(pid, status)
	}

set secondary address on host-network pods #106715

set secondary address on host-network pods #106715

Conversation

aojea commented Nov 29, 2021 • edited

k8s-ci-robot commented Nov 29, 2021

aojea commented Nov 29, 2021

k8s-ci-robot commented Nov 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea Dec 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Nov 29, 2021

danwinship commented Dec 4, 2021 • edited

aojea commented Dec 4, 2021

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jan 13, 2022

Choose a reason for hiding this comment

aojea Jan 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Jan 17, 2022

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jan 25, 2022

aojea commented Feb 4, 2022

mrunalp commented Feb 4, 2022

thockin commented Feb 4, 2022

k8s-ci-robot commented Feb 4, 2022

aojea commented Nov 29, 2021 •

edited

aojea Dec 20, 2021 •

edited

danwinship commented Dec 4, 2021 •

edited

aojea Jan 17, 2022 •

edited