podIPs order match node IP family preference (Downward API)

[aojea]

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]

@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]

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

[aojea]

/retest

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

[aojea]

/priority important-soon

/test pull-kubernetes-integration
TestCertRotationContinuousRequests

[aojea]

/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

[aojea]

is there some subtle golang thing that disallow this?

[thockin]

I don't know - is something not working?

[aojea]

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]

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]

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]

[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]

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