fix #3856: removing the PodUploadWebSocketListener

[shawkins]

Description

This aligns all of the Exec / Pod upload logic into a single listener.

It moves the send queue logic onto the ExecWebSocketListener. To bypass the usage of pipes there is an internal feature that exposes an OutputStream via ExecWatch.getInput - we could consider exposing that for end users as well. The flag for that also enables the ExecWebSocketListener to terminate when an error is seen on stdErr - that again could also a feature that is exposed to users.

Much of the other changes are around simplifying the PodOperationContext (switched to a lombok builder) and related methods.

There were a couple of existing bugs / issues as well:

• ExecWebSocketListener.onError needed its initial check negated - it was skipping all of the cleanup. Related to this was a minor change to only log the error when there is no listener.
• The close handling needed refined.
  • The jdk http client makes no guarantees about when the input side is closed after a call to close. The okHttp implementation has a 1 minute timeout after which it will automatically close the input. So I've added similar handling to the jdk implementation. I'll also update the javadocs for that.
  • When you close the ExecWebSocketListener it should not fully close, rather just send close and allow for graceful termination.
  • Similarly if the ExecWebSocketListener is closed from the remote side prior to the exit code being sent, we should not treat that as an exceptional outcome. Rather just complete the exitCode future with null. These last two together make it so we can appropriately detect the difference between an expected closure and a truly exceptional condition when doing the pod upload logic - as you don't always get a Status first.

@rohanKanojia @manusa I've opened this as a draft as I'll add more tests / docs, but would also like some feedback on exposing the terminate on error and direct input stream to users.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[shawkins]

I have a follow up set of changes to implement #4112 - which also makes public the ability to terminate on error. I can include them on this pr or open a separate one once this is merged. A bigger change that was required was to eliminate the usage of piped streams - we can't ensure the thread guarantee with other http client implementations, so we should shouldn't use them internally, have them in the api, and call out to not use them. However that coupled with adding back pressure adds some complexity to the replacement stream / websocket logic.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

53.4% Coverage
0.0% Duplication

[manusa]

I'm happy with the switch to Lombok (there's some partial usage scattered all around).
However, I'm not sure why you decided to use Lombok for this class in the scope of this PR.

[shawkins]

It relates more to the follow on pr #4115 - I had already started on some of those changes and was wanting to make it easier to manipulate the context.

[metacosm]

Maybe too late but I'm not sure how well Lombok will play with Quarkus when it comes to native compilation and library size…

[shawkins]

Maybe too late but I'm not sure how well Lombok will play with Quarkus when it comes to native compilation and library size…

lombok is already used in other places, for example the model classes. I don't recall that it inhibited native compilation there.