add MachineConfiguration conditions for boot image updates #1882
Conversation
|
Hello @djoshy! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/S
|
I do have an open question. If boot image updates are disabled(which is possible via the opt-in mechanism), none of the conditions are exactly applicable. Should I make a specific condition for the "Off" state, and set it to true? |
djoshy
force-pushed
the
machine-configuration-conditions
branch
from
35f03f4 to
258c414
Compare
| @@ -494,3 +494,19 @@ const ( | |||
| // Special represents an action that is internal to the MCO, and is not allowed in user defined NodeDisruption policies. | |||
| SpecialStatusAction NodeDisruptionPolicyStatusActionType = "Special" | |||
| ) | |||
|
|
|||
| // These strings will be used to indicate success and failures for MachineConfiguration knobs via conditions. | |||
| // Please note: These are subject to change. Both additions and deletions may be made. | |||
There was a problem hiding this comment.
The cost of having these in openshift/api is going to be, "will not be removed, semantically changed, or abandoned". I won't say it's completely impossible to remove them, but the impact to user scripts is pretty extreme.
There was a problem hiding this comment.
Took this bit off, I don't envision us removing this in the future...I shouldn't have stolen this boilerplate from elsewhere :P
| // +openshift:enable:FeatureGate=ManagedBootImages | ||
| MachineConfigurationBootImageUpdateFailed string = "BootImageUpdateFailed" | ||
|
|
||
| // MachineConfigurationBootImageUpdateSuccess means that the MCO was able to successfully update all boot images. |
There was a problem hiding this comment.
is there a generation involved here? If a new bootimage is created, this immediately becomes false?
There was a problem hiding this comment.
I know we've removed the "Success" condition as it is seemed redundant, but for the 2 remainder states the generation of the boot images configmap will be included in every condition update as we discussed
|
Chatted with @deads2k on slack, and we boiled this down to two conditions:
All of them should include progress counts of the machinesets, and the generation info of the source configmap. If neither of these conditions are true, this means that the controller is in a happy/done state. |
djoshy
force-pushed
the
machine-configuration-conditions
branch
from
258c414 to
051406b
Compare
Boiling it down to two conditions has helped with this too; as there we are no longer tracking a success state, the other two conditions being false feels valid when the feature is "off" as well. Also to make a note: The MCO team will consider implementing something like PoolSynchronizerStatus for this in the future |
|
@djoshy: all tests passed! 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. |
|
/lgtm |
|
/tide refresh |
deads2k
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, djoshy 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 |
This PR adds a few new
MachineConfigurationconditions, which will be used by the MSBIC to inform the user of the results of the boot image reconciliation loop. These conditions will be behind the ManagedBootImages feature gate.More info: