[OCPCLOUD-907] Add `remediationsAllowed` field to MHC status #652

JoelSpeed · 2020-07-21T16:36:33Z

This adds an remediationsAllowed field to the MHC status that tells a user how many more remediations will be allowed before the controller starts short circuiting remediations.

IE if the value of the field is 1, 1 more machine can be remediated before maxUnhealthy blocks the remediation. If the value is 0, no more remediations are allowed, the limit of maxUnhealthy has been reached.

This also adds a condition that shows whether remediation is currently considered to be allowed or not

This is the openshift counterpart to kubernetes-sigs/cluster-api#3372
/hold

Want to make sure we agree on the naming upstream before merging this

openshift-ci-robot · 2020-07-21T18:16:51Z

@JoelSpeed: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-upgrade	`f7ada32`	link	`/test e2e-aws-upgrade`
ci/prow/e2e-gcp-operator	`f7ada32`	link	`/test e2e-gcp-operator`
ci/prow/e2e-azure-operator	`f7ada32`	link	`/test e2e-azure-operator`

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 · 2020-10-19T14:21:29Z

/hold cancel

I think things have stabilised on this feature upstream, this PR now matches what is being done upstream and as such I think is good to go into 4.7

JoelSpeed · 2020-10-20T09:36:28Z

/retest

elmiko

this generally makes sense to me, i just had a minor question

elmiko · 2020-10-20T20:58:39Z

pkg/apis/machine/v1beta1/condition_types.go

+
+// ANCHOR: Condition
+
+// Condition defines an observation of a Cluster API resource operational state.


i know this shares some ancestry with upstream, should we say "Machine API" in our version here?

elmiko · 2020-10-20T20:59:15Z

pkg/apis/machine/v1beta1/condition_types.go

+
+// ANCHOR: Conditions
+
+// Conditions provide observations of the operational state of a Cluster API resource.


same thing here, re: Machine API

JoelSpeed · 2020-10-23T16:00:20Z

@elmiko I fixed your comments and a couple of other issues, please take another look

elmiko

thanks Joel, i went over the code again and just had a question about something that didn't make immediate sense to me.

elmiko · 2020-10-23T16:48:07Z

pkg/controller/machinehealthcheck/machinehealthcheck_controller.go

+		)
+
+		// Remediation not allowed, the number of not started or unhealthy machines exceeds maxUnhealthy
+		mhc.Status.RemediationsAllowed = 0


does this get updated on the object in etcd at some point?

Good catch, I've moved the reconcile status so it happens for all code paths, ideally I would spend time to do a bigger refactor of this logic so it's more obvious what's going on here

I will make sure to update the tests before we merge this

JoelSpeed · 2020-10-28T16:50:24Z

/retest

elmiko

this continues to make sense for me, i just had a small question

elmiko · 2020-10-28T19:45:13Z

pkg/controller/machinehealthcheck/machinehealthcheck_controller.go

+// unhealthyMachineCount calculates the number of presently unhealthy or missing machines
+// ie the delta between the expected number of machines and the current number deemed healthy
+func unhealthyMachineCount(mhc *mapiv1.MachineHealthCheck) int {
+	return derefInt(mhc.Status.ExpectedMachines) - derefInt(mhc.Status.CurrentHealthy)


is it possible that either of these derefInt calls could fail (eg the value does not exist)?

I think it will default to 0 if the deref fails (I think that's the point of the function), but no matter the behaviour, this hasn't actually changed from before this PR, just moved

ok perfect. thanks!

…eck controller

elmiko · 2020-10-30T12:40:58Z

/approve

openshift-ci-robot · 2020-10-30T12:41:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

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

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

JoelSpeed · 2020-11-10T17:42:29Z

/retest

alexander-demicev

/lgtm

JoelSpeed · 2020-11-16T11:13:17Z

/retest

JoelSpeed · 2020-11-17T12:40:16Z

/retest

JoelSpeed · 2020-11-18T10:14:38Z

/retest

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2020

openshift-ci-robot requested review from beekhof and paulfantom July 21, 2020 16:36

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

JoelSpeed force-pushed the allowed-remediations branch from f7ada32 to 726603f Compare October 19, 2020 14:19

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2020

JoelSpeed changed the title ~~[OCPCLOUD-907] Add allowedRemediations field to MHC status~~ [OCPCLOUD-907] Add remediationsAllowed field to MHC status Oct 19, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 19, 2020

elmiko reviewed Oct 20, 2020

View reviewed changes

JoelSpeed force-pushed the allowed-remediations branch from 726603f to e6613a5 Compare October 23, 2020 15:59

elmiko reviewed Oct 23, 2020

View reviewed changes

JoelSpeed force-pushed the allowed-remediations branch from f659c2f to 164e0e9 Compare October 28, 2020 14:54

elmiko reviewed Oct 28, 2020

View reviewed changes

JoelSpeed added 5 commits October 30, 2020 11:17

Add RemediationsAllowed field to the MachineHealthCheck status

1bcdf67

Add conditions Getter/Setter utilities

25edfeb

Set RemediationsAllowed and Conditions on Status from MachineHealthCh…

9da1a78

…eck controller

Ensure status is reconciled after update

bdb06c3

Ensure status of MHC is checked in reconcile tests

5431a50

JoelSpeed force-pushed the allowed-remediations branch from 164e0e9 to 5431a50 Compare October 30, 2020 11:23

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2020

alexander-demicev approved these changes Nov 13, 2020

View reviewed changes

openshift-ci-robot assigned alexander-demicev Nov 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2020

openshift-merge-robot merged commit 7750896 into openshift:master Nov 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OCPCLOUD-907] Add `remediationsAllowed` field to MHC status #652

[OCPCLOUD-907] Add `remediationsAllowed` field to MHC status #652

JoelSpeed commented Jul 21, 2020 •

edited

openshift-ci-robot commented Jul 21, 2020

JoelSpeed commented Oct 19, 2020

JoelSpeed commented Oct 20, 2020

elmiko left a comment

elmiko Oct 20, 2020

elmiko Oct 20, 2020

JoelSpeed commented Oct 23, 2020

elmiko left a comment

elmiko Oct 23, 2020

JoelSpeed Oct 26, 2020

JoelSpeed commented Oct 28, 2020

elmiko left a comment

elmiko Oct 28, 2020

JoelSpeed Oct 30, 2020

elmiko Oct 30, 2020

elmiko commented Oct 30, 2020

openshift-ci-robot commented Oct 30, 2020

JoelSpeed commented Nov 10, 2020

alexander-demicev left a comment

JoelSpeed commented Nov 16, 2020

JoelSpeed commented Nov 17, 2020

JoelSpeed commented Nov 18, 2020


		// ANCHOR: Condition

		// Condition defines an observation of a Cluster API resource operational state.


		// ANCHOR: Conditions

		// Conditions provide observations of the operational state of a Cluster API resource.

[OCPCLOUD-907] Add remediationsAllowed field to MHC status #652

[OCPCLOUD-907] Add remediationsAllowed field to MHC status #652

Conversation

JoelSpeed commented Jul 21, 2020 • edited

openshift-ci-robot commented Jul 21, 2020

JoelSpeed commented Oct 19, 2020

JoelSpeed commented Oct 20, 2020

elmiko left a comment

Choose a reason for hiding this comment

elmiko Oct 20, 2020

Choose a reason for hiding this comment

elmiko Oct 20, 2020

Choose a reason for hiding this comment

JoelSpeed commented Oct 23, 2020

elmiko left a comment

Choose a reason for hiding this comment

elmiko Oct 23, 2020

Choose a reason for hiding this comment

JoelSpeed Oct 26, 2020

Choose a reason for hiding this comment

JoelSpeed commented Oct 28, 2020

elmiko left a comment

Choose a reason for hiding this comment

elmiko Oct 28, 2020

Choose a reason for hiding this comment

JoelSpeed Oct 30, 2020

Choose a reason for hiding this comment

elmiko Oct 30, 2020

Choose a reason for hiding this comment

elmiko commented Oct 30, 2020

openshift-ci-robot commented Oct 30, 2020

JoelSpeed commented Nov 10, 2020

alexander-demicev left a comment

Choose a reason for hiding this comment

JoelSpeed commented Nov 16, 2020

JoelSpeed commented Nov 17, 2020

JoelSpeed commented Nov 18, 2020

[OCPCLOUD-907] Add `remediationsAllowed` field to MHC status #652

[OCPCLOUD-907] Add `remediationsAllowed` field to MHC status #652

JoelSpeed commented Jul 21, 2020 •

edited