Added delay before reporting and enforced a minimum interval of 10s #6608
Conversation
pkg/agent/monitortool/monitor.go
Outdated
| @@ -444,11 +455,17 @@ func (m *NodeLatencyMonitor) monitorLoop(stopCh <-chan struct{}) { | |||
| case <-tickerCh: | |||
| // Try to send pingAll signal | |||
| m.pingAll(ipv4Socket, ipv6Socket) | |||
| // Introduce a delay to allow for response processing | |||
| time.Sleep(time.Duration(m.reportInterval / 2)) | |||
There was a problem hiding this comment.
A sleep is blocking and is definitely not the right approach for a select-based loop like this one.
There was a problem hiding this comment.
thanks,removed the sleep block.
pkg/agent/monitortool/monitor.go
Outdated
| var pingInterval time.Duration | ||
| if nlm.Spec.PingIntervalSeconds < MinReportIntervalSeconds { | ||
| pingInterval = time.Duration(MinReportIntervalSeconds) * time.Second | ||
| } else { | ||
| pingInterval = time.Duration(nlm.Spec.PingIntervalSeconds) * time.Second | ||
| } |
There was a problem hiding this comment.
Please read the original issue carefully. The MinReportIntervalSeconds value should not impact how frequently ICMP probes are sent (we should still honor the value configured through nlm.Spec.PingIntervalSeconds). This change should be about decoupling the "ping" operation and the "reporting" operation. One possible approach would be to have 2 different tickers in the monitorLoop function, and perhaps ensuring there is some jitter (for reporting?) in case the ping period and the reporting period are the same.
Yushmanth-reddy
force-pushed
the
rectify_nodelatncyStats
branch
from
43f7a1e
to
230c862
Compare
Signed-off-by: Yushmanth <[email protected]>
|
@antoninbas tried to decouple by adding 2 tickers. But I didn't add jitters as of now. |
| ipv6ProtocolICMPRaw = "ip6:ipv6-icmp" | ||
| protocolICMP = 1 | ||
| protocolICMPv6 = 58 | ||
| MinReportIntervalSeconds = 10 |
There was a problem hiding this comment.
there is no need to export this constant
also it feels like this could be typed as a duration: const minReportInterval = 10*time.Second
| if pingInterval < MinReportIntervalSeconds*time.Second { | ||
| m.reportInterval = MinReportIntervalSeconds * time.Second | ||
| } else { | ||
| m.reportInterval = pingInterval |
There was a problem hiding this comment.
maybe as a "cheap" alternative to jitter, we could set this to m.reportInterval = pingInterval + 1*time.Second instead? This way we could ensure that the 2 tickers don't tick in lockstep.
|
@Yushmanth-reddy any update regarding this PR? |
|
Sorry for the delay @antoninbas. I've decided to close this pull request for now. I feel that I need to take some more time to better understand the project. Once I'm confident, I'll revisit and solve issues. |
Solves #6570
I tried to set min pingInterval to be 10s and introduced a delay before reporting the nodeLatencyStats.
seeking your guidance in resolving this issue and looking forward to your feedback on the proposed changes