Skip to content
This repository was archived by the owner on Aug 12, 2025. It is now read-only.

Conversation

@gianarb
Copy link
Contributor

@gianarb gianarb commented May 6, 2020

If we attempt to remove a machine that is not running on Packet (for any
reason), we are in a loop where the reconciler returns 404 from Packet
API and the source never gets deleted.

This code should check for a 404, and it should pass forward the
finalizer trigger a garbage collection.

I presume we do not need to check for the number of instances anymore,
because if they are 0 we never get over the not found error.

@gianarb gianarb requested a review from deitch May 6, 2020 12:15
Copy link
Contributor

@deitch deitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo and one minor change

}
return ctrl.Result{}, fmt.Errorf("error retrieving machine status %s: %v", packetmachine.Name, err)
}
if device == nil {

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

if err.(*packngo.ErrorResponse).Response != nil && err.(*packngo.ErrorResponse).Response.StatusCode == http.StatusNotFound {
// When the server does not exist we do not have anything left to do.
// Probably somebody manually deleted the server from the UI or via API.
logger.Info("Server not found, nothig left to do")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "nothig" should be "nothing"

@gianarb gianarb requested a review from deitch May 6, 2020 12:59
If we attempt to remove a machine that is not running on Packet (for any
reason), we are in a loop where the reconciler returns 404 from Packet
API and the source never gets deleted.

This code should check for a 404, and it should pass forward the
finalizer trigger a garbage collection.

I presume we do not need to check for the number of `instances` anymore,
because if they are `0` we never get over the `not found error`.
@deitch deitch merged commit 2cd36af into kubernetes-retired:v1alpha3 May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants