Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local #4207

MartinForReal · 2023-07-03T07:17:03Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

move health probe related code to azure_loadbalancer_healthprobe*

Does this PR introduce a user-facing change?

Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-07-03T07:17:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MartinForReal

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:

~~OWNERS~~ [MartinForReal]

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

MartinForReal · 2023-07-03T09:44:28Z

pkg/provider/azure_loadbalancer_healthprobe.go

+			// Not need to verify probePort is in correct range again.
+			for _, item := range serviceManifest.Spec.Ports {
+				//nolint:gosec
+				if item.Port == int32(port) {


upper bound check has been added. false alarm.

MartinForReal · 2023-07-03T11:18:07Z

/test pull-cloud-provider-azure-e2e-ccm-capz

MartinForReal · 2023-07-04T01:36:49Z

/test pull-cloud-provider-azure-e2e-ccm-capz

pkg/consts/consts.go

tests/e2e/network/service_annotations.go

MartinForReal · 2023-07-05T02:35:10Z

pkg/provider/azure_loadbalancer_healthprobe.go

+}
+
+// getHealthProbeConfigProbeIntervalAndNumOfProbe
+func (az *Cloud) getHealthProbeConfigProbeIntervalAndNumOfProbe(serviceManifest *v1.Service, port int32) (*int32, *int32, error) {


newly added.

…afficpolicy is local

MartinForReal · 2023-07-05T02:35:19Z

pkg/provider/azure_loadbalancer_healthprobe.go

+// getHealthProbeConfigProbeInterval get probe interval in seconds
+// minimum probe interval in seconds is 5. ref: https://docs.microsoft.com/en-us/rest/api/load-balancer/load-balancers/create-or-update#probe
+// if probeInterval is not set, set it to default instead ref: https://docs.microsoft.com/en-us/rest/api/load-balancer/load-balancers/create-or-update#probe
+func (*Cloud) getHealthProbeConfigProbeInterval(serviceManifest *v1.Service, port int32) (*int32, error) {


newly added.

MartinForReal · 2023-07-05T02:35:30Z

pkg/provider/azure_loadbalancer_healthprobe.go

+// getHealthProbeConfigNumOfProbe get number of probes
+// minimum number of unhealthy responses is 2. ref: https://docs.microsoft.com/en-us/rest/api/load-balancer/load-balancers/create-or-update#probe
+// if numberOfProbes is not set, set it to default instead ref: https://docs.microsoft.com/en-us/rest/api/load-balancer/load-balancers/create-or-update#probe
+func (*Cloud) getHealthProbeConfigNumOfProbe(serviceManifest *v1.Service, port int32) (*int32, error) {


newly added.

MartinForReal · 2023-07-05T02:36:22Z

pkg/provider/azure_loadbalancer_healthprobe.go

+		properties.RequestPath = path
+	}
+
+	properties.IntervalInSeconds, properties.ProbeThreshold, err = az.getHealthProbeConfigProbeIntervalAndNumOfProbe(serviceManifest, port.Port)


extracted three functions from here.

MartinForReal · 2023-07-05T02:37:01Z

pkg/provider/azure_loadbalancer.go

@@ -2396,15 +2193,18 @@ func (az *Cloud) getExpectedLBRules(
 	if servicehelpers.NeedsHealthCheck(service) && !(consts.IsPLSEnabled(service.Annotations) && consts.IsPLSProxyProtocolEnabled(service.Annotations)) {
 		podPresencePath, podPresencePort := servicehelpers.GetServiceHealthCheckPathPort(service)
 		lbRuleName := az.getLoadBalancerRuleName(service, v1.ProtocolTCP, podPresencePort, isIPv6)
-
+		probeInterval, numberOfProbes, err := az.getHealthProbeConfigProbeIntervalAndNumOfProbe(service, podPresencePort)


newly added.

feiskyer

/lgtm

MartinForReal · 2023-07-05T06:10:40Z

/cherrypick release-1.27

k8s-infra-cherrypick-robot · 2023-07-05T06:11:17Z

@MartinForReal: #4207 failed to apply on top of branch "release-1.27":

Applying: Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local
Using index info to reconstruct a base tree...
M	pkg/provider/azure_loadbalancer.go
M	pkg/provider/azure_loadbalancer_test.go
M	tests/e2e/network/service_annotations.go
Falling back to patching base and 3-way merge...
Auto-merging tests/e2e/network/service_annotations.go
Auto-merging pkg/provider/azure_loadbalancer_test.go
CONFLICT (content): Merge conflict in pkg/provider/azure_loadbalancer_test.go
Auto-merging pkg/provider/azure_loadbalancer.go
CONFLICT (content): Merge conflict in pkg/provider/azure_loadbalancer.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.27

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.

MartinForReal · 2023-07-05T13:11:29Z

/cherrypick release-1.27

k8s-infra-cherrypick-robot · 2023-07-05T13:12:06Z

@MartinForReal: #4207 failed to apply on top of branch "release-1.27":

Applying: Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local
Using index info to reconstruct a base tree...
M	pkg/provider/azure_loadbalancer.go
M	pkg/provider/azure_loadbalancer_test.go
M	tests/e2e/network/service_annotations.go
Falling back to patching base and 3-way merge...
Auto-merging tests/e2e/network/service_annotations.go
Auto-merging pkg/provider/azure_loadbalancer_test.go
CONFLICT (content): Merge conflict in pkg/provider/azure_loadbalancer_test.go
Auto-merging pkg/provider/azure_loadbalancer.go
CONFLICT (content): Merge conflict in pkg/provider/azure_loadbalancer.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.27

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.

nimimeht · 2023-07-26T17:23:03Z

@MartinForReal - was this PR successfully cherry-picked for release-1.27? I don't see the merge commit for this PR in the release changelog: v1.27.5...v1.27.6

MartinForReal · 2023-07-27T00:11:55Z

@nimimeht please refer to 48ccfc3

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 3, 2023

k8s-ci-robot requested review from feiskyer and jwtty July 3, 2023 07:17

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 3, 2023

MartinForReal force-pushed the shafan/repo branch from e1b4fa5 to c48a6e5 Compare July 3, 2023 07:21

github-advanced-security bot found potential problems Jul 3, 2023

View reviewed changes

MartinForReal force-pushed the shafan/repo branch 3 times, most recently from 3a36fef to 97f36e3 Compare July 3, 2023 08:27

feiskyer reviewed Jul 5, 2023

View reviewed changes

pkg/consts/consts.go Outdated Show resolved Hide resolved

feiskyer reviewed Jul 5, 2023

View reviewed changes

tests/e2e/network/service_annotations.go Outdated Show resolved Hide resolved

MartinForReal force-pushed the shafan/repo branch from 97f36e3 to c00a160 Compare July 5, 2023 02:31

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 5, 2023

MartinForReal force-pushed the shafan/repo branch from c00a160 to 3728d5a Compare July 5, 2023 02:33

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 5, 2023

MartinForReal commented Jul 5, 2023

View reviewed changes

Support customization of numOfProbe and probeInterval when externaltr…

c405fc7

…afficpolicy is local

MartinForReal force-pushed the shafan/repo branch from 3728d5a to c405fc7 Compare July 5, 2023 02:40

MartinForReal commented Jul 5, 2023

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 5, 2023

feiskyer reviewed Jul 5, 2023

View reviewed changes

k8s-ci-robot assigned feiskyer Jul 5, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2023

k8s-ci-robot merged commit 6948221 into kubernetes-sigs:master Jul 5, 2023
16 checks passed

MartinForReal deleted the shafan/repo branch July 5, 2023 06:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local #4207

Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local #4207

MartinForReal commented Jul 3, 2023 •

edited

k8s-ci-robot commented Jul 3, 2023

MartinForReal Jul 3, 2023

MartinForReal commented Jul 3, 2023

MartinForReal commented Jul 4, 2023

MartinForReal Jul 5, 2023

MartinForReal Jul 5, 2023

MartinForReal Jul 5, 2023

MartinForReal Jul 5, 2023

MartinForReal Jul 5, 2023

feiskyer left a comment

MartinForReal commented Jul 5, 2023

k8s-infra-cherrypick-robot commented Jul 5, 2023

MartinForReal commented Jul 5, 2023

k8s-infra-cherrypick-robot commented Jul 5, 2023

nimimeht commented Jul 26, 2023

MartinForReal commented Jul 27, 2023

Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local #4207

Support customization of numOfProbe and probeInterval when externaltrafficpolicy is local #4207

Conversation

MartinForReal commented Jul 3, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jul 3, 2023

MartinForReal Jul 3, 2023

Choose a reason for hiding this comment

MartinForReal commented Jul 3, 2023

/test pull-cloud-provider-azure-e2e-ccm-capz

MartinForReal commented Jul 4, 2023

MartinForReal Jul 5, 2023

Choose a reason for hiding this comment

MartinForReal Jul 5, 2023

Choose a reason for hiding this comment

MartinForReal Jul 5, 2023

Choose a reason for hiding this comment

MartinForReal Jul 5, 2023

Choose a reason for hiding this comment

MartinForReal Jul 5, 2023

Choose a reason for hiding this comment

feiskyer left a comment

Choose a reason for hiding this comment

MartinForReal commented Jul 5, 2023

k8s-infra-cherrypick-robot commented Jul 5, 2023

MartinForReal commented Jul 5, 2023

k8s-infra-cherrypick-robot commented Jul 5, 2023

nimimeht commented Jul 26, 2023

MartinForReal commented Jul 27, 2023

MartinForReal commented Jul 3, 2023 •

edited