Add short-circuiting support and drop MDB by enxebre · Pull Request #414 · openshift/machine-api-operator

enxebre · 2019-10-17T10:23:11Z

Introduce support for short-circuiting based on: https://github.com/openshift/enhancements/blob/master/enhancements/machine-health-checking.md#goals

Introduce spec.MaxUnhealthy (int/string with percentage)
Introduce status.ExpectedMachines
Introduce status.CurrentHealthy
Every new function introduced is unit tested

The MHC does a best effort to wait for the cluster to stabilise and to ensure that no farther remediation takes place for a targeted pool of machines once its MaxUnhealthy threshold is reached. This is similar to what the node life cycle controller does for reducing the eviction rate as nodes goes unhealthy in a given zone.

Initial

I1017 11:52:39.074373   45304 machinehealthcheck_controller.go:97] Reconciling openshift-machine-api/example
I1017 11:52:39.074444   45304 machinehealthcheck_controller.go:110] Reconciling openshift-machine-api/example: finding targets
I1017 11:52:39.074697   45304 machinehealthcheck_controller.go:199] Reconciling openshift-machine-api/example/agl1610192-prs4g-worker-us-east-1a-sd962/ip-10-0-140-128.ec2.internal: health checking
I1017 11:52:40.328357   45304 machinehealthcheck_controller.go:134] Reconciling openshift-machine-api/example: monitoring mhc: total targets: 1,  maxUnavailable: 40%, unhealthy: 0. Remediations are allowed
I1017 11:52:40.328442   45304 machinehealthcheck_controller.go:162] Reconciling openshift-machine-api/example: no targets meet unhealthy criteria

During a scale out

I1017 11:55:00.546021   45359 machinehealthcheck_controller.go:97] Reconciling openshift-machine-api/example
I1017 11:55:00.546107   45359 machinehealthcheck_controller.go:110] Reconciling openshift-machine-api/example: finding targets
I1017 11:55:00.546643   45359 machinehealthcheck_controller.go:199] Reconciling openshift-machine-api/example/agl1610192-prs4g-worker-us-east-1a-sd962/ip-10-0-140-128.ec2.internal: health checking
I1017 11:55:00.546678   45359 machinehealthcheck_controller.go:199] Reconciling openshift-machine-api/example/agl1610192-prs4g-worker-us-east-1a-f4jl6/: health checking
I1017 11:55:00.546696   45359 machinehealthcheck_controller.go:213] Reconciling openshift-machine-api/example/agl1610192-prs4g-worker-us-east-1a-f4jl6/: is likely to go unhealthy in 9m2.453312s
I1017 11:55:00.546713   45359 machinehealthcheck_controller.go:199] Reconciling openshift-machine-api/example/agl1610192-prs4g-worker-us-east-1a-sthfm/: health checking
I1017 11:55:00.546745   45359 machinehealthcheck_controller.go:213] Reconciling openshift-machine-api/example/agl1610192-prs4g-worker-us-east-1a-sthfm/: is likely to go unhealthy in 8m58.453267s
I1017 11:55:00.546768   45359 machinehealthcheck_controller.go:199] Reconciling openshift-machine-api/example/agl1610192-prs4g-worker-us-east-1a-rc6cw/: health checking
I1017 11:55:00.546801   45359 machinehealthcheck_controller.go:213] Reconciling openshift-machine-api/example/agl1610192-prs4g-worker-us-east-1a-rc6cw/: is likely to go unhealthy in 9m0.453209s
I1017 11:55:00.546888   45359 machinehealthcheck_controller.go:199] Reconciling openshift-machine-api/example/agl1610192-prs4g-worker-us-east-1a-ljkqs/: health checking
I1017 11:55:00.546910   45359 machinehealthcheck_controller.go:213] Reconciling openshift-machine-api/example/agl1610192-prs4g-worker-us-east-1a-ljkqs/: is likely to go unhealthy in 8m56.453099s
W1017 11:55:00.702954   45359 machinehealthcheck_controller.go:126] Reconciling openshift-machine-api/example: total targets: 5,  maxUnhealthy: 40%, unhealthy: 4. Short-circuiting remediation

Manifest using percentage

kubectl get mhc example -nopenshift-machine-api -oyaml
apiVersion: healthchecking.openshift.io/v1alpha1
kind: MachineHealthCheck
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"healthchecking.openshift.io/v1alpha1","kind":"MachineHealthCheck","metadata":{"annotations":{},"name":"example","namespace":"openshift-machine-api"},"spec":{"maxUnhealthy":"40%","selector":{"matchLabels":{"machine.openshift.io/cluster-api-cluster":"agl1610192-prs4g","machine.openshift.io/cluster-api-machine-role":"worker","machine.openshift.io/cluster-api-machine-type":"worker","machine.openshift.io/cluster-api-machineset":"agl1610192-prs4g-worker-us-east-1a"}},"unhealthyConditions":[{"status":"Unknown","timeout":"300s","type":"Ready"},{"status":"False","timeout":"300s","type":"Ready"}]}}
  creationTimestamp: "2019-10-16T18:13:52Z"
  generation: 3
  name: example
  namespace: openshift-machine-api
  resourceVersion: "303870"
  selfLink: /apis/healthchecking.openshift.io/v1alpha1/namespaces/openshift-machine-api/machinehealthchecks/example
  uid: 3261c3f0-025f-4866-b4b9-0fff711a59d7
spec:
  maxUnhealthy: 40%
  selector:
    matchLabels:
      machine.openshift.io/cluster-api-cluster: agl1610192-prs4g
      machine.openshift.io/cluster-api-machine-role: worker
      machine.openshift.io/cluster-api-machine-type: worker
      machine.openshift.io/cluster-api-machineset: agl1610192-prs4g-worker-us-east-1a
  unhealthyConditions:
  - status: Unknown
    timeout: 300s
    type: Ready
  - status: "False"
    timeout: 300s
    type: Ready
status:
  currentHealthy: 4
  expectedMachines: 5

beekhof · 2019-10-17T11:24:48Z

Introduce spec.MaxUnhealthy (int/string with percentage)

I like this in theory, however won't the act of remediating (and therefor deleting) a node reduce the value of Status.ExpectedMachines and render the value of spec.MaxUnhealthy meaningless?

beekhof · 2019-10-17T11:33:53Z

One other nice aspect of MDB was the ability for multiple interested parties to specify the pools of machines they were interested in and what the availability requirements where.

For example, the system as a whole might tolerate up to 50% of the nodes being fenced, Ceph might only allow 1. MDB respected the most restrictive requirement without requiring any co-ordination between the two pieces of software.

Thinking more, it also allows something like Ceph (when running across masters and workers) to limit spec.MaxUnhealthy to 1 for the whole system, but MAO to use reboot for masters (since they can't be reprovisioned yet) and reprovisioning for workers.

Maybe these use cases are not strong enough to justify preserving the functionality, but its good for us to decide that explicitly :)

enxebre · 2019-10-17T11:41:56Z

/test e2e-azure-operator

enxebre · 2019-10-17T12:08:35Z

hey @beekhof

I like this in theory, however won't the act of remediating (and therefor deleting) a node reduce the value of Status.ExpectedMachines and render the value of spec.MaxUnhealthy meaningless?

That's extremely unlikely to happen as soon as a deletionTimestamp is given to a machine a new one is persisted by the controller, so most of the times there's actually overlapping making the restriction harder.
Also if that would ever happen to be the case and ExpectedMachines would be temporary reduced, machines wouldn't just get remediated right away they still would need to wait for the timeouts.

That said this is as stated in the desc "best effort to wait for the cluster to stabilise and to ensure that no farther remediation takes place for a targeted pool of machines once its MaxUnhealthy threshold is reached. This is similar to what the node life cycle controller does for reducing the eviction rate as nodes goes unhealthy in a given zone." which implements https://github.com/openshift/enhancements/blob/master/enhancements/machine-health-checking.md

For your first use case I'm not sure how that'd work with the current mdb implementation.
That said I definitely see benefits for mdb, please if you are interested in pursueing this for later releases it'd be very useful to have all the use cases written down and have an enhacements PR to discuss through. For now we prefer to go simpler and keep mdb feature gated which means we can have both and you can still leveraged it for your use cases.

beekhof · 2019-10-17T12:19:28Z

Thanks for clarifying the ExpectedMachines behaviour, that seems reasonable and IIUC metal (with its long reboot times) won't see any problems either.

The immediate driver for the extra things MDB did (Hyperconverged Ceph) doesn't seem to be as important anymore. I'm happy to stick with this PR and base the next round of changes on feedback from the field.

enxebre · 2019-10-17T12:21:34Z

/test e2e-azure-operator

enxebre · 2019-10-17T14:05:27Z

/test e2e-azure-operator

enxebre · 2019-10-17T14:10:24Z

/test govet

enxebre · 2019-10-17T14:25:21Z

/test e2e-aws-upgrade

enxebre · 2019-10-17T14:28:55Z

/test e2e-azure-operator

enxebre · 2019-10-17T14:33:01Z

/retest

enxebre · 2019-10-17T14:46:28Z

/retest

enxebre · 2019-10-17T15:02:00Z

/retest

enxebre · 2019-10-17T15:23:45Z

/retest

enxebre · 2019-10-17T15:30:42Z

/retest

enxebre · 2019-10-17T15:36:18Z

/retest

enxebre · 2019-10-17T15:41:55Z

/retest

enxebre · 2019-10-17T15:44:01Z

/retest

enxebre · 2019-10-17T15:45:43Z

/retest

enxebre · 2019-10-17T15:48:01Z

/retest

enxebre · 2019-10-17T16:00:02Z

/retest

enxebre · 2019-10-17T16:05:41Z

/retest

enxebre · 2019-10-17T16:17:21Z

/retest

enxebre · 2019-10-17T16:28:20Z

/retest

enxebre · 2019-10-17T21:51:06Z

/retest

enxebre · 2019-10-17T21:54:18Z

/retest

enxebre · 2019-10-18T06:59:44Z

/retest

openshift-ci-robot · 2019-10-18T08:00:53Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`e338576`	link	`/test e2e-aws-scaleup-rhel7`
ci/prow/e2e-azure-operator	`e338576`	link	`/test e2e-azure-operator`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

enxebre · 2019-10-18T08:36:56Z

This is a follow up for #412
I'm fast merging this to enable farther development and put up the PRs for:
Move strategy field into an annotation #415
Remove feature gate #416

/approve

openshift-ci-robot · 2019-10-18T08:37:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [enxebre]

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

Add short-circuiting support and drop MDB

e338576

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 17, 2019

openshift-ci-robot requested review from beekhof and spangenberg October 17, 2019 10:23

enxebre mentioned this pull request Oct 17, 2019

Drop strategy field in favour of annotation #415

Merged

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

enxebre merged commit 806e47a into openshift:master Oct 18, 2019

Uh oh!

Conversation

enxebre commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beekhof commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beekhof commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beekhof commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 17, 2019

Uh oh!

enxebre commented Oct 18, 2019

Uh oh!

openshift-ci-robot commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enxebre commented Oct 18, 2019

Uh oh!

openshift-ci-robot commented Oct 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

enxebre commented Oct 17, 2019 •

edited

Loading

beekhof commented Oct 17, 2019 •

edited

Loading

enxebre commented Oct 17, 2019 •

edited

Loading

openshift-ci-robot commented Oct 18, 2019 •

edited

Loading