Separate Direct and Indirect streaming paths, implement indirect path for CRI #36020

timstclair · 2016-11-01T23:05:10Z

This PR refactors the pkg/kubelet/container.Runtime interface to remove the ExecInContainer, PortForward and AttachContainer methods. Instead, those methods are part of the DirectStreamingRuntime interface which all "legacy" runtimes implement. I also added an IndirectStreamingRuntime which handles the redirect path and is implemented by CRI runtimes. To control the size of this PR, I did not fully setup the indirect streaming path for the dockershim, so I left legacy path behind.

Most of this PR is moving & renaming associated with the refactoring. To understand the functional changes, I suggest tracing the code from getExec in pkg/kubelet/server/server.go, which calls GetExec in pkg/kubelet/kubelet_pods.go to determine whether to follow the direct or indirect path.

For #29579

/cc @kubernetes/sig-node

This change is

yujuhong · 2016-11-02T00:20:01Z

Reviewed 11 of 20 files at r1.
Review status: 11 of 26 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

pkg/kubelet/kubelet_pods.go, line 1134 at r1 (raw file):

      return nil, err
  }
  podUID = kl.podManager.TranslatePodUID(podUID)

I'm fine with moving the function call here, but we should be careful and make sure that all outward facing methods call this function. Ideally we should just use a wrapper for all host method to ensure translation is done. This is out of the scope for the current PR though.

pkg/kubelet/container/runtime.go, line 124 at r1 (raw file):

}

type DirectStreamingRuntime interface {

Add comment for the interface.

pkg/kubelet/container/runtime.go, line 135 at r1 (raw file):

}

type IndirectStreamingRuntime interface {

Add comment for the interface.

pkg/kubelet/container/runtime.go, line 138 at r1 (raw file):

  GetExec(id ContainerID, cmd []string, stdin, stdout, stderr, tty bool) (*url.URL, error)
  GetAttach(id ContainerID, stdin, stdout, stderr bool) (*url.URL, error)
  GetPortForward(podFullName string, podUID types.UID) (*url.URL, error)

Can we not use podFullName? It's not very well-defined. "UID" or "name+namespace" is preferred. e

pkg/kubelet/dockertools/docker_manager.go, line 267 at r1 (raw file):

      imageStatsProvider:     newImageStatsProvider(client),
      seccompProfileRoot:     seccompProfileRoot,
  }

Need to ensure that docker manager implements DirectStreamingRuntime now that it's not part of the Runtime interface.
Add var _ kubecontainer.DirectStreamingRuntime = &DockerManager{} to the beginning of the file.

pkg/kubelet/kuberuntime/kuberuntime_manager.go, line 116 at r1 (raw file):

  kubecontainer.Runtime
  kubecontainer.IndirectStreamingRuntime
  kubecontainer.ContainerCommandRunner

include kubecontainer.DirectStreamingRuntime since we still keep the old methods.

pkg/kubelet/rkt/rkt.go, line 278 at r1 (raw file):

      return nil, fmt.Errorf("rkt: cannot get config from rkt api service: %v", err)
  }

Need to ensure that rkt implements DirectStreamingRuntime now that it's not part of the Runtime interface.
Add var _ kubecontainer.DirectStreamingRuntime = &Runtime{} to the beginning of the file.

Comments from Reviewable

yujuhong · 2016-11-02T00:23:34Z

Made the first pass. Most changes seem reasonable/straightforward. The cleanup of RunInContainer is nice. Will take a closer look later. Look forward to the next PR where legacy methods get deleted.

timstclair · 2016-11-02T02:08:03Z

Thanks, addressed all comments.

Review status: 11 of 26 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

pkg/kubelet/kubelet_pods.go, line 1134 at r1 (raw file):