agent: handle initial session message#797
Merged
tonistiigi merged 2 commits intomoby:masterfrom Jun 5, 2016
Merged
Conversation
The role of the session message has expanded greatly in the last week, including node and networking information. Previously, it was sufficient to drop the first message and listen for updates. Now, there is critical bootstrapping data that must be propagated at session startup time. This change ensures this. Signed-off-by: Stephen J Day <stephen.day@docker.com>
|
|
||
| func (s *session) handleSessionMessage(ctx context.Context, msg *api.SessionMessage) error { | ||
| select { | ||
| case s.messages <- msg: |
Contributor
There was a problem hiding this comment.
You still need to handle Disconnect message somewhere here don't you?
Contributor
Author
There was a problem hiding this comment.
Odd. That should have been in (*Agent).handleSessionMessage. Looks like it was removed here: 1e193df#diff-6ea70e0a4de93ebcb83000a028edc163L290.
Contributor
There was a problem hiding this comment.
That's what I remembered seeing from before but went back and looked there and wasn't there.
Contributor
Author
There was a problem hiding this comment.
Thanks for pointing this out. We are removing this field and found a much better way to force disconnect.
Current coverage is 52.56%
@@ master #797 diff @@
==========================================
Files 78 78
Lines 11544 11542 -2
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 6070 6067 -3
+ Misses 4639 4638 -1
- Partials 835 837 +2
|
Contributor
|
LGTM |
After some close study of session handling and GRPC, we found it to be sufficient to close the Server's transport stream to force a reconnection at the agent-level. Under this realization, we have removed the `disconnect` field from `api.SessionMessage`. Connection rebalancing is not yet implemented, but we have confirmed that the current behavior is sufficient to support in the future without changes in the agent. The detected connection closer on the client-side forces GRPC to call into the picker and select a new manager node. Signed-off-by: Stephen J Day <stephen.day@docker.com>
Contributor
Author
|
cc @tonistiigi PTAL the disconnect removal. |
Member
|
LGTM |
cyli
added a commit
to cyli/swarmkit
that referenced
this pull request
May 25, 2018
to reconnect. Just return an error - if an error is returned, the agent will reconnect anyway - it seems like the transport closing was a way to convince GRPC's load balancer to start reconnecting automatically. See moby#797. Signed-off-by: Ying Li <ying.li@docker.com>
cyli
added a commit
to cyli/swarmkit
that referenced
this pull request
May 29, 2018
to reconnect. Just return an error - if an error is returned, the agent will reconnect anyway - it seems like the transport closing was a way to convince GRPC's load balancer to start reconnecting automatically. See moby#797. Signed-off-by: Ying Li <ying.li@docker.com>
cyli
added a commit
to cyli/swarmkit
that referenced
this pull request
May 29, 2018
to reconnect. Just return an error - if an error is returned, the agent will reconnect anyway - it seems like the transport closing was a way to convince GRPC's load balancer to start reconnecting automatically. See moby#797. Signed-off-by: Ying Li <ying.li@docker.com>
Merged
thaJeztah
pushed a commit
to thaJeztah/swarmkit
that referenced
this pull request
Aug 19, 2019
to reconnect. Just return an error - if an error is returned, the agent will reconnect anyway - it seems like the transport closing was a way to convince GRPC's load balancer to start reconnecting automatically. See moby#797. Signed-off-by: Ying Li <ying.li@docker.com> (cherry picked from commit 90418ad) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The role of the session message has expanded greatly in the last week,
including node and networking information. Previously, it was sufficient
to drop the first message and listen for updates. Now, there is critical
bootstrapping data that must be propagated at session startup time.
This change ensures this.
Signed-off-by: Stephen J Day stephen.day@docker.com
cc @mrjana @tonistiigi @aluzzardi