-
Notifications
You must be signed in to change notification settings - Fork 518
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
Add checking all IPs in Service reachibility verification #2188
Conversation
This is adding support for Dual Stack Kubernetes services
d561071
to
27dee97
Compare
/test pull-perf-tests-clusterloader2-kubemark |
/retest |
/test pull-perf-tests-clusterloader2-kubemark |
address := net.JoinHostPort(ip, fmt.Sprint(port)) | ||
command := fmt.Sprintf("curl %s", address) | ||
_, err = execservice.RunCommand(pod, command) | ||
if err != 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.
Consider using ErrorList for collecting errors from attempted connections to all the IP addresses rather than immediately breaking from the loop on receiving the first non-nil error here.
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.
I agree that would be useful, if we wanted to return errors, or log them
But here we just want to check if both IPs are reachable, what is the benefit of checking second one, if we know, that first one already failed?
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.
Ah, I didn't read the rest of this measurement's logic. I thought we're plugging this error through somewhere higher up the call stack and wanted to know if, for instance, IPv6 network programming failed but IPv4 did not 🤷.
In that case I'm okay with leaving it as it is.
/retest |
/retest |
/approve |
/assign @marseel |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marseel, mborsz, panslava, tosi3k 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 |
This is adding support for Dual Stack Kubernetes services
What type of PR is this?
/kind feature
What this PR does / why we need it:
When checking if service is reachable we were checking only first Service IP
This PR adds checking all IPs, this adds support for DualStack services, which can have 2 IPs