fix: report a missing instance as a final state - #19
Merged
Merged
Conversation
Contributor
Author
|
Description updated, added trade-off paragraph. |
Collaborator
|
@corioliskraft sorry for the lack of response but I was on vacation until today and this project is currently still in active development internally. Therefore we weren't expecting contributions yet 😅 I'll have a look at your changes in more detail |
tuunit
force-pushed
the
fix/instance-not-found
branch
2 times, most recently
from
September 18, 2026 12:14
1942e96 to
6b2bebd
Compare
tuunit
reviewed
Sep 21, 2026
Collaborator
There was a problem hiding this comment.
I added a new context/scope for tests that have bootstrap data and tests that don't have bootstrap data. Therefore this file has a lot of indentation changes but the focus was on scoping and reducing the duplication of test initialization code
tuunit
reviewed
Sep 21, 2026
| err.Error(), | ||
| infrav1.MachineInstanceReadyCondition, | ||
| infrav1.MachineReadyCondition, | ||
| ) |
Collaborator
There was a problem hiding this comment.
we thought about if this should be set to Unknown but I decided against it because Unknown should be reserved to bugs, invalid responses from the provider but in this case we have a valid response that tells us a instance has not been found
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. test: cover the error return of the machine reconcile The regression test for the gone server asserted this before. It now asserts the opposite, so a rejected VM creation covers it instead. test: cover the deletion of a machine whose server is gone 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. test: cover a failed server delete during machine deletion 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. test: drop the excessive comment of the failed delete spec
…th bootstrap data and tests without Signed-off-by: Jan Larwig <jan.larwig@digits.schwarz>
Signed-off-by: Jan Larwig <jan.larwig@digits.schwarz>
tuunit
force-pushed
the
fix/instance-not-found
branch
from
September 21, 2026 08:45
0266fc5 to
c4dea47
Compare
tuunit
approved these changes
Sep 21, 2026
Collaborator
|
Test evidence: 
|
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
2 participants
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.