-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
channelz: ensure SubChannel
is always registered with parent
#7094
channelz: ensure SubChannel
is always registered with parent
#7094
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #7094 +/- ##
=======================================
Coverage 81.07% 81.08%
=======================================
Files 345 345
Lines 33927 33928 +1
=======================================
+ Hits 27508 27512 +4
+ Misses 5241 5235 -6
- Partials 1178 1181 +3
|
@daniel-weisse do you know if this is a regression with the latest release? If so we may prefer to restore the previous behavior instead. |
The Channel and SubChannel types were introduced in From a brief look at the v1.62.2 code, it looks like parent information was always omitted from its string representation if there was no parent: grpc-go/internal/channelz/id.go Lines 69 to 75 in 10baa6b
grpc-go/internal/channelz/id.go Lines 48 to 50 in 10baa6b
Edit: Took another look at the code. grpc-go/internal/channelz/funcs.go Lines 247 to 255 in 10baa6b
In v1.63 however, the grpc-go/internal/channelz/funcs.go Lines 146 to 150 in c68f456
I think the correct way would be passing the parent information here, like it is done later on in the function. One fix would be adjusting the signature of the So we would go from this: func RegisterSubChannel(pid int64, ref string) *SubChannel { to this: func RegisterSubChannel(parent *Channel, ref string) *SubChannel { The function then no longer needs to load the parent from the channel map |
This sounds good. Subchannels should always have parents, so I was skeptical about the initial change proposal -- we should ideally be logging both the channel+subchannel IDs in all the subchannel events. |
SubChannel.parent
in string representation if parent
is nil
SubChannel
is always registered with parent
…s nil Signed-off-by: Daniel Weiße <dw@edgeless.systems>
…parent is nil" This reverts commit 3694b2e.
45e7acf
to
c328773
Compare
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
c328773
to
5f17e6e
Compare
Updated the PR to restore the |
if pid == nil { | ||
return nil, errors.New("a SubChannel's parent id cannot be nil") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not return an error here since it complicates the code so much, and it should just be an invariant of the code like any other dereference. Just panic and we'll make sure RegisterSubChannel
is always called properly.
Actually, this change is pretty important to get done quickly, so I created #7101 and we will be doing a patch release with it ASAP. Thank you for the fix!
As mentioned in the review comment, we'll be continuing this in #7101. Sorry for that, but it needs to get done very quickly. Thank you! |
channelz.SubChannel
may be missing parent information if created usingchannelz.RegisterSubChannel
.This is because
channelz.RegisterSubChannel
does not take achannelz.Channel
as argument, but just the ID of this channel.The function itself then tries to load the Channel from a package scoped
channelMap
.This map does not contain any entries if channelz data collection is turned off, resulting in the SubChannel being created without a parent.
Which in turn results in the SubChannel's string representation being logged as
<nil> SubChannel #1234
(1234 being some ID of the SubChannel).This PR fixes this by restoring the behavior of
v1.62.2
, which instead of passing just the ID of a SubChannel's parent, passed a reference to the parent Channel, returning an error if it isnil
:grpc-go/internal/channelz/funcs.go
Lines 199 to 206 in 10baa6b
RELEASE NOTES: none