MGMT-4718 Capture network latency and packet loss at L3 network connectivity #187
Conversation
This file contains 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
openshift-ci-robot
added
the
do-not-merge/work-in-progress
jordigilh
force-pushed
the
MGMT-4718_capture_network_latency
branch
from
8d50ae2 to
02ba00f
Compare
YuviGold
reviewed
Mar 31, 2021
YuviGold
reviewed
Mar 31, 2021
YuviGold
reviewed
Mar 31, 2021
jordigilh
force-pushed
the
MGMT-4718_capture_network_latency
branch
from
02ba00f to
c30b1e5
Compare
jordigilh
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
jordigilh
force-pushed
the
MGMT-4718_capture_network_latency
branch
from
c30b1e5 to
76a49e9
Compare
|
/retest |
jordigilh
force-pushed
the
MGMT-4718_capture_network_latency
branch
2 times, most recently
from
58f5b4e to
0a69fcd
Compare
jordigilh
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
jordigilh
force-pushed
the
MGMT-4718_capture_network_latency
branch
2 times, most recently
from
9a82355 to
3ed01a7
Compare
jordigilh
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
needs-rebase
jordigilh
force-pushed
the
MGMT-4718_capture_network_latency
branch
3 times, most recently
from
c5225b8 to
be0517a
Compare
|
@YuviGold please review |
openshift-ci
bot
removed
the
needs-rebase
ori-amizur
reviewed
Apr 29, 2021
jordigilh
force-pushed
the
MGMT-4718_capture_network_latency
branch
from
be0517a to
dfd5212
Compare
ori-amizur
reviewed
Apr 30, 2021
jordigilh
force-pushed
the
MGMT-4718_capture_network_latency
branch
from
dfd5212 to
1499058
Compare
ori-amizur
reviewed
May 4, 2021
| L3Connectivity: []*models.L3Connectivity{}, | ||
| } | ||
| addresses := getOutgoingAddresses(conCheck.getHost().Nics) | ||
| dataCh := make(chan any) |
There was a problem hiding this comment.
Why channel with size 1 ?
There was a problem hiding this comment.
because there is only 1 routine consuming the data from the channel. Having the channel with a bigger queue won't improve the time it takes for this single routine to process all the information generated by all the other routines.
src/commands/connectivity_check.go
Outdated
| allDstMacs = append(allDstMacs, destNic.Mac) | ||
| func l2CheckConnectivity(dataCh chan any, conCheck connectivityCmd) { | ||
| defer sendDone(dataCh) | ||
| doneCh := make(chan any) |
There was a problem hiding this comment.
Suggest to have single method to signal the go-routine terminates - either wait-group or done channel.
It is not relevant to channels that the done information is sent along with the data.
There was a problem hiding this comment.
ok. I'll refactor this part to use a wait group like in
assisted-installer-agent/src/commands/connectivity_check.go
Lines 123 to 124 in 1499058
It will save the need to use this
doneCh to sync.
jordigilh
force-pushed
the
MGMT-4718_capture_network_latency
branch
from
1499058 to
1cfbb11
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jordigilh, ori-amizur The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR extends the L3 connectivity information gathered by the agent by capturing the average latency (RTT) and the packet loss information per host connection.
Code has been refactored and unit tests extended to cover the new functionality as well as the aggregation process to validate the refactored code.
This PR is the counterpart in the assisted-service.
@pkliczewski @jakub-dzon @machacekondra @masayag @YuviGold please review at your discretion in the meanwhile.
/Jordi