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

[WIP] On-demand serial port connection [full ci] #1918

Closed
wants to merge 1 commit into from
Closed

[WIP] On-demand serial port connection [full ci] #1918

wants to merge 1 commit into from

Conversation

caglar10ur
Copy link
Contributor

@caglar10ur caglar10ur commented Aug 10, 2016

On-demand serial port connection

- Tether always starts SSH server
- spec/serial.go no longer exists
- ContainerCreate calls AddInteractionToContainer to add Serial ports which
calls interaction.InteractionJoin
- ContainerCreate calls AddLoggingToContainer to add Serial ports which
calls logging.LoggingJoin
- VMs start with disconnected ports if 'run -d ...' used
- ContainerAttach toggles the connected bit via interaction.Bind
- We tear down the serial connection when we detach from the container.
This logic assumes that we let only 1 client to attach. Need to revisit
this and implement a reference counter.

- Commit now uses ChangeVersion to guard against updates happened
between when vm's config is read and when it is applied. To support that
we call Refresh in every time we modify the state. So each state change
now may contain one or more property collector call.

- Cosmetic changes all over

fixes #858

@caglar10ur
Copy link
Contributor Author

@hickeng can you take a quick look as a 1st pass?

return derr.NewRequestNotFoundError(fmt.Errorf("No such container: %s", msg))
}

func UnknownError(msg string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have both unknownerror and server error? They seem to serve much the same role

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code was using those, the only difference is the string that we are returning. Server error from vs Unknown error from. I could remove Unknown error and use ServerError everywhere if Unknown error messages are not required for some reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sflxn can you comment whether we need UnknownError for some reason?

@emlin
Copy link
Contributor

emlin commented Aug 16, 2016

One comment for your comments: 😄

ContainerCreate calls AddPortsToContainer to add Serial ports which calls interaction.Bind

which calls interaction.Join

@emlin
Copy link
Contributor

emlin commented Aug 16, 2016

Another question is you had Unbind method, but not called from docker engine. Should that be invoked after attach?

@caglar10ur
Copy link
Contributor Author

@hickeng I would love to get a new review

)

// Handler defines the interface
type Handler interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Convert them to return handle

@caglar10ur caglar10ur changed the title On-demand serial port connection [WIP] On-demand serial port connection Sep 13, 2016
@caglar10ur
Copy link
Contributor Author

Pushed the new version as it was sitting on my local disk but this is still WIP (missing reconnect logic) so no need to review it at this point.

@caglar10ur caglar10ur changed the title [WIP] On-demand serial port connection [WIP] On-demand serial port connection [full ci] Sep 19, 2016
- Tether always starts SSH server
- spec/serial.go no longer exists
- ContainerCreate calls AddInteractionToContainer to add Serial ports which
calls interaction.InteractionJoin
- ContainerCreate calls AddLoggingToContainer to add Serial ports which
calls logging.LoggingJoin
- VMs start with disconnected ports if 'run -d ...' used
- ContainerAttach toggles the connected bit via interaction.Bind
- We tear down the serial connection when we detach from the container.
This logic assumes that we let only 1 client to attach. Need to revisit
this and implement a reference counter.

- Commit now uses ChangeVersion to guard against updates happened
between when vm's config is read and when it is applied. To support that
we call Refresh in every time we modify the state. So each state change
now may contain one or more property collector call.

- Cosmetic changes all over

fixes #858
@caglar10ur
Copy link
Contributor Author

closing this as I started to split this into multiple PRs

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

Successfully merging this pull request may close these issues.

On-demand serial port connection
4 participants