Skip to content
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

Minor fix in KAS LB HealthCheck #2656

Merged
merged 1 commit into from Jun 7, 2023

Conversation

orenc1
Copy link
Contributor

@orenc1 orenc1 commented Jun 7, 2023

Not all cloud provider load balancer provisioners populate the Hostname field in the service.status.loadBalancer.ingress Some cloud providers (e.g. Azure, GCP, MetalLB) have load balancers that their ingress points are IP-based, not hostname-based. This PR changes the kube apiserver load balancer healthcheck to accomodate for IP-based ingress points as well. This fixes in issue where in some providers, the hosted cluster is successfully created but it has not completed due to a misleading 'APIServer load balancer is not provisioned' message.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

@openshift-ci openshift-ci bot requested review from csrwng and sjenning June 7, 2023 07:44
@openshift-ci openshift-ci bot added area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release and removed do-not-merge/needs-area labels Jun 7, 2023
@orenc1
Copy link
Contributor Author

orenc1 commented Jun 7, 2023

/retest

Comment on lines 726 to 727
if len(svc.Status.LoadBalancer.Ingress) == 0 || svc.Status.LoadBalancer.Ingress[0].Hostname == "" &&
svc.Status.LoadBalancer.Ingress[0].IP == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this read something like this?

if len(svc.Status.LoadBalancer.Ingress) == 0 ||
    (svc.Status.LoadBalancer.Ingress[0].Hostname == "" && svc.Status.LoadBalancer.Ingress[0].IP == "") {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i moved the AND clause to the same line, but the parenthesis is not necessary in this case, due to order of operations.

Not all cloud provider load balancer provisioners populate the Hostname field in the service.status.loadBalancer.ingress
Some cloud providers (e.g. Azure, GCP, MetalLB) have load balancers that their ingress points are IP-based, not hostname-based.
This PR changes the kube apiserver load balancer healthcheck to accomodate for IP-based ingress points as well.
This fixes in issue where in some providers, the hosted cluster is successfully created but it has not completed due to a misleading 'APIServer load balancer is not provisioned' message.

Signed-off-by: Oren Cohen <ocohen@redhat.com>
Copy link
Contributor

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel, orenc1

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 7, 2023

@orenc1: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-kubevirt-aws-ovn 133d98c link false /test e2e-kubevirt-aws-ovn

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/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 8ccb78b into openshift:main Jun 7, 2023
11 of 12 checks passed
@orenc1 orenc1 deleted the fix_lb_hc_kubevirt branch June 8, 2023 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants