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

{container} Bugfix #15856: az container exec - remove eol check to avoid closing terminal before it even started on linux #16000

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

bitcloud
Copy link
Contributor

The backend sends an empty webSocket message after establishing an connection. The removed lines of code interpreted that as an EOL which exits the webSocket read loop.
As the Windows code doesn't do that as well, I removed it here as well now.
The terminal gets closed after the webSocket closes anyway.

Fixes: #15856

Description
As described in #15856 there is currently an issue getting a remote terminal to a container instance with az container exec as it immediately closes the connection.

Testing Guide

  • start a container e.g. with az container create with a long living container
  • try to execute a shell inside the container with az container exec --exec-command "/bin/bash" ...
  • currently it just exits without an error or shell

History Notes
[container] az container exec: remove eol check to avoid closing terminal before it even started on linux


This checklist is used to make sure that common guidelines for a pull request are followed.

The backend sends an empty webSocket message after establishing an connection. The removed lines of code interpreted that as an EOL which exits the webSocket read loop.
As the Windows code doesn't do that as well, I removed it here as well now.
The terminal gets closed after the webSocket closes anyway.

Fixes: #15856
@bitcloud bitcloud requested a review from zhoxing-ms as a code owner November 20, 2020 12:32
@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Nov 20, 2020
@ghost
Copy link

ghost commented Nov 20, 2020

Thank you for your contribution bitcloud! We will review the pull request and get back to you soon.

@bitcloud
Copy link
Contributor Author

I'm not quite sure if I should submit that as a hotfix as well to merge that also against the current version?

@yonzhan
Copy link
Collaborator

yonzhan commented Nov 21, 2020

container

@yonzhan yonzhan requested a review from qwordy November 21, 2020 02:14
@yungezz yungezz added the Container Instances az container label Nov 23, 2020
@bitcloud
Copy link
Contributor Author

bitcloud commented Dec 9, 2020

Any updates here? Anything else I should do?

@qwordy
Copy link
Member

qwordy commented Dec 10, 2020

Any updates here? Anything else I should do?

Wait for code owner to review.

@yungezz
Copy link
Member

yungezz commented Dec 23, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@qwordy
Copy link
Member

qwordy commented Dec 24, 2020

@zhoxing-ms

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Dec 24, 2020

Actually, I am not the owner of container module, which should be owned by service team.
@hjhhh3000vitae Could you please help to review this PR or ask the relevant person to look at it?

@hjhhh3000vitae
Copy link
Contributor

Sorry I'm not the owner either and I am not aware which team is the owner.

@qwordy
Copy link
Member

qwordy commented Jan 12, 2021

@zhoxing-ms Are you willing to be owner of this module?

@qwordy qwordy merged commit 87f979e into Azure:dev Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Container Instances az container customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'az container exec' does NOT run /bin/bash
6 participants