[WIP] OCPBUGS-7359: Azure: move to kube-proxy LB probes, don't detach masters when unready #60

damdo · 2023-03-10T11:56:42Z

CCM change matching in-tree: openshift/kubernetes#1506

TODO:

update tests

openshift-ci-robot · 2023-03-10T11:56:47Z

@damdo: This pull request references Jira Issue OCPBUGS-7359, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

CCM change matching in-tree: openshift/kubernetes#1506

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.

openshift-ci · 2023-03-10T11:58:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joelspeed for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…y default

openshift-ci-robot · 2023-03-10T12:02:03Z

@damdo: This pull request references Jira Issue OCPBUGS-7359, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

CCM change matching in-tree: openshift/kubernetes#1506

TODO:

update tests

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.

JoelSpeed

Would like to see some unit testing to make sure we are getting the expected ports and health paths out of this before we merge

JoelSpeed · 2023-03-10T12:08:21Z

pkg/provider/azure_loadbalancer.go

@@ -2198,7 +2206,7 @@ func (az *Cloud) buildHealthProbeRulesForPort(serviceManifest *v1.Service, port
 			}
 		}
 		if path == nil {
-			path = pointer.String(consts.HealthProbeDefaultRequestPath)
+			path = pointer.String("/healthz")


What is the default request path? Is it not already /healthz?

No the default is / Do we want to change that at the const level?

Where else is the const used?

Nowhere else. So we can probably change it.

Then I would change it yes :)

JoelSpeed · 2023-03-10T12:09:31Z

pkg/provider/azure_loadbalancer.go

@@ -2178,7 +2186,7 @@ func (az *Cloud) buildHealthProbeRulesForPort(serviceManifest *v1.Service, port
 					//nolint:gosec
 					if item.Port == int32(port) {
 						//found the port
-						properties.Port = pointer.Int32(item.NodePort)
+						properties.Port = pointer.Int32(lbNodesHealthCheckPort)


Is this correct? Is this not someone having specified in an annotation that they want a specific port? I'm not sure why there seems to be an override here anyway? Can you explain why this needs to be changed?

Yeah wanted to ask you an opinion on this.
Not sure why the probe port is being overridden by the NodePort here.
But if that's because we are "defaulting" then I think we should set it to lbNodesHealthCheckPort

Would need to dig into the code to work out and have an opinion on this, have you done any archeology to work out when this was introduced and why?

openshift-ci · 2023-03-10T14:33:22Z

@damdo: The following tests 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/openshift-e2e-azure-ovn-techpreview-upgrade	`04f8b7e`	link	false	`/test openshift-e2e-azure-ovn-techpreview-upgrade`
ci/prow/unit	`04f8b7e`	link	true	`/test unit`
ci/prow/e2e-azure-ovn-upgrade	`04f8b7e`	link	true	`/test e2e-azure-ovn-upgrade`
ci/prow/e2e-azure-ovn-ccm	`04f8b7e`	link	false	`/test e2e-azure-ovn-ccm`

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.

JoelSpeed · 2023-03-29T10:35:41Z

pkg/provider/azure_loadbalancer.go

This commit will want to be UPSTREAM: XXXX: so that once we agree on the approach upstream we can drop this, need a PR for that though

openshift-merge-robot · 2023-03-30T19:12:49Z

PR needs rebase.

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.

JoelSpeed · 2023-04-03T14:44:05Z

/close

We've superseded this now

openshift-ci-robot · 2023-04-03T14:46:50Z

@damdo: This pull request references Jira Issue OCPBUGS-7359. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In response to this:

CCM change matching in-tree: openshift/kubernetes#1506

TODO:

update tests

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.

openshift-ci · 2023-04-03T14:46:53Z

@JoelSpeed: Closed this PR.

In response to this:

/close

We've superseded this now

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.

openshift-ci-robot added the jira/severity-critical Referenced Jira bug's severity is critical for the branch this PR is targeting. label Mar 10, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 10, 2023

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Mar 10, 2023

openshift-ci bot requested review from elmiko and RadekManak March 10, 2023 11:58

damdo added 2 commits March 10, 2023 12:58

UPSTREAM: <carry>: loadbalancer: use kube-proxy based health probes b…

719baa8

…y default

UPSTREAM: <carry>: do not detach masters from lb when unready

04f8b7e

damdo force-pushed the azure-lb-issue-workaround-OCPBUGS-7359 branch from bb5a9a7 to 04f8b7e Compare March 10, 2023 11:59

JoelSpeed reviewed Mar 10, 2023

View reviewed changes

damdo mentioned this pull request Mar 14, 2023

OCPBUGS-7359: Azure: move to kube-proxy LB probes, don't detach masters when unready openshift/kubernetes#1506

Merged

2 tasks

damdo mentioned this pull request Mar 27, 2023

DO NOT MERGE: Debug Azure lb masters lockdown issue openshift-cloud-team/cloud-provider-azure#1

Closed

JoelSpeed reviewed Mar 29, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2023

openshift-ci bot closed this Apr 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] OCPBUGS-7359: Azure: move to kube-proxy LB probes, don't detach masters when unready #60

[WIP] OCPBUGS-7359: Azure: move to kube-proxy LB probes, don't detach masters when unready #60

damdo commented Mar 10, 2023 •

edited

openshift-ci-robot commented Mar 10, 2023

openshift-ci bot commented Mar 10, 2023

openshift-ci-robot commented Mar 10, 2023

JoelSpeed left a comment

JoelSpeed Mar 10, 2023

damdo Mar 10, 2023

JoelSpeed Mar 10, 2023

damdo Mar 10, 2023 •

edited

JoelSpeed Mar 10, 2023

JoelSpeed Mar 10, 2023

damdo Mar 10, 2023

JoelSpeed Mar 10, 2023

openshift-ci bot commented Mar 10, 2023

JoelSpeed Mar 29, 2023

openshift-merge-robot commented Mar 30, 2023

JoelSpeed commented Apr 3, 2023

openshift-ci-robot commented Apr 3, 2023

openshift-ci bot commented Apr 3, 2023

[WIP] OCPBUGS-7359: Azure: move to kube-proxy LB probes, don't detach masters when unready #60

[WIP] OCPBUGS-7359: Azure: move to kube-proxy LB probes, don't detach masters when unready #60

Conversation

damdo commented Mar 10, 2023 • edited

openshift-ci-robot commented Mar 10, 2023

openshift-ci bot commented Mar 10, 2023

openshift-ci-robot commented Mar 10, 2023

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damdo Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 10, 2023

Choose a reason for hiding this comment

openshift-merge-robot commented Mar 30, 2023

JoelSpeed commented Apr 3, 2023

openshift-ci-robot commented Apr 3, 2023

openshift-ci bot commented Apr 3, 2023

damdo commented Mar 10, 2023 •

edited

damdo Mar 10, 2023 •

edited