fix: report a missing instance as a final state - #19
Open
corioliskraft wants to merge 5 commits into
Open
Conversation
When the server of an already provisioned machine was gone, the reconcile used the reason InstanceError and returned an error, so the controller retried for ever and wrote an error line for a state that no retry changes. The provider now reports that state with its own error value. The reconcile turns it into the reason InstanceNotFound on the conditions and into a warning event, and it gives no error back to the controller runtime, so the retry stops. It still creates no replacement server.
The regression test for the gone server asserted this before. It now asserts the opposite, so a rejected VM creation covers it instead.
Author
|
Description updated, added trade-off paragraph. |
The spec drives a provisioned control plane machine into the terminal state of a missing instance, then deletes it while the cloud answers the server delete with not found. The load balancer target and the finalizer must go. The spec is the fourth caller of updateMachineControlPlaneLabel, so unparam reports the namespace parameter that always receives "default". The parameter has been refactored away, and the helper writes the namespace like the helper above it.
The delete path removes the finalizer only after the cloud reports the server as deleted or as already gone. The other exit had no spec: a deletion that answers with any other error must keep the object. The new spec injects a transient error into the server delete, then asserts that the reconcile returns that error, that the server is still there, and that the finalizer still holds the object, so the next attempt can find the instance ID.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #15
Today the reconcile of a
StackitMachinewhose server is gone sets the reasonInstanceErrorand returns an error, so the controller retries a state that noretry changes. With this change the
StackitMachinebecomes not ready with thereason
InstanceNotFound, and a warning event names the server. The reconcilegives no error back, so the retry loop stops. It still creates no replacement
server. The automatic replacement is out of scope here and is addressed by #7.
The trade-off: no error means no requeue, so a false
InstanceNotFounddoes notcorrect itself. The machine would then keep a false "not ready", and the
provider would not repair its load balancer target or its bastion SSH access,
until a watch event arrives, or in a quiet cluster until the cache resync, up to
10 hours. A false
InstanceNotFoundneeds the API to hide a live server fromthe lookup by ID and from the lookup by tags, so it is unlikely.
The terminal state keeps
Status.InstanceID, so the deletion of such a machinegoes to
DeleteServerand not to the early finalizer removal. Two new testscover that call:
cloud.ErrNotFoundstill removes the load balancer target andinfrav1.MachineFinalizer, and any other error keeps the object and itsfinalizer, so the next reconcile can repeat the delete. The not-found test is the
fourth caller of
updateMachineControlPlaneLabel, sounparamreported thenamespaceparameter that always receives"default", therefore this parameterwas refactored out. The helper now writes
"default"itself, likeupdateMachineBootstrapSecretdoes.