Bug 1943378: Fix InstanceCreate volume cleanup #191

pierreprinetti · 2021-07-21T13:04:49Z

With this change, the volume cleanup is triggered by any early failure
of InstanceCreate. That is to say: if InstanceCreate creates a volume,
that volume will be deleted if any of the next steps fail and cause the
server never to be created.

openshift-ci · 2021-07-21T13:05:04Z

@pierreprinetti: This pull request references Bugzilla bug 1943378, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @eurijon

In response to this:

Bug 1943378: Fix InstanceCreate volume cleanup

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.

openshift-ci · 2021-07-21T13:05:05Z

@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: eurijon.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@pierreprinetti: This pull request references Bugzilla bug 1943378, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)

bug target release (4.9.0) matches configured target release for branch (4.9.0)

bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @eurijon

In response to this:

Bug 1943378: Fix InstanceCreate volume cleanup

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.

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.

mandre · 2021-07-22T08:09:18Z

pkg/cloud/openstack/clients/machineservice.go

 	}

+	// Turn off deferred cleanup
+	instanceCreateSuccessful = true


Typically you don't want to "disable" a destructive action in the happy path, but want to do it the other way around: enable the cleanup when something goes wrong.

One easy way to do it would be to rename the variable to something like needVolumeCleanup and set it to true while handling the server creation failure. This also has the nice side effect of removing the negative condition in the cleanup check (easier to read).

pkg/cloud/openstack/clients/machineservice.go

pierreprinetti · 2021-07-22T12:31:37Z

Is it a bit clearer now?

I don't dislike comments by the way.

mandre · 2021-07-22T12:50:45Z

pkg/cloud/openstack/clients/machineservice.go

 func (is *InstanceService) InstanceCreate(clusterName string, name string, clusterSpec *openstackconfigv1.OpenstackClusterProviderSpec, config *openstackconfigv1.OpenstackProviderSpec, cmd string, keyName string, configClient configclient.ConfigV1Interface) (instance *Instance, err error) {
+	// This variable is used in early returns to trigger the deletion of
+	// associated resources in case of failure to create the server.
+	needsCleanup := true


Why initializing needsCleanup to true? Initially, we don't need cleanup - it's only after failing to provision the node.

pkg/cloud/openstack/clients/machineservice.go

With this change, the volume cleanup is triggered by any early failure of InstanceCreate. That is to say: if InstanceCreate creates a volume, that volume will be deleted if any of the next steps fail and cause the server never to be created.

openshift-ci · 2021-07-22T13:17:52Z

@pierreprinetti: This pull request references Bugzilla bug 1943378, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @eurijon

In response to this:

Bug 1943378: Fix InstanceCreate volume cleanup

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.

openshift-ci · 2021-07-22T13:17:53Z

@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: eurijon.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@pierreprinetti: This pull request references Bugzilla bug 1943378, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)

bug target release (4.9.0) matches configured target release for branch (4.9.0)

bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @eurijon

In response to this:

Bug 1943378: Fix InstanceCreate volume cleanup

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.

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.

mandre

/lgtm
/hold pending CI

pierreprinetti · 2021-07-22T13:24:20Z

Clarified out-of-band.

I have changed the commit (and PR) description to explain the problem I'm trying to solve, then reset to a version @mandre and I found more talkative. Then seasoned with comments.

mandre · 2021-07-22T15:52:32Z

/hold cancel

mandre · 2021-07-22T15:57:32Z

/approve

openshift-ci · 2021-07-22T15:57:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandre

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~~ [mandre]

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

openshift-ci · 2021-07-22T15:58:31Z

@pierreprinetti: All pull requests linked via external trackers have merged:

Bugzilla bug 1943378 has been moved to the MODIFIED state.

In response to this:

Bug 1943378: Fix InstanceCreate volume cleanup

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.

* improve generator readme * add prerequisites section * improve readme

openshift-ci bot requested review from iamemilio and mdbooth July 21, 2021 13:04

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jul 21, 2021

pierreprinetti force-pushed the bz1943378-fixup branch from bbef234 to f161d8c Compare July 22, 2021 07:48

mandre reviewed Jul 22, 2021

View reviewed changes

pierreprinetti force-pushed the bz1943378-fixup branch 4 times, most recently from 8038781 to 42bd7c7 Compare July 22, 2021 09:20

mandre reviewed Jul 22, 2021

View reviewed changes

pkg/cloud/openstack/clients/machineservice.go Show resolved Hide resolved

pierreprinetti force-pushed the bz1943378-fixup branch from 42bd7c7 to ba4c79c Compare July 22, 2021 12:30

pierreprinetti force-pushed the bz1943378-fixup branch from ba4c79c to 944f18a Compare July 22, 2021 12:34

mandre reviewed Jul 22, 2021

View reviewed changes

pierreprinetti force-pushed the bz1943378-fixup branch from 944f18a to 46d9b4c Compare July 22, 2021 13:14

pierreprinetti force-pushed the bz1943378-fixup branch from 46d9b4c to b44d7d7 Compare July 22, 2021 13:17

mandre reviewed Jul 22, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021

openshift-ci bot assigned mandre Jul 22, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2021

openshift-merge-robot merged commit 58f8692 into openshift:master Jul 22, 2021

pierreprinetti deleted the bz1943378-fixup branch July 22, 2021 15:58

EmilienM mentioned this pull request Jan 31, 2023

Manual rebase on upstream #252

Closed

pierreprinetti pushed a commit to shiftstack/cluster-api-provider-openstack that referenced this pull request Apr 22, 2024

Improve generator readme (openshift#191)

53d4c05

* improve generator readme * add prerequisites section * improve readme

Bug 1943378: Fix InstanceCreate volume cleanup #191

Bug 1943378: Fix InstanceCreate volume cleanup #191

Uh oh!

Conversation

pierreprinetti commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jul 21, 2021

Uh oh!

openshift-ci bot commented Jul 21, 2021

Uh oh!

mandre Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pierreprinetti commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mandre Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci bot commented Jul 22, 2021

Uh oh!

openshift-ci bot commented Jul 22, 2021

Uh oh!

mandre left a comment

Choose a reason for hiding this comment

Uh oh!

pierreprinetti commented Jul 22, 2021

Uh oh!

mandre commented Jul 22, 2021

Uh oh!

mandre commented Jul 22, 2021

Uh oh!

openshift-ci bot commented Jul 22, 2021

Uh oh!

openshift-ci bot commented Jul 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pierreprinetti commented Jul 21, 2021 •

edited

Loading

pierreprinetti commented Jul 22, 2021 •

edited

Loading