fix: set readyToUse: false only when the machine is dirty
#22
Conversation
utkuozdemir
force-pushed
the
fix-ready-to-use-flag
branch
from
d03af49 to
d68392c
Compare
|
|
||
| infraMachineStatus.TypedSpec().Value.ReadyToUse = false | ||
|
|
||
| return controller.NewRequeueInterval(1 * time.Second) |
There was a problem hiding this comment.
It will get persisted if we return nil here too. Why do we need to requeue?
There was a problem hiding this comment.
It is an overall improvement of sorts - I thought it's probably a better idea to only persist the new state in the happy path. To avoid unwanted modifications.
Nvm, didn't read the question properly. The actual reason is, after we mark the machine as "not ready to use", we trigger a reconcile, so that the next reconcile will not fall into this if condition, and the pending wipes/reboots will be processed.
But because we want the "not ready to use" flag to be updated immediately so in can be observed in Omni, we do "save & requeue".
| // the changes will trigger a new reconciliation, we can simply return here | ||
| return nil | ||
| return controller.NewRequeueErrorf(h.requeueInterval, "wiped the machine, requeue") |
There was a problem hiding this comment.
The comment no longer reflects the code.
Why do we need to requeue here?
There was a problem hiding this comment.
Removed the comment ✅ .
The reason is: I thought not persisting the changes if we have pending tasks would be "safer". But actually we don't need to return here at all, so I removed the return now.
utkuozdemir
force-pushed
the
fix-ready-to-use-flag
branch
2 times, most recently
from
c0960ed to
38c20e8
Compare
Change the logic of `infraMachineStatus.readyToUse` flag to represent the dirtiness state, i.e., if there is a pending wipe for that machine. Signed-off-by: Utku Ozdemir <[email protected]>
utkuozdemir
force-pushed
the
fix-ready-to-use-flag
branch
from
38c20e8 to
8227a3d
Compare
|
pulled in in #25, closing. |
Change the logic of
infraMachineStatus.readyToUseflag to represent the dirtiness state, i.e., if there is a pending wipe for that machine.Depends on siderolabs/omni#868 to pass tests.
Closes siderolabs/omni#850.