Add numOfProbe and probeInterval in health probe configuration #950

MartinForReal · 2021-12-22T12:32:03Z

Signed-off-by: Shangxiang Fan shafan@microsoft.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #948

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Two new annotations are added in order to customize health probe behavior

service.beta.kubernetes.io/azure-load-balancer-health-probe-interval: "15"
service.beta.kubernetes.io/azure-load-balancer-health-probe-num-of-probe: "2"

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

k8s-ci-robot · 2021-12-22T12:32:11Z

Hi @MartinForReal. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 · 2021-12-22T12:33:13Z

More units are being added.

coveralls · 2021-12-23T05:48:23Z

Coverage increased (+0.06%) to 80.003% when pulling 9665e61 on MartinForReal:feat/add_probe_interval into c0a5fd8 on kubernetes-sigs:master.

MartinForReal · 2021-12-23T06:17:52Z

/assign @nilo19 @feiskyer

feiskyer · 2021-12-24T01:16:43Z

/ok-to-test

feiskyer · 2021-12-24T01:18:28Z

pkg/provider/azure_loadbalancer.go

+	numberOfProbes, err := getInt32FromAnnotations(s.Annotations, consts.ServiceAnnotationLoadBalancerHealthProbeNumOfProbe, func(val *int32) error {
+		//total probe should be less than 120, minimum probe interval is 2 and minimum number of unhealthy responses is 2
+		//number of probe values should range between 2 to 60
+		if *val > 60 || *val < 2 {


Could you get the interval first and validate the total probe interval by number*interval?

Moved this business rule out of validator function and check it at the end of the function, which decouples the validator functions.

feiskyer · 2021-12-24T01:19:28Z

pkg/provider/azure_loadbalancer.go

+
+	probeInterval, err := getInt32FromAnnotations(s.Annotations, consts.ServiceAnnotationLoadBalancerHealthProbeInterval, func(val *int32) error {
+		//total probe should be less than 120
+		if (*val)*(*numberOfProbes) >= 120 {


also need to check the minimum value

validation rule is added .

feiskyer · 2021-12-24T01:20:22Z

pkg/provider/azure_loadbalancer.go

+	return probeInterval, numberOfProbes, nil
+}
+
+type Int32BusinessValidator func(*int32) error


Could you add some comments for newly added types and functions?

feiskyer · 2021-12-24T01:22:33Z

pkg/provider/azure_loadbalancer_test.go

+	}
+}
+
+func Test_getProbeIntervalInSecondsAndNumOfProbe(t *testing.T) {


Thanks for adding those tests. Could you also add another test for updating annotation values? e.g. when the value of the probe interval annotation changes, the probe interval on LB should be updated from the old value to the new one

This is a common case for ccm but this case is out of current scope because here we only provide expected configuration of loadbalancer probe

nilo19 · 2021-12-24T01:30:38Z

@MartinForReal please enrich the doc here in a separate PR. https://github.com/kubernetes-sigs/cloud-provider-azure/blob/master/site/content/en/topics/loadbalancer.md

Signed-off-by: Shangxiang Fan <shafan@microsoft.com>

MartinForReal · 2021-12-24T08:04:14Z

/retest

nilo19 · 2021-12-24T09:19:09Z

/lgtm

feiskyer

Thanks for addressing the comments. LGTM

/lgtm
/approve

After the PR merged, please file new PRs for 1) update docs and 2) cherry pick the changes to release branches

k8s-ci-robot · 2021-12-27T02:16:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, 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~~ [feiskyer]

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

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 22, 2021

k8s-ci-robot requested review from andyzhangx and feiskyer December 22, 2021 12:32

MartinForReal changed the title ~~[wip] add numOfProbe and probeInterval in health probe configuration~~ Add numOfProbe and probeInterval in health probe configuration Dec 23, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2021

k8s-ci-robot assigned feiskyer and nilo19 Dec 23, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 24, 2021

feiskyer reviewed Dec 24, 2021

View reviewed changes

add numOfProbe and probeInterval in health probe configuration

9665e61

Signed-off-by: Shangxiang Fan <shafan@microsoft.com>

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 24, 2021

feiskyer approved these changes Dec 27, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 27, 2021

k8s-ci-robot merged commit 35267b3 into kubernetes-sigs:master Dec 27, 2021

MartinForReal deleted the feat/add_probe_interval branch December 27, 2021 02:28

MartinForReal mentioned this pull request Dec 28, 2021

REQUEST: New membership for MartinForReal kubernetes/org#3157

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add numOfProbe and probeInterval in health probe configuration #950

Add numOfProbe and probeInterval in health probe configuration #950

MartinForReal commented Dec 22, 2021 •

edited

Loading

k8s-ci-robot commented Dec 22, 2021

MartinForReal commented Dec 22, 2021

coveralls commented Dec 23, 2021 •

edited

Loading

MartinForReal commented Dec 23, 2021

feiskyer commented Dec 24, 2021

feiskyer Dec 24, 2021

MartinForReal Dec 24, 2021

feiskyer Dec 24, 2021

MartinForReal Dec 24, 2021

feiskyer Dec 24, 2021

MartinForReal Dec 24, 2021

feiskyer Dec 24, 2021

MartinForReal Dec 24, 2021

nilo19 commented Dec 24, 2021

MartinForReal commented Dec 24, 2021

nilo19 commented Dec 24, 2021

feiskyer left a comment

k8s-ci-robot commented Dec 27, 2021

Add numOfProbe and probeInterval in health probe configuration #950

Add numOfProbe and probeInterval in health probe configuration #950

Conversation

MartinForReal commented Dec 22, 2021 • edited Loading

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 Dec 22, 2021

MartinForReal commented Dec 22, 2021

coveralls commented Dec 23, 2021 • edited Loading

MartinForReal commented Dec 23, 2021

feiskyer commented Dec 24, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilo19 commented Dec 24, 2021

MartinForReal commented Dec 24, 2021

nilo19 commented Dec 24, 2021

feiskyer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 27, 2021

MartinForReal commented Dec 22, 2021 •

edited

Loading

coveralls commented Dec 23, 2021 •

edited

Loading