Refactor streaming code to support interop testing #16451

ncdc · 2015-10-28T19:13:36Z

Refactor exec/attach/port forward client and server code to better
support interop testing of different client and server subprotocol
versions.

Fixes #16119

ncdc · 2015-10-28T19:14:07Z

@smarterclayton @lavalamp @liggitt here's my WIP on #16119

k8s-bot · 2015-10-28T19:18:09Z

GCE e2e build/test failed for commit 85e4045030e14b1aa14880669d5b812a94704025.

k8s-github-robot · 2015-10-28T19:20:15Z

Labelling this PR as size/XXL

k8s-bot · 2015-11-23T22:07:10Z

GCE e2e test build/test passed for commit d56f72058496f49e116f30648a61e788bde0f364.

k8s-bot · 2015-12-11T14:22:40Z

GCE e2e test build/test passed for commit d56f72058496f49e116f30648a61e788bde0f364.

ncdc · 2015-12-11T14:25:03Z

@smarterclayton @lavalamp are you interested in having me continue with this? If so, I'll rebase and would appreciate feedback. If not, we can close it.

smarterclayton · 2015-12-11T21:18:58Z

I think it's a good thing to do, it's your call though whether it's more
important than other changes you have.

On Fri, Dec 11, 2015 at 2:38 PM, Yu-Ju Hong notifications@github.com
wrote:

Assigned #16451 #16451 to
@smarterclayton https://github.com/smarterclayton.

—
Reply to this email directly or view it on GitHub
#16451 (comment).

ncdc · 2015-12-11T21:22:02Z

I'm looking for feedback on the current state of the PR to see if it's in the right direction. Also, when we move to HTTP/2, it's highly likely that the httpstream package's Connection and Stream types will become deprecated; they're not used for the websocket implementation, and the HTTP/2 implementation pattern will likely be operating solely on the request and response bodies and muxing stdout/stderr as needed (similar to how you did it in the wsstream).

ncdc · 2015-12-11T21:23:09Z

But I do also think helping David out with the group / version / kind apiserver changes are more important.

smarterclayton · 2015-12-11T21:36:27Z

The structure looks fine to me. Anything is better than the mess before. This is less bad.

ncdc · 2016-01-18T14:24:07Z

@lavalamp @smarterclayton @liggitt PR updated (finally), PTAL

ncdc · 2016-01-18T14:40:27Z

I see I have some import direction issues that are causing the httptest.serve flag to leak into most/all of the CLI commands. Working on a fix now.

k8s-bot · 2016-01-18T14:54:46Z

GCE e2e test build/test passed for commit 925767319e56e468c8dec23490a1df91d056d89e.

ncdc · 2016-01-18T15:27:32Z

Apparently the leaking is occurring because pkg/kubectl/cmd/exec.go and pkg/kubectl/cmd/attach.go are importing pkg/kubelet/server to make use of my new server.SupportedStreamingProtocols var in this PR. I'm not sure why importing that package would yield an httptest flag coming into the picture, though. It looks like the server package only imports httptest in server_test.go...

ncdc · 2016-01-18T16:43:28Z

Well this was fun: pkg/kubelet/server -> pkg/kubelet/container -> https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/container/runtime_mock.go#L22 which ultimately imports httptest

ncdc · 2016-01-18T18:01:24Z

Ok, so the question is this: is it appropriate to put SupportedStreamingProtocols in pkg/kubelet/server? If so, then we are going to end up with --httptest.serve being a new no-op flag in all kubectl commands until we can resolve what to do about pkg/kubelet/container/runtime_mock.go, since that's ultimately responsible for pulling in httptest.

yujuhong · 2016-01-21T22:58:04Z

@ncdc, if the runtime_mock.go file is causing problems, I can move it to the test file that's using the mock until we have better solutions.

smarterclayton · 2016-01-22T17:58:46Z

I'd prefer to fix runtime_mock.go if at all possible (by changing how the flag is registered).

ncdc · 2016-01-22T18:06:52Z

@smarterclayton any regular (non-test) file that pulls in testify's mock package will end up transitively pulling in net/http/httptest, and the httptest package has a flag that is registered as soon as you import the package. It's not just the Kubernetes code that has an issue... I found some code that the Mesos cloud provider imports (another Mesos library) that also has a mock, so even if we fix it here, it's not a complete fix...

k8s-bot · 2016-03-22T17:23:22Z

GCE e2e build/test passed for commit 54e8b98497daf287b3bd091dd71ad213e947b83a.

ncdc · 2016-03-22T18:07:51Z

@kubernetes/rh-cluster-infra PTAL if you have some time to review

smarterclayton · 2016-04-01T16:07:41Z

pkg/kubelet/server/remotecommand/websocket.go

+)
+
+// standardShellChannels returns the standard channel types for a shell connection (STDIN 0, STDOUT 1, STDERR 2)
+// along with the approprxate duplex value


Describe what protocols this supports. Typo in approximate

smarterclayton · 2016-04-01T16:10:32Z

Are the existing E2E tests sufficient to cover this function?

ncdc · 2016-04-01T16:18:00Z

Are the existing E2E tests sufficient to cover this function?

They cover whatever the current protocol is. The new unit tests cover version skew testing.

smarterclayton · 2016-04-01T16:58:55Z

Changes LGTM

smarterclayton · 2016-04-01T16:59:18Z

Squash and I'll tag

Refactor exec/attach client and server code to better support interoperability testing of different client and server subprotocol versions.

ncdc · 2016-04-01T17:06:01Z

Squashed

k8s-bot · 2016-04-01T17:12:42Z

GCE e2e build/test failed for commit 83ea14f586a79e1b9b5901710231f5d9873cb991.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

ncdc · 2016-04-01T17:20:41Z

Failure was #22639 / #23607

k8s-bot · 2016-04-01T17:41:39Z

GCE e2e build/test passed for commit 4551ba6.

k8s-github-robot · 2016-04-01T17:42:46Z

Removing LGTM because the release note process has not been followed.
One of the following labels is required "release-note", "release-note-none", or "release-note-action-required"
Please see: https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/release-notes.md

k8s-github-robot · 2016-04-01T21:47:19Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-04-01T22:24:31Z

GCE e2e build/test passed for commit 4551ba6.

k8s-github-robot · 2016-04-01T23:28:13Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-04-02T00:11:03Z

GCE e2e build/test passed for commit 4551ba6.

k8s-github-robot · 2016-04-02T00:11:26Z

Automatic merge from submit-queue

googlebot added the cla: yes label Oct 28, 2015

k8s-github-robot assigned yujuhong Oct 28, 2015

k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 28, 2015

ncdc force-pushed the exec-interop-testing branch from 85e4045 to d56f720 Compare November 23, 2015 21:33

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

yujuhong assigned smarterclayton and unassigned yujuhong Dec 11, 2015

ncdc force-pushed the exec-interop-testing branch from d56f720 to 9257673 Compare January 18, 2016 14:23

ncdc changed the title ~~[do not merge] WIP: Refactor streaming code to support interop testing~~ Refactor streaming code to support interop testing Jan 18, 2016

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

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

k8s-github-robot added the release-note-label-needed label Mar 31, 2016

ncdc mentioned this pull request Apr 1, 2016

API: use cases for kubectl attach? #23335

Closed

smarterclayton reviewed Apr 1, 2016
View reviewed changes

Refactor exec code to support version skew testing

4551ba6

Refactor exec/attach client and server code to better support interoperability testing of different client and server subprotocol versions.

ncdc force-pushed the exec-interop-testing branch from 83ea14f to 4551ba6 Compare April 1, 2016 17:05

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2016

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

ncdc added release-note-none Denotes a PR that doesn't merit a release note. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 1, 2016

k8s-github-robot removed the release-note-label-needed label Apr 1, 2016

k8s-github-robot merged commit 3b65927 into kubernetes:master Apr 2, 2016

ncdc deleted the exec-interop-testing branch February 13, 2017 17:24

Refactor streaming code to support interop testing #16451

Refactor streaming code to support interop testing #16451

Conversation

ncdc commented Oct 28, 2015

ncdc commented Oct 28, 2015

k8s-bot commented Oct 28, 2015

k8s-github-robot commented Oct 28, 2015

k8s-bot commented Nov 23, 2015

k8s-bot commented Dec 11, 2015

ncdc commented Dec 11, 2015

smarterclayton commented Dec 11, 2015

ncdc commented Dec 11, 2015

ncdc commented Dec 11, 2015

smarterclayton commented Dec 11, 2015

ncdc commented Jan 18, 2016

ncdc commented Jan 18, 2016

k8s-bot commented Jan 18, 2016

ncdc commented Jan 18, 2016

ncdc commented Jan 18, 2016

ncdc commented Jan 18, 2016

yujuhong commented Jan 21, 2016

smarterclayton commented Jan 22, 2016

ncdc commented Jan 22, 2016

k8s-bot commented Mar 22, 2016

ncdc commented Mar 22, 2016

smarterclayton Apr 1, 2016

Choose a reason for hiding this comment

smarterclayton commented Apr 1, 2016

ncdc commented Apr 1, 2016

smarterclayton commented Apr 1, 2016

smarterclayton commented Apr 1, 2016

ncdc commented Apr 1, 2016

k8s-bot commented Apr 1, 2016

ncdc commented Apr 1, 2016

k8s-bot commented Apr 1, 2016

k8s-github-robot commented Apr 1, 2016

k8s-github-robot commented Apr 1, 2016

k8s-bot commented Apr 1, 2016

k8s-github-robot commented Apr 1, 2016

k8s-bot commented Apr 2, 2016

k8s-github-robot commented Apr 2, 2016