Backport MHC pause feature from cluster API #887
Conversation
pkg/util/annotations/helpers.go
Outdated
| // AddAnnotations sets the desired annotations on the object and returns true if the annotations have changed. | ||
| func AddAnnotations(o metav1.Object, desired map[string]string) bool { | ||
| if len(desired) == 0 { | ||
| return false | ||
| } | ||
| annotations := o.GetAnnotations() | ||
| if annotations == nil { | ||
| annotations = make(map[string]string) | ||
| o.SetAnnotations(annotations) | ||
| } | ||
| hasChanged := false | ||
| for k, v := range desired { | ||
| if cur, ok := annotations[k]; !ok || cur != v { | ||
| annotations[k] = v | ||
| hasChanged = true | ||
| } | ||
| } | ||
| return hasChanged | ||
| } |
There was a problem hiding this comment.
Is this used anywhere? Couldn't see it being used in the implementation
There was a problem hiding this comment.
I copied this package from upstream and removed everything unused. This one is used by the tests.
There was a problem hiding this comment.
The tests appear to be explicitly testing this function and not using any of the other functions we care about, right now I'd be tempted to delete those tests and this function as neither add anything from our use case as far as I can tell, do you agree?
There was a problem hiding this comment.
oh, I should look more carefully at the test code I copy. You're right. I remove the test file and the function
See openshift/enhancements#827 Signed-off-by: Marc Sluiter <msluiter@redhat.com>
slintes
force-pushed
the
pause-annotation
branch
from
74eae61
to
73720ee
Compare
| "github.com/openshift/machine-api-operator/pkg/util/external" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| "k8s.io/apimachinery/pkg/util/intstr" | ||
| "k8s.io/klog/v2" | ||
|
|
||
| mapiv1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" | ||
| "github.com/openshift/machine-api-operator/pkg/metrics" | ||
| "github.com/openshift/machine-api-operator/pkg/util/conditions" |
There was a problem hiding this comment.
@JoelSpeed when is the best time to do cleanups like this? As we go, separate commit, or separate PR?
There was a problem hiding this comment.
just for the record, this is not a cleanup only, I needed to add new import, and there was no clear place where to add it because of the mix of mapi and k8s imports in 2 sections 🙂
There was a problem hiding this comment.
I'd normally prefer separate commits, especially when it's larger stuff.
As this one is pretty small, and not too difficult to review, I'm happy with it as is.
My general preference in situations like this is to "prefactor", make all the changes to the existing code that are needed as the first few commits, then add the new stuff later, makes the review process as a whole a lot easier in my experience
|
/retest |
1 similar comment
|
/retest |
|
/approve |
|
[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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
|
Failing tests seem unrelated to the changes here and are failing for other PRs /lgtm |
|
/override e2e-aws-operator |
|
@slintes: slintes unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file. In response to this:
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. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/override ci/prow/e2e-aws-operator The test suite is permafailing on a broken test, we are working on a fix for this, I don't think there are any explicit problems presented by this PR |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/e2e-aws-operator In response to this:
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. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/override ci/prow/e2e-aws-operator The test suite is permafailing on a broken test, we are working on a fix for this, I don't think there are any explicit problems presented by this PR |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/e2e-aws-operator In response to this:
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. |
|
/test ci/prow/e2e-aws-upgrade |
|
@slintes: The specified target(s) for
Use
In response to this:
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. |
|
/test e2e-aws-upgrade |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
11 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required |
|
@slintes: The following tests failed, say
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. |
|
/override ci/prow/e2e-aws-operator The test suite is permafailing on a broken test, we are working on a fix for this, I don't think there are any explicit problems presented by this PR |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/e2e-aws-operator In response to this:
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. |
With this PR we backport the pause annotation for MachineHealthCheck resources from upstream SIG Cluster API. It can be used for e.g. pausing remediation during cluster upgrades. See openshift/enhancements#827
Upstream code:
https://github.com/kubernetes-sigs/cluster-api/blob/87270011c60fdd336949ad22dd1b3c59498b8480/controllers/machinehealthcheck_controller.go#L134
https://github.com/kubernetes-sigs/cluster-api/blob/a5e1b5fe003a29ec09a433cc5f6118e76993caa2/api/v1alpha4/common_types.go#L54