✨ Support range of values for unhealthy machines in machine health check spec #4128

shysank · 2021-01-27T23:53:23Z

What this PR does / why we need it:
Adds new field UnhealthyRange to specify unhealthy machine threshold as a range of values

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #4020

k8s-ci-robot · 2021-01-27T23:53:32Z

Hi @shysank. 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.

shysank · 2021-01-27T23:54:40Z

controllers/machinehealthcheck_controller_test.go

@@ -1677,7 +1832,7 @@ func TestIsAllowedRemediation(t *testing.T) {
 			maxUnhealthy:     nil,
 			expectedMachines: int32(3),
 			currentHealthy:   int32(0),
-			allowed:          true,
+			allowed:          false,


should MaxUnhealthy be defaulted in webhooks?

CecileRobertMichon

Can you please amend https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/proposals/20191030-machine-health-checking.md to reflect these changes?

CecileRobertMichon · 2021-01-29T01:18:43Z

/ok-to-test

wfernandes

We may also want to revise or update our documentation here (https://cluster-api.sigs.k8s.io/tasks/healthcheck.html#remediation-short-circuiting) to include information about the UnhealthyRange configuration.

wfernandes · 2021-02-01T23:32:35Z

api/v1alpha4/machinehealthcheck_types.go

+	// Any further remediation is only allowed if the number of machines selected by "selector" as not healthy
+	// is within the range of "UnhealthyRange"


I'd include a comment here that this property takes precedence over MaxUnhealthy.

wfernandes · 2021-02-02T20:46:57Z

Thanks for the updates @shysank !
/lgtm
/retest

vincepri · 2021-02-03T17:51:52Z

api/v1alpha4/machinehealthcheck_types.go

+	// Any further remediation is only allowed if the number of machines selected by "selector" as not healthy
+	// is within the range of "UnhealthyRange". Takes precedence over MaxUnhealthy.
+	// +optional
+	// +kubebuilder:validation:Pattern=^\[[0-9]+-[0-9]+\]$
+	UnhealthyRange *string `json:"unhealthyRange,omitempty"`


Could we provide an example on how to use the range in the comment?

Looking at the code below, is this range common in Kubernetes API? Do we have an example on how it's being used and implemented in other APIs?

The closest I could find in kubernetes was limit ranges. for cpu, but I'm not sure if that's what we really want. Another one was service-node-port-range in api server, but again that was very specific to ports.

shysank · 2021-02-03T19:40:48Z

/retest

vincepri · 2021-02-03T21:35:27Z

/retest

wfernandes · 2021-02-03T22:54:43Z

/lgtm

fabriziopandini · 2021-03-05T19:39:49Z

/milestone v0.4.0

shysank · 2021-03-09T06:17:26Z

/retest

fabriziopandini · 2021-03-09T10:32:07Z

/lgtm

controllers/machinehealthcheck_controller.go

CecileRobertMichon

minor nit, lgtm

…k spec

CecileRobertMichon · 2021-03-10T05:06:53Z

/lgtm
/approve

k8s-ci-robot · 2021-03-10T05:07:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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~~ [CecileRobertMichon]

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 27, 2021

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 27, 2021

k8s-ci-robot requested review from CecileRobertMichon and detiber January 27, 2021 23:53

shysank commented Jan 27, 2021

View reviewed changes

CecileRobertMichon reviewed Jan 28, 2021

View reviewed changes

shysank force-pushed the feature/4020 branch from 565c402 to f116294 Compare January 29, 2021 00:36

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 Jan 29, 2021

shysank force-pushed the feature/4020 branch from f116294 to 1cd69e7 Compare January 29, 2021 05:22

wfernandes reviewed Feb 1, 2021

View reviewed changes

shysank force-pushed the feature/4020 branch 2 times, most recently from 413ce9f to f2a8f95 Compare February 2, 2021 19:49

k8s-ci-robot assigned wfernandes Feb 2, 2021

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

vincepri requested changes Feb 3, 2021

View reviewed changes

shysank force-pushed the feature/4020 branch from f2a8f95 to fdd39cd Compare February 3, 2021 19:21

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2021

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

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2021

k8s-ci-robot added this to the v0.4.0 milestone Mar 5, 2021

shysank force-pushed the feature/4020 branch from fdd39cd to 07fd77c Compare March 8, 2021 19:47

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 8, 2021

k8s-ci-robot assigned fabriziopandini Mar 9, 2021

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

CecileRobertMichon reviewed Mar 9, 2021

View reviewed changes

controllers/machinehealthcheck_controller.go Outdated Show resolved Hide resolved

CecileRobertMichon reviewed Mar 9, 2021

View reviewed changes

support range of values for unhealthy machines in machine health chec…

7d53242

…k spec

shysank force-pushed the feature/4020 branch from 07fd77c to 7d53242 Compare March 10, 2021 04:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2021

k8s-ci-robot assigned CecileRobertMichon Mar 10, 2021

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

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

k8s-ci-robot merged commit d18ddcd into kubernetes-sigs:master Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Support range of values for unhealthy machines in machine health check spec #4128

✨ Support range of values for unhealthy machines in machine health check spec #4128

shysank commented Jan 27, 2021

k8s-ci-robot commented Jan 27, 2021

shysank Jan 27, 2021

CecileRobertMichon left a comment

CecileRobertMichon commented Jan 29, 2021

wfernandes left a comment

wfernandes Feb 1, 2021

wfernandes commented Feb 2, 2021

vincepri Feb 3, 2021

vincepri Feb 3, 2021

shysank Feb 3, 2021 •

edited

Loading

shysank commented Feb 3, 2021

vincepri commented Feb 3, 2021

wfernandes commented Feb 3, 2021

fabriziopandini commented Mar 5, 2021

shysank commented Mar 9, 2021

fabriziopandini commented Mar 9, 2021

CecileRobertMichon left a comment

CecileRobertMichon commented Mar 10, 2021

k8s-ci-robot commented Mar 10, 2021

		// Any further remediation is only allowed if the number of machines selected by "selector" as not healthy
		// is within the range of "UnhealthyRange"

✨ Support range of values for unhealthy machines in machine health check spec #4128

✨ Support range of values for unhealthy machines in machine health check spec #4128

Conversation

shysank commented Jan 27, 2021

k8s-ci-robot commented Jan 27, 2021

shysank Jan 27, 2021

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented Jan 29, 2021

wfernandes left a comment

Choose a reason for hiding this comment

wfernandes Feb 1, 2021

Choose a reason for hiding this comment

wfernandes commented Feb 2, 2021

vincepri Feb 3, 2021

Choose a reason for hiding this comment

vincepri Feb 3, 2021

Choose a reason for hiding this comment

shysank Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

shysank commented Feb 3, 2021

vincepri commented Feb 3, 2021

wfernandes commented Feb 3, 2021

fabriziopandini commented Mar 5, 2021

shysank commented Mar 9, 2021

fabriziopandini commented Mar 9, 2021

CecileRobertMichon left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented Mar 10, 2021

k8s-ci-robot commented Mar 10, 2021

shysank Feb 3, 2021 •

edited

Loading