✨ Improve re-entrancy of device creation and delete workflows #209

detiber · 2020-12-04T21:51:45Z

What this PR does / why we need it:

Enables better re-entrancy for controller crashes during Machine creation and deletion

cpanato

/lgtm
/hold for maintainers to give a final look

I did the test running locally the e2e and it runs fine
will be good to we add the account in cncf so we can test this and become more confident.

cpanato · 2020-12-05T15:11:12Z

/assign @gianarb

controllers/packetmachine_controller.go

displague · 2020-12-06T15:26:29Z

controllers/packetmachine_controller.go

 		switch {
 		// TODO: find a better way than parsing the error messages for this.
-		case strings.Contains(err.Error(), " no available hardware reservations "):
+		case err != nil && strings.Contains(err.Error(), " no available hardware reservations "):


Why is this non-fatal? (I could use comments)

Does the provider API return an error even though the device will be created, making this more of a provider warning than a fatal error?
Is the HTTP response a 2xx with an errors block, perhaps? If so that may be a better way to detect this condition than string matching.

Updating the comment to provide more info, but if we do not special case this then Machine creation will fail after the first attempt and will not be retried (presence of ErrorMessage/ErrorReason in CAPI indicates a fatal error that should not be retried).

This was causing issues with users that wanted to run clusters on reserved hardware without having additional free reservations available during the upgrade, since attempts to create new Machines before the reservations were made available by Machines that were being deleted weren't immediately available.

Now in this block, we are going to try to adopt any existing device that carries the tags we would have placed in our previous attempt to create this device.

Is this because the create call can error before returning the UUID (perhaps a timeout), somehow despite the error the device creation may succeed, and we want to try to salvage the creation?

It mainly means that we haven't recorded the providerID for any reason. That could be:

k8s api call to patch the resource to set the providerID failed

controller crashed, preventing reconciliation from completing

api call to EM API to create device succeeded, but returned an error for some reason (could be timeout)

Thinking through this more, I should also update reconcileDelete to account for these use cases as well to avoid orphaning resources in the same cases as above.

displague

I suspect the updated reconcile logic in this PR may prevent any creates.

I also left some nitpicks about comments and the conditions used (before this PR and in this PR).

detiber · 2020-12-07T19:40:44Z

Moving the immediate fix for v0.3.6 to #212 to separate discussion about the reentrancy changes.

…reation process

…sult in a fatal error

gianarb · 2020-12-08T13:44:54Z

/lgtm

Thanks!!

displague · 2020-12-08T14:35:53Z

controllers/packetmachine_controller.go

+		}
+
+		if device != nil {
+			if err := r.deleteDevice(device.ID, force); err != nil {


Did you want to put the same http.StatusNotFound check here, or make deleteDevice include that logic?

I don't think it should be needed here. If by chance the device is deleted in between the list call made in GetDeviceByTags and the call to delete, then we'll just end up doing another reconciliation to remove the finalizer.

k8s-ci-robot · 2020-12-08T19:19:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, detiber, displague, gianarb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [detiber,gianarb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2021-03-08T20:14:10Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

fejta-bot · 2021-04-07T20:56:06Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

displague · 2021-04-20T21:19:48Z

/remove-lifecycle rotten

displague · 2021-05-05T23:10:26Z

controllers/packetmachine_controller.go

 		dev, err = r.PacketClient.GetDevice(providerID)
+		if err != nil {
+			var perr *packngo.ErrorResponse
+			if errors.As(err, &perr) && perr.Response != nil && perr.Response.StatusCode == http.StatusNotFound {


Perhaps we should also capture http.StatusForbidden for #245, unless the behavior has changed in the provider API.

k8s-triage-robot · 2021-08-03T23:46:15Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

k8s-triage-robot · 2021-09-02T23:57:11Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

detiber · 2021-09-08T19:19:27Z

Closing in favor of #269

detiber · 2021-09-08T19:20:00Z

/close

k8s-ci-robot · 2021-09-08T19:20:12Z

@detiber: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Fix error handling

b674928

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 4, 2020

k8s-ci-robot requested review from deitch and gianarb December 4, 2020 21:51

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2020

cpanato approved these changes Dec 5, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2020

k8s-ci-robot assigned cpanato Dec 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2020

k8s-ci-robot assigned gianarb Dec 5, 2020

displague reviewed Dec 6, 2020

View reviewed changes

controllers/packetmachine_controller.go Show resolved Hide resolved

displague reviewed Dec 6, 2020

View reviewed changes

controllers/packetmachine_controller.go Show resolved Hide resolved

displague reviewed Dec 6, 2020

View reviewed changes

displague suggested changes Dec 6, 2020

View reviewed changes

detiber force-pushed the doh branch from a0161cb to 1b2c144 Compare December 7, 2020 16:10

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2020

detiber force-pushed the doh branch from 1b2c144 to 7ded1e2 Compare December 7, 2020 16:25

displague approved these changes Dec 7, 2020

View reviewed changes

detiber added 3 commits December 7, 2020 14:43

Make PacketMachine controller re-entrant when crashes happen during c…

1b56beb

…reation process

inability to find device by id during normal reconciliation should re…

ff70dd7

…sult in a fatal error

Improve deletion when providerID is not set for some reason

c99c3d6

detiber force-pushed the doh branch from 7ded1e2 to c99c3d6 Compare December 7, 2020 19:44

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 7, 2020

detiber changed the title ~~🐛 Fix crash on Machine creation and improve re-entrancy~~ 🐛 Improve re-entrancy of device creation and delete workflows Dec 7, 2020

detiber changed the title ~~🐛 Improve re-entrancy of device creation and delete workflows~~ ✨ Improve re-entrancy of device creation and delete workflows Dec 7, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2020

displague reviewed Dec 8, 2020

View reviewed changes

gianarb approved these changes Dec 8, 2020

View reviewed changes

displague approved these changes Dec 8, 2020

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 8, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 7, 2021

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 20, 2021

displague reviewed May 5, 2021

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 3, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 2, 2021

k8s-ci-robot closed this Sep 8, 2021

✨ Improve re-entrancy of device creation and delete workflows #209

✨ Improve re-entrancy of device creation and delete workflows #209

Uh oh!

Conversation

detiber commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpanato left a comment

Choose a reason for hiding this comment

Uh oh!

cpanato commented Dec 5, 2020

Uh oh!

Uh oh!

Uh oh!

displague Dec 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

detiber Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

detiber Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

displague left a comment

Choose a reason for hiding this comment

Uh oh!

detiber commented Dec 7, 2020

Uh oh!

gianarb commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

displague Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

detiber Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Dec 8, 2020

Uh oh!

fejta-bot commented Mar 8, 2021

Uh oh!

fejta-bot commented Apr 7, 2021

Uh oh!

displague commented Apr 20, 2021

Uh oh!

displague May 5, 2021

Choose a reason for hiding this comment

Uh oh!

k8s-triage-robot commented Aug 3, 2021

Uh oh!

k8s-triage-robot commented Sep 2, 2021

Uh oh!

detiber commented Sep 8, 2021

Uh oh!

detiber commented Sep 8, 2021

Uh oh!

k8s-ci-robot commented Sep 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

detiber commented Dec 4, 2020 •

edited

Loading

displague Dec 6, 2020 •

edited

Loading

gianarb commented Dec 8, 2020 •

edited

Loading