-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
OCPQE-28543: Check load balancer healthcheck port and path on aws #29544
base: master
Are you sure you want to change the base?
Conversation
@sunzhaohua2: This pull request references OCPQE-28543 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Changes LGTM, looks like the |
/lgtm |
/retest-required |
1 similar comment
/retest-required |
/retest |
@@ -88,6 +92,38 @@ var _ = g.Describe("[sig-cloud-provider][Feature:OpenShiftCloudControllerManager | |||
o.HaveField("Contents", o.ContainSubstring("cloud-provider=external")), | |||
))) | |||
}) | |||
|
|||
g.It("Cluster scoped load balancer healthcheck port and path should be 10256/healthz", func() { |
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.
The PR looks good, just want to check if that port could ever change, if so you'll save yourself some headaches in having to go submit a test rename PR if you take the port name itself out of the test name and keep it more generic.
"should be correct" or something like that
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.
The port number is the "kube-proxy" health check port. We don't actually use kube-proxy, but instead OVN-K exposes a health check endpoint at the same port.
If we were to change the port, this would mean all CCMs would have to be updated with a new port, and all implementations/distros of Kube would also need to update
In short, I don't see the port number changing, but, we could change the name anyway just in case
/lgtm Held in case you want to update test name, but I don't think it's required given Joel's feedback, though you may want to just in case. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, JoelSpeed, sunzhaohua2 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 |
@dgoodwin Thanks for holding on the test, based on Joel's feedback, I'm sure we don't need to update it. |
/hold cancel |
1 similar comment
@sunzhaohua2: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Job Failure Risk Analysis for sha: d62cd1a
|
Add case on aws to check cluster scoped load balancer healthcheck port and path should be 10256/healthz.
@JoelSpeed @huali9 @miyadav PTAL