podIPs order match node IP family preference (Downward API) #103307

aojea · 2021-06-29T19:33:33Z

Following up on #97979,
we should implement the same logic for the exposed PodIPs in the downward API, so
both values are identical.

In addition, it also fixes a bug that causes panic in the kubelet, because the podIPs object was mutated meanwhile the code was parsing the IP addresses.

xref: #102817

Additional information

I've tried different approaches, also to filter directly the podIPs in

kubernetes/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go

Line 231 in 5f8f5ab

    
           func (m *kubeGenericRuntimeManager) determinePodSandboxIPs(podNamespace, podName string, podSandbox *runtimeapi.PodSandboxStatus) []string {

but I've discarded that approach because it has to add a dependeny to the runtimeManager on the kubelet nodeIPs.

Another alternative that I started and stopped because it is toooo disruptive, is to refactor all code to stop plumbing podIP and podIPs

What type of PR is this?

/kind bug
Fixes #103263, #102806

kubelet: the returned value for PodIPs is the same in the Downward API and in the pod.status.PodIPs field

k8s-ci-robot · 2021-06-29T19:33:40Z

@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-06-29T19:34:16Z

/sig network
/sig node
/assign @danwinship @thockin
/cc @ehashman

aojea · 2021-06-29T21:49:08Z

/retest

Kubernetes e2e suite: [sig-node] RuntimeClass should reject a Pod requesting a deleted RuntimeClass [NodeFeature:RuntimeHandler]

aojea · 2021-06-30T10:06:24Z

/priority important-soon

/test pull-kubernetes-integration
TestCertRotationContinuousRequests

aojea · 2021-06-30T13:10:13Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

Kubernetes e2e suite: [sig-storage] CSI Volumes [Driver: csi-hostpath] [Testpattern: Dynamic PV (filesystem volmode)] volumeLimits should verify that all csinodes have volume limits expand_less

pkg/kubelet/kubelet_pods.go

runtimes may return an arbitrary number of Pod IPs, however, kubernetes only takes into consideration the first one of each IP family. The order of the IPs are the one defined by the Kubelet: - default prefer IPv4 - if NodeIPs are defined, matching the first nodeIP family PodIP is always the first IP of PodIPs. The downward API must expose the same IPs and in the same order than the pod.Status API object.

aojea · 2021-07-06T22:24:28Z

pkg/kubelet/kubelet_pods.go

+	}
+
+	// make podIPs order match node IP family preference #97979
+	podIPs = kl.sortPodIPs(podIPs)


is there some subtle golang thing that disallow this?

I don't know - is something not working?

it does work, I just pass the slice as argument and override the same variable with the output, I think is fine, but I just wanted to double check

thockin · 2021-07-06T23:53:30Z

No, that;s fine :)

…

On Tue, Jul 6, 2021 at 4:49 PM Antonio Ojea ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/kubelet/kubelet_pods.go <#103307 (comment)> : > +} + +// convertStatusToAPIStatus creates an api PodStatus for the given pod from +// the given internal pod status. It is purely transformative and does not +// alter the kubelet state at all. +func (kl *Kubelet) convertStatusToAPIStatus(pod *v1.Pod, podStatus *kubecontainer.PodStatus) *v1.PodStatus { + var apiPodStatus v1.PodStatus + + // copy pod status IPs to avoid race conditions with PodStatus #102806 + podIPs := make([]string, len(podStatus.IPs)) + for j, ip := range podStatus.IPs { + podIPs[j] = ip + } + + // make podIPs order match node IP family preference #97979 + podIPs = kl.sortPodIPs(podIPs) it does work, I just pass the slice as argument and override the same variable with the output, I think is fine, but I just wanted to double check — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#103307 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVHT2VFXQSE6ZNRZJ73TWOI7JANCNFSM47Q4EVLQ> .

thockin · 2021-07-06T23:54:30Z

This whole sorting things makes me a little uneasy, not going to lie. But it's already being done elsewhere, so ....

/lgtm
/approve

k8s-ci-robot · 2021-07-06T23:54:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, 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~~ [thockin]

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

aojea · 2021-07-07T09:05:48Z

This whole sorting things makes me a little uneasy, not going to lie. But it's already being done elsewhere, so ....

/lgtm
/approve

I think that this is one of the things that goes to the bucket of "needs a refactor", the podIP and podIPs combo are plumbed in a lot of functions, we no longer need podIP IIUIC .... as I wrote in the description I think that this should be done in the runtime

kubernetes/pkg/kubelet/kuberuntime/kuberuntime_sandbox.go

Line 231 in 5f8f5ab

    
           func (m *kubeGenericRuntimeManager) determinePodSandboxIPs(podNamespace, podName string, podSandbox *runtimeapi.PodSandboxStatus) []string {

but that is a big change , adds a new dependency on the kubelet, and there is a big PR to refactor these code #102344 in flight, 🤷

k8s-ci-robot added do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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 Jun 29, 2021

aojea changed the title ~~podIPs order match node IP family preference~~ podIPs order match node IP family preference (Downward API) Jun 29, 2021

k8s-ci-robot added area/kubelet 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 Jun 29, 2021

k8s-ci-robot assigned danwinship and thockin Jun 29, 2021

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Jun 29, 2021

k8s-ci-robot requested review from ehashman, andrewsykim and dims June 29, 2021 19:34

aojea force-pushed the kubelet_podIPs branch from 6b0ed2b to a833063 Compare June 30, 2021 09:17

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 Jun 30, 2021

thockin reviewed Jul 6, 2021

View reviewed changes

pkg/kubelet/kubelet_pods.go Show resolved Hide resolved

thockin reviewed Jul 6, 2021

View reviewed changes

pkg/kubelet/kubelet_pods.go Outdated Show resolved Hide resolved

aojea force-pushed the kubelet_podIPs branch from a833063 to c34360e Compare July 6, 2021 22:13

aojea force-pushed the kubelet_podIPs branch from c34360e to a7469cf Compare July 6, 2021 22:15

aojea commented Jul 6, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2021

k8s-ci-robot merged commit 2547c5b into kubernetes:master Jul 7, 2021

k8s-ci-robot added this to the v1.22 milestone Jul 7, 2021

aojea mentioned this pull request Jul 7, 2021

kubelet: panic runtime error: invalid memory address or nil pointer dereference in net.IsIPv4String #102806

Closed

hakman mentioned this pull request Oct 2, 2021

Use separate cloud.config file for in-tree vs out-of-tree components kubernetes/kops#12435

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

podIPs order match node IP family preference (Downward API) #103307

podIPs order match node IP family preference (Downward API) #103307

aojea commented Jun 29, 2021 •

edited

Loading

k8s-ci-robot commented Jun 29, 2021

aojea commented Jun 29, 2021

aojea commented Jun 29, 2021

aojea commented Jun 30, 2021

aojea commented Jun 30, 2021

aojea Jul 6, 2021 •

edited

Loading

thockin Jul 6, 2021

aojea Jul 6, 2021

thockin commented Jul 6, 2021 via email

thockin commented Jul 6, 2021

k8s-ci-robot commented Jul 6, 2021

aojea commented Jul 7, 2021

podIPs order match node IP family preference (Downward API) #103307

podIPs order match node IP family preference (Downward API) #103307

Conversation

aojea commented Jun 29, 2021 • edited Loading

Additional information

What type of PR is this?

k8s-ci-robot commented Jun 29, 2021

aojea commented Jun 29, 2021

aojea commented Jun 29, 2021

aojea commented Jun 30, 2021

aojea commented Jun 30, 2021

aojea Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

thockin Jul 6, 2021

Choose a reason for hiding this comment

aojea Jul 6, 2021

Choose a reason for hiding this comment

thockin commented Jul 6, 2021 via email

thockin commented Jul 6, 2021

k8s-ci-robot commented Jul 6, 2021

aojea commented Jul 7, 2021

aojea commented Jun 29, 2021 •

edited

Loading

aojea Jul 6, 2021 •

edited

Loading