Backport the pause feature for MachineHealthChecks

[slintes]

We'd like to backport the pause feature for MachineHealthChecks from upstream SIG Cluster API,
see https://github.com/kubernetes-sigs/cluster-api/blob/37c862b5c9256d7ae65cd8ee8ddd7a103d51fe90/controllers/machinehealthcheck_controller.go#L134

[slintes]

Not sure about what values are good for the annotation. It's not being checked, but we might want to utilize it to sync between multiple parties who want to set this annotation?

[dhellmann]

This is a pretty subtle change to a doc to imply a new feature. Is there more to cover about testing or other aspects of the work that might be more clearly described in a new document?

[JoelSpeed]

I don't know that we need a new document, but certainly a new paragraph within the implementation details would be good to explain the pause feature

### Pause Annotation

To allow ... to achieve ... . We will implement ...

We do not care about the value, but the value can be used to ... 

[slintes]

fair point, good suggestion @JoelSpeed, thanks

@dhellmann would that work for you?

[dhellmann]

Sure, adding more details to the implementation section (and any others that might need to be expanded) here addresses my concerns.

[JoelSpeed]

Do we know upstream if the convention is to treat the annotation value as a singleton or a list? I could imagine it being a list (like finalizers) where each party is expected to add and remove its own annotation from the list, would be good to see if there's a note upstream about this behaviour so we can ensure we mimic that when we build controllers that will pause MHCs. That behaviour should be briefly identified here IMO

[slintes]

Unfortunately annotations are a map (while finalizers are a string array), so you can't compare them directly. Will ask upstream what their idea for the value is, and how to sync between multiple users.

[JoelSpeed]

Unfortunately annotations are a map (while finalizers are a string array), so you can't compare them directly

My thought was more the value itself could be a list, eg:

annotations:
  cluster-x.k8s.io/paused: "controller-1,controller-2,controller-3"

I don't know if this kind of thing is supported upstream or not, but I suspect they've documented the appropriate behaviour here

[slintes]

I asked on the cluster API Slack.
Today the value is meant to be empty.
The list of users for the annotation might be interesting, although it would require that patching is always strongly consistent, which can cause a lot of extra reconciliations to our controllers.

I'd suggest to also just leave the value empty for now, and handle a user list in a potential follow up. Does that make sense?

[JoelSpeed]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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 [JoelSpeed]

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

[JoelSpeed]

/hold for now, just want to double check with the rest of the Cluster Infra team that they're happy with this

[JoelSpeed]

/hold cancel
/lgtm