Bug 1800425: More appropriate annotation name

[beekhof]

Since MHC has no control over how the machine is remediated, it would be
better not to imply that it will (only) be via reboot.

[beekhof]

/assign @frobware

[beekhof]

/retest

[openshift-ci-robot]

@beekhof: This pull request references Bugzilla bug 1800425, which is invalid:

• expected the bug to target the "4.4.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1800425: More appropriate annotation name

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.

[enxebre]

/retest

[beekhof]

/bugzilla refresh

@enxebre I was going to wait until the other PR merged and then rebase this one

[openshift-ci-robot]

@beekhof: This pull request references Bugzilla bug 1800425, which is invalid:

• expected the bug to target the "4.4.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

@enxebre I was going to wait until the other PR merged and then rebase this one

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.

[bison]

I agree with the intent here, but now the strategy name remains generic (reboot -> external) while the annotation is provider-specific: host.metal3.io/external-remediation

Could we make the strategy name match more closely?

Also, I forget, is this still feature gated in a way that blocks upgrades? If not, even if it's still tech-preview, can we update it like this? If I update an existing cluster that uses the reboot strategy, suddenly that's not recognized and I get the "delete" strategy with no warning.

[beekhof]

Rebased now that #475 has been merged.

I agree with the intent here, but now the strategy name remains generic (reboot -> external) while the annotation is provider-specific: host.metal3.io/external-remediation

Could we make the strategy name match more closely?

Are we just talking about changing
remediationStrategyExternal = mapiv1.RemediationStrategyType("external”)
to something like
remediationStrategyExternal = mapiv1.RemediationStrategyType(“baremetal-external”)
to indicate that this only works for baremetal?

Otherwise, the challenge of using machine.openshift.io/external-remediation is that metal3 see's openshift.io in the annotation name and thinks "that's openshift specific, we shouldn't be implementing that".

I don't think we can both have machine.openshift.io/external-remediation as a platform agnostic annotation name, and push the implementation onto providers that need to run elsewhere.

Also, I forget, is this still feature gated in a way that blocks upgrades? If not, even if it's still tech-preview, can we update it like this? If I update an existing cluster that uses the reboot strategy, suddenly that's not recognized and I get the "delete" strategy with no warning.

I don't believe it is feature gated in 4.3, but we've also never shipped anything that depended these annotations. That does not exclude the possibility that customers have dug into the MAO internals and implemented something - but are annotations held to the same versioning standards as CRDs? I thought that was part of the attraction of using annotations when the "final" interface is still an open question.

[beekhof]

Resolved goof when merging with #475
Remaining test failures seem to be from etcd-operator

[bison]

Are we just talking about changing
remediationStrategyExternal = mapiv1.RemediationStrategyType("external”)
to something like
remediationStrategyExternal = mapiv1.RemediationStrategyType(“baremetal-external”)
to indicate that this only works for baremetal?

Yeah, sorry, that wasn't very clear. More or less what you described, yeah. If different strategies result in provider specific annotations going onto Machine objects, I just think the RemediationStrategyType string should reflect the provider in some way. So baremetal-external or metal3 or whatever would be fine.

I don't believe it is feature gated in 4.3, but we've also never shipped anything that depended these annotations. That does not exclude the possibility that customers have dug into the MAO internals and implemented something - but are annotations held to the same versioning standards as CRDs? I thought that was part of the attraction of using annotations when the "final" interface is still an open question.

Fair enough. I think you're probably right. I mostly wanted to bring up that the current behavior for an unknown strategy is to just silently fallback to "delete the Machine", which I think is a separate issue. I'll file a bug on that.

[beekhof]

@bison happy enough with the change to give lgtm?

[bison]

/lgtm

[beekhof]

@enxebre what do you think, can we target this for 4.4?

[enxebre]

/bugzilla refresh

[openshift-ci-robot]

@enxebre: This pull request references Bugzilla bug 1800425, which is valid. The bug has been moved to the POST state.

In response to this:

/bugzilla refresh

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.

[enxebre]

/approve

[openshift-ci-robot]

[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

Needs approval from an approver in each of these files:

• pkg/controller/machinehealthcheck/OWNERS [enxebre]

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

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-ci-robot]

@beekhof: All pull requests linked via external trackers have merged. Bugzilla bug 1800425 has been moved to the MODIFIED state.

In response to this:

Bug 1800425: More appropriate annotation name

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.

[openshift-ci-robot]

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	0247b9c	link	/test e2e-aws-scaleup-rhel7

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.

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.