Decouple remotecommand #41543

dshulyak · 2017-02-16T08:57:36Z

Refactored unversioned/remotecommand to decouple it from undesirable dependencies:

term package now is not required, and functionality required to resize terminal size can be plugged in directly in kubectl
in order to remove dependency on kubelet package - constants from kubelet/server/remotecommand were moved to separate util package (pkg/util/remotecommand)
remotecommand_test.go moved to pkg/client/tests module

k8s-reviewable · 2017-02-16T08:57:42Z

This change is

dshulyak · 2017-02-16T09:01:24Z

@sttts @caesarxuchao @deads2k I moved changes related to remotecommand refactoring into separate PR, perhaps it will be easier for you to review it this way

caesarxuchao · 2017-04-04T01:03:26Z

pkg/client/unversioned/remotecommand/resize/resize.go

+	"io"
+
+	"k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/kubernetes/pkg/util/term"


This way term is still depended by the package remotecommand. Can we move the definition of Size and TerminalSizeQueue to the package remotecommand to reverse the dependency direction?

caesarxuchao · 2017-04-04T01:05:56Z

pkg/util/remotecommand/constants.go

+limitations under the License.
+*/
+
+package remotecommand


I think you mentioned somewhere that these constants belonged to apimachinery, which I agree. pkg/util/remotecommand/constants.go seems the wrong place. cc @sttts @lavalamp

caesarxuchao · 2017-04-04T01:07:54Z

@dshulyak i left two comments, otherwise looks ok.

dshulyak · 2017-04-05T13:03:24Z

@caesarxuchao I addressed all issues, please take when you will have time

caesarxuchao

Generally looks good. A few nits.

caesarxuchao · 2017-04-05T22:15:59Z

pkg/client/tests/remotecommand_test.go

@@ -221,7 +221,7 @@ func TestStream(t *testing.T) {
 			url, _ := url.ParseRequestURI(server.URL)
 			config := restclient.ContentConfig{
 				GroupVersion:         &schema.GroupVersion{Group: "x"},
-				NegotiatedSerializer: testapi.Default.NegotiatedSerializer(),
+				NegotiatedSerializer: api.Codecs,


Why is this change necessary?

I know testapi is a mess, but api.Codecs is a global variable we want to get rid of, every new use of it increases our tech debt.

cant recally why i had to change it, will revert

caesarxuchao · 2017-04-05T22:20:10Z

pkg/client/unversioned/remotecommand/resize.go

+}
+
+// GetResizeFunc will return function that handles terminal resize
+func GetResizeFunc(resizeQueue TerminalSizeQueue) func(io.Writer) {


It looks unnecessary to abstract the anonymous func to the GetResizeFunc. Actually I found the original code to be easier to read. Could you revert this part?

ah right, it was needed when resize was separate package and TerminalSizeQueue was a part of term package

caesarxuchao · 2017-04-05T22:25:35Z

staging/src/k8s.io/apimachinery/pkg/util/remotecommand/constants.go

@@ -41,6 +45,9 @@ const (
 	// attachment/execution. It is the 4th version of the subprotocol and
 	// adds support for exit codes.
 	StreamProtocolV4Name = "v4.channel.k8s.io"
+
+	NonZeroExitCodeReason = metav1.StatusReason("NonZeroExitCode")
+	ExitCodeCauseType     = metav1.CauseType("ExitCode")


Where do these two lines come from?

they were defined in pkg/kubelet/server/remotecommand/exec.go. I had to move them as well cause they are used by client/remotecommand

caesarxuchao · 2017-04-05T22:28:58Z

pkg/client/unversioned/remotecommand/resize.go

+)
+
+// Size represents the width and height of a terminal.
+type Size struct {


@sttts are you ok with moving definition of Size and TerminalSizeQueue to client-go/tool/remotecommand? We did the move to break the dependency on pkg/util/terms.

caesarxuchao · 2017-04-05T22:34:38Z

staging/src/k8s.io/apimachinery/pkg/util/remotecommand/constants.go

+	"time"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)

 const (
 	DefaultStreamCreationTimeout = 30 * time.Second


@sttts @deads2k are you ok with moving these constants to apimachinery? @dshulyak seemed to be following @deads2k's suggestion (#41331 (comment)).

caesarxuchao · 2017-04-06T17:16:25Z

/lgtm

caesarxuchao · 2017-04-06T17:16:29Z

/approve

caesarxuchao · 2017-04-06T17:18:37Z

@lavalamp could you approve the change? It's a step required to support exec, attach, port-forward in client-go. Thanks.

caesarxuchao · 2017-04-06T17:18:54Z

Thank you, @dshulyak

caesarxuchao · 2017-04-07T17:22:30Z

@lavalamp could you approve this PR? Thanks.

lavalamp

Sorry, I have a few change requests. Also I think many of the commits in this PR should be squashed.

lavalamp · 2017-04-12T22:13:20Z

pkg/kubelet/server/server.go

@@ -41,6 +41,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	consts "k8s.io/apimachinery/pkg/util/remotecommand"


Don't call this "consts", it's very confusing. (Maybe make a remotecommand/consts package if you really want to, but I don't think that's necessary.)

Multiple places.

lavalamp · 2017-04-12T22:15:44Z

pkg/client/unversioned/remotecommand/resize.go

+package remotecommand
+
+// Size represents the width and height of a terminal.
+type Size struct {


If you move this out of the term package, you need to change the name to TerminalSize, and probably explain why it's needed in the comment.

lavalamp · 2017-04-12T22:17:28Z

staging/src/k8s.io/apimachinery/pkg/util/remotecommand/constants.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2016 The Kubernetes Authors.
+Copyright 2017 The Kubernetes Authors.


Do not change years in headers.

lavalamp · 2017-04-12T22:19:43Z

pkg/kubelet/server/server.go

 	"k8s.io/kubernetes/pkg/kubelet/cm"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
 	"k8s.io/kubernetes/pkg/kubelet/server/portforward"
-	"k8s.io/kubernetes/pkg/kubelet/server/remotecommand"
+	remotecommandserver "k8s.io/kubernetes/pkg/kubelet/server/remotecommand"


I guess I would have expected remotecommand/server, remotecommand/client, and maybe remotecommand/consts (or /public), none of those being under the kubelet package. Not asking for a change in this PR, just noting.

In order to move client/unversioned/remotecommand to client-go as a followup for this change we have to decouple it from tons of dependencies

lavalamp · 2017-04-13T20:43:05Z

/lgtm
/approve

k8s-github-robot · 2017-04-13T20:43:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, dshulyak, lavalamp

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [lavalamp]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-04-14T02:52:04Z

Automatic merge from submit-queue (batch tested with PRs 44406, 41543, 44071, 44374, 44299)

ixdy · 2017-04-14T18:23:31Z

This PR appears to have broken the crossbuild:

I0414 11:07:29.245] # k8s.io/kubernetes/pkg/util/term
I0414 11:07:29.245] pkg/util/term/resizeevents_windows.go:28: undefined: Size
I0414 11:07:29.245] pkg/util/term/setsize_unsupported.go:21: undefined: Size

caesarxuchao · 2017-04-14T18:31:53Z

That definition of Size has been moved to :https://github.com/kubernetes/kubernetes/pull/41543/files#diff-a730f0242b04da050b4932246123ae45R23.

It's interesting why this PR passes CI. It looks like https://github.com/kubernetes/kubernetes/blob/master/pkg/util/term/resizeevents_windows.go#L28 should always fail to compile.

ixdy · 2017-04-14T18:34:23Z

PR testing by default only does fastbuild, i.e. only linux/amd64.

caesarxuchao · 2017-04-14T18:38:40Z

i'm sending a fix.

caesarxuchao · 2017-04-14T18:45:49Z

Sent #44506

Automatic merge from submit-queue fix cross-build Fix #41543 (comment)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 16, 2017

k8s-github-robot assigned lavalamp Feb 16, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Feb 16, 2017

dshulyak mentioned this pull request Feb 16, 2017

Move client/unversioned/remotecommand to client-go #41331

Merged

sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Feb 16, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2017

seh mentioned this pull request Apr 3, 2017

Move "kubernetes/pkg/client/unversioned/(portforward|remotecommand)" packages kubernetes/client-go#170

Closed

caesarxuchao reviewed Apr 4, 2017

View reviewed changes

caesarxuchao self-assigned this Apr 4, 2017

dshulyak force-pushed the decouple_remotecommand branch from 2fce8ba to 9e8ae5a Compare April 4, 2017 09:46

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2017

dshulyak force-pushed the decouple_remotecommand branch 4 times, most recently from 327f420 to 72ec16d Compare April 4, 2017 12:38

caesarxuchao reviewed Apr 5, 2017

View reviewed changes

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

lavalamp suggested changes Apr 12, 2017

View reviewed changes

Decouple remotecommand client from term/kubelet dependencies

f50480c

In order to move client/unversioned/remotecommand to client-go as a followup for this change we have to decouple it from tons of dependencies

dshulyak force-pushed the decouple_remotecommand branch from 7b99cde to f50480c Compare April 13, 2017 12:58

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2017

lavalamp approved these changes Apr 13, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2017

k8s-github-robot merged commit 4653a9b into kubernetes:master Apr 14, 2017

seh mentioned this pull request Apr 14, 2017

ref(*): kubernetes v1.6 helm/helm#2248

Merged

ixdy mentioned this pull request Apr 14, 2017

create multiple BUILD files in vendor. #40777

Merged

caesarxuchao mentioned this pull request Apr 14, 2017

fix cross-build #44506

Merged

k8s-github-robot pushed a commit that referenced this pull request Apr 14, 2017

Merge pull request #44506 from caesarxuchao/fix-cross-build

32e927f

Automatic merge from submit-queue fix cross-build Fix #41543 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple remotecommand #41543

Decouple remotecommand #41543

dshulyak commented Feb 16, 2017

k8s-reviewable commented Feb 16, 2017

dshulyak commented Feb 16, 2017

caesarxuchao Apr 4, 2017

caesarxuchao Apr 4, 2017

caesarxuchao commented Apr 4, 2017

dshulyak commented Apr 5, 2017

caesarxuchao left a comment

caesarxuchao Apr 5, 2017

dshulyak Apr 6, 2017

caesarxuchao Apr 5, 2017

dshulyak Apr 6, 2017

caesarxuchao Apr 5, 2017

dshulyak Apr 6, 2017

caesarxuchao Apr 5, 2017

caesarxuchao Apr 5, 2017

caesarxuchao commented Apr 6, 2017

caesarxuchao commented Apr 6, 2017

caesarxuchao commented Apr 6, 2017

caesarxuchao commented Apr 6, 2017

caesarxuchao commented Apr 7, 2017

lavalamp left a comment

lavalamp Apr 12, 2017

lavalamp Apr 12, 2017

lavalamp Apr 12, 2017

lavalamp Apr 12, 2017

lavalamp commented Apr 13, 2017

k8s-github-robot commented Apr 13, 2017

k8s-github-robot commented Apr 14, 2017

ixdy commented Apr 14, 2017

caesarxuchao commented Apr 14, 2017

ixdy commented Apr 14, 2017

caesarxuchao commented Apr 14, 2017

caesarxuchao commented Apr 14, 2017

Decouple remotecommand #41543

Decouple remotecommand #41543

Conversation

dshulyak commented Feb 16, 2017

k8s-reviewable commented Feb 16, 2017

dshulyak commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Apr 4, 2017

dshulyak commented Apr 5, 2017

caesarxuchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Apr 6, 2017

caesarxuchao commented Apr 6, 2017

caesarxuchao commented Apr 6, 2017

caesarxuchao commented Apr 6, 2017

caesarxuchao commented Apr 7, 2017

lavalamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Apr 13, 2017

k8s-github-robot commented Apr 13, 2017

k8s-github-robot commented Apr 14, 2017

ixdy commented Apr 14, 2017

caesarxuchao commented Apr 14, 2017

ixdy commented Apr 14, 2017

caesarxuchao commented Apr 14, 2017

caesarxuchao commented Apr 14, 2017