Skip to content

Bug 1943378: Eliminate instanceCreate volume leak #188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Jul 21, 2021
Merged

Bug 1943378: Eliminate instanceCreate volume leak #188

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion pkg/cloud/openstack/clients/machineservice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,8 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust
}
}

var cleanupOperationsInCaseOfServerCreationFailure []func() error

// Set default Tags
machineTags := []string{
"cluster-api-provider-openstack",
Expand Down Expand Up @@ -730,6 +732,9 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust
return nil, fmt.Errorf("Create bootable volume err: %v", err)
}

cleanupOperationsInCaseOfServerCreationFailure = append(cleanupOperationsInCaseOfServerCreationFailure, func() error {
return volumes.Delete(is.volumeClient, volume.ID, nil).ExtractErr()
Copy link

@mdbooth mdbooth Jul 21, 2021

Choose a reason for hiding this comment

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

This differs from the delete line below:

err = volumes.Delete(is.volumeClient, volumeID, volumes.DeleteOpts{}).ExtractErr()

Have you managed to test it manually?

Copy link

@mdbooth mdbooth Jul 21, 2021

Choose a reason for hiding this comment

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

There's also at least 1 other error condition on line 777 which, if it failed, would result in the same leak.

There might be a slightly more robust solution. How about something like:

defer func() error {
    if server == nil {
        err = volumes.Delete(is.volumeClient, volumeID, volumes.DeleteOpts{}).ExtractErr()
        ... log any error
    }
}

Copy link
Member Author

@pierreprinetti pierreprinetti Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I tested manually and it works in the case in point (quota error).

However the defer solution is more powerful (cover other cases as well) and obviously better. I'm back to the keyboard.

Copy link
Member Author

@pierreprinetti pierreprinetti Jul 21, 2021

Choose a reason for hiding this comment

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

By the way, as far as I can tell, passing an empty volumes.DeleteOpts{} is a fancy noop with lots of reflection in the way.

})
volumeID = volume.ID

err = volumes.WaitForStatus(is.volumeClient, volumeID, "available", 300)
Expand Down Expand Up @@ -820,7 +825,12 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust
KeyName: keyName,
}).Extract()
if err != nil {
return nil, fmt.Errorf("Create new server err: %v", err)
for _, cleanup := range cleanupOperationsInCaseOfServerCreationFailure {
if e := cleanup(); e != nil {
err = fmt.Errorf("%w. Additionally: %v", err, e)
}
}
return nil, err
}

is.computeClient.Microversion = ""
Expand Down