Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support error websocket channel #1045

Merged
merged 4 commits into from
May 23, 2018
Merged

Support error websocket channel #1045

merged 4 commits into from
May 23, 2018

Conversation

giannello
Copy link

This PR stems from jenkins-kubernetes not being able to properly deal with the return code when execing into a container. The kubernetes-client doesn't provide a separate pipe for the error channel exposed over the websocker, merging it with stderr instead.

This changeset exposes the error channel as a separate stream, and renames the others to keep a consistent naming strategy. The resize channel is deliberately ignored.

@giannello
Copy link
Author

@piyush-garg
Copy link
Contributor

ok to test

@piyush-garg
Copy link
Contributor

retest

@carlossg
Copy link
Contributor

Thanks @giannello !
I gave it a try but I think it's still missing the methods in PodOperationsImpl in order to pass the stream where you would want errors to be written.

It can get confusing given that there's already

  • writingError
  • readingError
  • redirectingError

Errorable interface also needs to be changed or a new one created.

And is the header Sec-WebSocket-Protocol: v4.channel.k8s.io needed too? kubernetes-client/python#297

@giannello giannello changed the title Support error websocket channel [WIP] Support error websocket channel Mar 25, 2018
@giannello
Copy link
Author

@carlossg added you as a collaborator in case you want to submit patches. Will take a look at the classes you mentioned tomorrow.
Not sure about the header, v4 should be the default if nothing else is specified - I don't really have an opinion and I'm around here just because I stumbled on that python client PR that mentioned the existence of a dedicated signaling channel, so please don't rely on my java skills.
Thank you for quickly jumping in, very appreciated.

@carlossg
Copy link
Contributor

In the meantime and given how many name changes are involved I'd love to hear from the maintainers before implementing the next changes

@carlossg
Copy link
Contributor

I have added the needed methods, and the implementation on jenkinsci/kubernetes-plugin#300 to use them

if (err != null) {
err.write(byteString.toByteArray());
if (error != null) {
error.write(byteString.toByteArray());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the interesting part, not sending error stream to stderr stream

To get error messages as json
@carlossg
Copy link
Contributor

This is ready, tests in kubernetes-plugin are passing using the new methods, would love to hear about the API changes and method naming from a maintainer
jenkinsci/kubernetes-plugin#300

@iocanel ?

@giannello giannello changed the title [WIP] Support error websocket channel Support error websocket channel Mar 31, 2018
@oscerd
Copy link
Member

oscerd commented Apr 5, 2018

ping @iocanel

@calebdelnay
Copy link

I'm wondering if there is anything a random outsider could do to help push this forward?

We use Jenkins to build Docker images using workers allocated via the Kubernetes plugin. However due to jenkinsci/kubernetes-plugin#300, which in turn depends on this issue, we're doing a manual build of the Kubernetes plugin with these fixes applied. It would be great to have these merged and included in upcoming releases.

@iocanel
Copy link
Member

iocanel commented Apr 24, 2018 via email

@carlossg
Copy link
Contributor

carlossg commented May 2, 2018

Ping @bparees

@bparees
Copy link

bparees commented May 2, 2018

@piyush1594 @iocanel bump, we definitely want to help enable the kubernetes pipeline plugin.

@carlossg
Copy link
Contributor

carlossg commented May 3, 2018

I wonder if it would be useful to avoid all the renaming for the sake of simplicity first so you can better see the actual changes needed

@giannello
Copy link
Author

I can push a less intrusive change, will find some time this afternoon to find a better naming.

@oscerd
Copy link
Member

oscerd commented May 11, 2018

ok to test

@oscerd
Copy link
Member

oscerd commented May 11, 2018

@piyush1594 @rohanKanojia @hrishin take a look at this PR.

@carlossg
Copy link
Contributor

any ETA for merging and releasing?

@iocanel
Copy link
Member

iocanel commented May 23, 2018

@giannello @carlossg: Apologies for the long delay. Since my main concern was naming and its addressed I'll merge asap.

@iocanel iocanel merged commit c78c1a4 into fabric8io:master May 23, 2018
@giannello giannello deleted the exec-ws-fixes branch May 23, 2018 09:11
@carlossg
Copy link
Contributor

@iocanel how long until it makes it into a release ?

@piyush-garg
Copy link
Contributor

Hey, @iocanel Thinking of releasing the client. We need to bump the minor version or it is not required for the time being

@piyush-garg
Copy link
Contributor

Hey @carlossg Kubernetes Client 3.2.0 released which have this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants