CORS-4278: Hardening the GCP destroy code to attempt to delete dependent resources #10035
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -45,6 +45,8 @@ const ( | |||||||
|
|
||||||||
| // DONE represents the done status for a compute service operation. | ||||||||
| DONE = "DONE" | ||||||||
|
|
||||||||
| alreadyInUseStr = "is already being used by " | ||||||||
| ) | ||||||||
|
|
||||||||
| // ClusterUninstaller holds the various options for the cluster we want to delete | ||||||||
|
|
@@ -441,6 +443,72 @@ func operationErrorMessage(op *compute.Operation) string { | |||||||
| return strings.Join(errs, ", ") | ||||||||
| } | ||||||||
|
|
||||||||
| func (o *ClusterUninstaller) handleDependentResourceError(ctx context.Context, op *compute.Operation, err error) error { | ||||||||
| errorCode := 0 | ||||||||
| errMsg := "" | ||||||||
| switch { | ||||||||
| case op != nil: | ||||||||
| errorCode = int(op.HttpErrorStatusCode) | ||||||||
| errMsg = operationErrorMessage(op) | ||||||||
| case err != nil: | ||||||||
| var apiErr *googleapi.Error | ||||||||
| if errors.As(err, &apiErr) { | ||||||||
| errorCode = apiErr.Code | ||||||||
| errMsg = apiErr.Message | ||||||||
| } else { | ||||||||
| errMsg = err.Error() | ||||||||
| } | ||||||||
| default: | ||||||||
| return fmt.Errorf("failed to extract information from operation or error") | ||||||||
| } | ||||||||
|
|
||||||||
| if errorCode == 400 && strings.Contains(errMsg, alreadyInUseStr) { | ||||||||
|
Member
There was a problem hiding this comment. strings.Contains(errMsg, alreadyInUseStr)❓ How future-proof is this condition? Will it ever change? For example, the wording might change. I have always wondered if it's OK to parse an error message like this. Any thoughts?
Member
There was a problem hiding this comment. Is there any detail in the returned error that is more deterministic? Maybe an error code like
Also, is there anywhere in the error that can indicate the dependent resource id, rather than the error message? |
||||||||
| splitDetails := strings.Split(errMsg, alreadyInUseStr) | ||||||||
| resource := strings.ReplaceAll( | ||||||||
| strings.ReplaceAll( | ||||||||
| strings.ReplaceAll(splitDetails[len(splitDetails)-1], "\"", ""), | ||||||||
| "'", "", | ||||||||
| ), ", resourceInUseByAnotherResource", "", | ||||||||
| ) | ||||||||
| splitResource := strings.Split(resource, "/") | ||||||||
|
|
||||||||
| if len(splitResource) > 6 || len(splitResource) < 5 { | ||||||||
| return fmt.Errorf("dependent resource information unable to be parsed: %s", resource) | ||||||||
| } | ||||||||
| // global -> ex: projects/xxxxxxxxxxxxxxxx/global/resource-type/resource-name | ||||||||
| // regional -> ex: projects/xxxxxxxxxxxxxx/region/region-name/resource-type/resource-name | ||||||||
| location, resourceType, resourceName := splitResource[len(splitResource)-3], splitResource[len(splitResource)-2], splitResource[len(splitResource)-1] | ||||||||
| o.Logger.Debugf("found dependent resource information: %s, %s, %s", location, resourceType, resourceName) | ||||||||
|
Comment on lines
+466
to
+481
Member
There was a problem hiding this comment. This part can probably be refactored into a separate func, for example: func (o *ClusterUninstaller) parseResourceFromError(errMsg string) (location, resourceType, resourceName string, err error) {}But then I have the same question whether it is future-proof to parse the error message? |
||||||||
|
|
||||||||
| var deleteErr error | ||||||||
| switch resourceType { | ||||||||
| case "backendServices": | ||||||||
| deleteErr = o.deleteBackendServiceByName(ctx, resourceName, location) | ||||||||
| case "firewalls": | ||||||||
| deleteErr = o.deleteFirewallByName(ctx, resourceName) | ||||||||
| case "forwardingRules": | ||||||||
| deleteErr = o.deleteForwardingRuleByName(ctx, resourceName, location) | ||||||||
| case "subnetworks": | ||||||||
| deleteErr = o.deleteSubnetworkByName(ctx, resourceName) | ||||||||
| case "routers": | ||||||||
| deleteErr = o.deleteRouterByName(ctx, resourceName) | ||||||||
| case "addresses": | ||||||||
| deleteErr = o.deleteAddressByName(ctx, resourceName, location) | ||||||||
| case "targetPools": | ||||||||
| deleteErr = o.deleteTargetPoolByName(ctx, resourceName) | ||||||||
| case "targetTcpProxies": | ||||||||
| deleteErr = o.deleteTargetTCPProxyByName(ctx, resourceName) | ||||||||
| default: | ||||||||
| deleteErr = fmt.Errorf("failed to find resource type: %s for %s", resourceType, resourceName) | ||||||||
|
Member
There was a problem hiding this comment.
Suggested change
|
||||||||
| } | ||||||||
|
|
||||||||
| if deleteErr != nil { | ||||||||
| return fmt.Errorf("failed to delete dependent resource: %w", deleteErr) | ||||||||
| } | ||||||||
| } | ||||||||
|
Member
There was a problem hiding this comment. We should have log when the deletion works: o.Logger.Infof("Successfully deleted dependent resource %s/%s", resourceType, resourceName) |
||||||||
| return nil | ||||||||
| } | ||||||||
|
|
||||||||
| func (o *ClusterUninstaller) handleOperation(ctx context.Context, op *compute.Operation, err error, item cloudResource, resourceType string) error { | ||||||||
| identifier := []string{item.typeName, item.name} | ||||||||
| if item.zone != "" { | ||||||||
|
|
@@ -454,13 +522,20 @@ func (o *ClusterUninstaller) handleOperation(ctx context.Context, op *compute.Op | |||||||
| o.deletePendingItems(item.typeName, []cloudResource{item}) | ||||||||
| return nil | ||||||||
| } | ||||||||
|
|
||||||||
| err = o.handleDependentResourceError(ctx, op, err) | ||||||||
| if err != nil { | ||||||||
| o.Logger.Debugf("failed to handle dependent resource error: %v", err) | ||||||||
| } | ||||||||
|
Comment on lines
+526
to
+529
Member
There was a problem hiding this comment. We should still return the error here so that the destroy loop will try again to delete the resource next time, right? depErr := o.handleDependentResourceError(ctx, op, err)
if depErr != nil {
// If we successfully identified and attempted to delete a dependent resource,
// return a more informative error
return fmt.Errorf("failed to delete %s %s: %w (dependent resource handling: %w)", resourceType, item.name, err, depErr)
}
// If dependent resource was handled successfully, return original error
// The retry logic will attempt deletion again
return fmt.Errorf("failed to delete %s %s: %w", resourceType, item.name, err)
Member
There was a problem hiding this comment. I think the gcp jobs are failing due to nil pointer panics (for example, ci/prow/e2e-gcp-ovn, ci/prow/e2e-gcp-default-config) are happening because of this 😓 |
||||||||
| } | ||||||||
|
|
||||||||
| // wait for operation to complete before checking any further | ||||||||
| op, err = o.waitFor(ctx, op, item) | ||||||||
| if op != nil && op.Status == DONE && isErrorStatus(op.HttpErrorStatusCode) { | ||||||||
| err = o.handleDependentResourceError(ctx, op, err) | ||||||||
| if err != nil { | ||||||||
| o.Logger.Debugf("failed to handle dependent resource error: %v", err) | ||||||||
| } | ||||||||
|
Comment on lines
+535
to
+538
Member
There was a problem hiding this comment. We should probably use a warning here: depErr := o.handleDependentResourceError(ctx, op, err)
if depErr != nil {
o.Logger.Warnf("Dependent resource cleanup failed for %s %s: %v", resourceType, item.name, depErr)
} |
||||||||
| o.resetRequestID(identifier...) | ||||||||
| return fmt.Errorf("failed to delete %s %s with error: %s", resourceType, item.name, operationErrorMessage(op)) | ||||||||
| } | ||||||||
|
|
||||||||
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably still want to keep this log?