Add port forwarding for rkt with kvm stage1 #32126
Conversation
|
Can a kubernetes member verify that this patch is reasonable to test? If so, please reply "ok to test". Regular contributors should join the org to skip this step. |
k8s-github-robot
added
size/S
|
We should also document that there's a slight behavioural difference here in that Correct me if I'm wrong ofcourse. |
| return fmt.Errorf("unable to do port forwarding: nsenter not found.") | ||
| var args []string | ||
| var fwCaller string | ||
| if strings.Contains(r.config.Stage1Image, "kvm") { |
There was a problem hiding this comment.
This won't work in the case that the alpha "stage1 override" annotation is used.
Does the rkt api-service return what stage1 a pod is using? If it doesn't, it probably should.
I'd also slightly prefer checking for a more or less exact match, not just string.Contains for this.
There was a problem hiding this comment.
@euank api service doesn't return the stage1 info today unfortunately...
@jjlakis To workaround this, we can do something like https://github.com/kubernetes/kubernetes/blob/release-1.4/pkg/kubelet/rkt/rkt.go#L1043-L1053
There was a problem hiding this comment.
@yifan-gu As far as I see, I am unable to get pod manifest in this scope. All pod information I got is kubecontainer.pod, or at most manifest from api-service. Am I wrong?
There was a problem hiding this comment.
@jjlakis manifest from api-service is an marshaled pod manifest. We can unmarshal it into a pod manifest.
There was a problem hiding this comment.
@jjlakis But we don't need to do that, the pod returned by api service has the annotation. So we can combine the annotation and the runtime.Config to check the stage1 image.
yifan-gu
added
release-note
k8s-github-robot
added
size/M
|
|
||
| socatPath, lookupErr := exec.LookPath("socat") | ||
| if lookupErr != nil { | ||
| return fmt.Errorf("unable to do port forwarding: socat not found.") | ||
| } | ||
|
|
||
| args := []string{"-t", fmt.Sprintf("%d", listResp.Pods[0].Pid), "-n", socatPath, "-", fmt.Sprintf("TCP4:localhost:%d", port)} | ||
| // Check in config and in annotations if we're running kvm flavor | ||
| isKvm := strings.Contains(r.config.Stage1Image, "kvm") |
There was a problem hiding this comment.
This breaks in the reverse now; if you have stage1-image=kvm and then have the annotation for non-kvm, isKvm still remains true.
There was a problem hiding this comment.
@euank I don't see any mistake, there's a loop below overriding isKvm if there's another image in annotations
|
PTAL @pskrzyns |
|
LGTM, leave @pskrzyns for the final review. |
|
LGTM We are OK with issue that it will work only for applications listening on |
yifan-gu
added
lgtm
|
Jenkins GCE e2e failed for commit 3a557c6. The magic incantation to run this job again is |
|
@k8s-bot test this please , issue #IGNORE |
|
@k8s-bot gce e2e test this |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
Port forwarding for rkt kvm using
socat.cc @yifan-gu @euank @pskrzyns @lukasredynk
This change is