Draft: Rewrite MachineSetNode controller to use qcontrollers #1158
Conversation
Signed-off-by: Orzelius <[email protected]>
| @@ -28,195 +30,246 @@ import ( | |||
| "github.com/siderolabs/omni/client/pkg/omni/resources/system" | |||
| ) | |||
|
|
|||
| // Questions for pr | |||
| // 1. which data is stored in the spec and which in the labels? | |||
|
|
|||
There was a problem hiding this comment.
This resource is used to extract the labels from any resource type.
In this case it's omni.MachineStatus labels.
As this controller doesn't use anything from the omni.MachineStatus spec, it can rely on the stripped down version of the resource.
There was a problem hiding this comment.
Okay I see, but is there a reason for why, for example, the evicted label shouldn't be an isEvicted field on the omni.MachineStatus spec?
Also, currently it seems that references to owner resources seem to be done using labels instead of spec fields.
For example:
- machineSets have a
LabelClusterinstead of having aclusterIdfield in the spec - machines resources have a
LabelMachineSetlabel instead of a aMachineSetIdor a similar field in the spec
Is there a reason for that? Is it due to avoid having confusing zero values in the spec as the labels can be deleted?
| }) | ||
| machineSetId, ok := machineSetNode.Metadata().Labels().Get(omni.LabelMachineSet) | ||
| if !ok { | ||
| // maybe should log a warning. Ask in pr |
There was a problem hiding this comment.
Logging would be nice to have. Can also return error here. It will also be logged.
|
In general: the tricky part about the QControllers is that all inputs for the controller should have finalizers added by the QController. So if the input is running we add the finalizer, then if the resource triggers reconcile and is tearing down, we should remove the finalizer. The problem is that without the finalizers it is not guaranteed that the delete operation comes to the controller. It will be unnoticed by it. |
|
Thank you for the review! Seems like this is not too far from the finish line so I'll take some time to try to finish this pr.
Okay I see, this was the part I was confused about. I'll try to figure it out on my own (will learn more that way :p) and implement the changes needed and will ping you again for feedback. Thank you for taking the time to explain! |
a WIP draft for the #556 issue