Skip to content

Conversation

@camrynl
Copy link
Contributor

@camrynl camrynl commented Oct 20, 2022

Reason for Change:

CNI blocks when sending failure report because CNS exits and is not receiving.
Unblocking channel with select/default clause.

Issue Fixed:

Requirements:

Notes:

@camrynl camrynl added cni Related to CNI. fix Fixes something. labels Oct 20, 2022
@camrynl camrynl requested a review from tamilmani1989 October 20, 2022 16:03
rbtr
rbtr previously approved these changes Oct 20, 2022

// Cancel - signal to tear down telemetry buffer
func (tb *TelemetryBuffer) Cancel() {
tb.cancel <- true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have not read through how the value sent on this channel is being used, so I don't know how messy it is. But the better pattern for this kind of binary "write once, read any" signal via a channel is to close the channel instead of sending a single item through it. Closing is non-blocking, and any reader waiting on the channel will immediately unblock and read a zero-value from the channel, and, if using two-value reads (v, ok := <-tb.cancel), a false indicating that the channel is closed.
It's important that this be the only writer for the channel, because any subsequent attempt to send on the closed channel will panic.

@tamilmani1989
Copy link
Member

on a second thought, i think we should remove tb.Cancel() from SendReport, SendCNIMetric, SendCNIEvent from telemetry/telemetry.go. I see no value with that call and its invoked only on write failure and it doesn't close server socket since telemetrybuffer has its own copy

tamilmani1989
tamilmani1989 previously approved these changes Oct 21, 2022
@camrynl camrynl merged commit f4d1c9b into Azure:master Oct 24, 2022
@camrynl camrynl deleted the channelFix branch October 24, 2022 20:16
rjdenney pushed a commit to rjdenney/azure-container-networking that referenced this pull request Jan 19, 2023
* implement select case

* remove calls to tb.cancel() in telemetry.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cni Related to CNI. fix Fixes something.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants