rkt: Generate a new Network Namespace for each Pod #45280
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
This change is |
|
Hi @JulienBalestra. 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 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. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-github-robot
added
size/L
JulienBalestra
changed the title
|
@k8s-bot ok to test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
/assign @yifan-gu |
pkg/kubelet/rkt/rkt.go
Outdated
| // but a pod id. This is because it knows too much about the infra container. | ||
| // We pretend the pod.UID is an infra container ID. | ||
| // This deception is only possible because we played the same trick in | ||
| // Currently the containerID is an UUID for a network namespace |
pkg/kubelet/rkt/rkt.go
Outdated
| // If we are running no-op network plugin, then get the pod IP from the rkt pod status. | ||
| if r.network.PluginName() == network.DefaultPluginName { | ||
| if latestPod != nil { | ||
| if latestPod != nil { |
There was a problem hiding this comment.
Can we change to:
if latestPod == nil {
glog.Warningf()
return podStatus, nil
}So that we don't need the big if block here
pkg/kubelet/rkt/rkt.go
Outdated
| for _, n := range latestPod.Networks { | ||
| if n.Name == defaultNetworkName { | ||
| podStatus.IP = n.Ipv4 | ||
| break | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
nit: Can just return in the above break, so the else can be removed. But it's ok to keep it here as it's not big.
|
LGTM except some small nits. Thanks for the fix @JulienBalestra ! Very appreciate it! |
JulienBalestra
force-pushed
the
run-pod-inside-unique-netns
branch
from
91aa35b
to
ce67897
Compare
|
I made the changes and I squashed them. |
|
@JulienBalestra LGTM, but the CI is failing, might need a rebase? |
JulienBalestra
force-pushed
the
run-pod-inside-unique-netns
branch
from
ce67897
to
7a2e0e2
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JulienBalestra, yifan-gu
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
approved
yifan-gu
added
the
release-note-none
yifan-gu
removed
release-note-label-needed
do-not-merge
|
Automatic merge from submit-queue |
|
Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
What this PR does / why we need it:
This PR concerns the Kubelet with the Container runtime rkt.
Currently, when a Pod stops and the kubelet restart it, the Pod will use the same network namespace based on its PodID.
When the Garbage Collection is triggered, it delete all the old resources and the current network namespace.
The Pods and all containers inside it loose the eth0 interface.
I explained more in details in #45149 how to reproduce this behavior.
This PR generates a new unique network namespace name for each new/restarting Pod.
The Garbage collection retrieve the correct network namespace and remove it safely.
Which issue this PR fixes :
fix #45149
Special notes for your reviewer:
Following @yifan-gu guidelines, so maybe expecting him for the final review.
Release note:
NONE