From da0208048422158d2a35e7addbf1ac36fecad384 Mon Sep 17 00:00:00 2001 From: Joseph Callen Date: Thu, 25 Jun 2026 17:13:15 -0400 Subject: [PATCH] OCPBUGS-92792: vsphere: fix CNS volume destroy to aggregate errors deleteCnsVolumes returned on the first error, which silently skipped remaining volumes on that vCenter and all volumes on subsequent vCenters. GetCnsVolumes had the same early-return on per-volume query failures, discarding already-queried results. Align both functions with the error aggregation pattern used by deleteVirtualMachines: collect errors, attempt all items, and return utilerrors.NewAggregate. Also log deleted/found counts at Info level so partial cleanup is visible without --log-level=debug. Co-Authored-By: Claude Opus 4.6 --- pkg/destroy/vsphere/client.go | 7 ++- pkg/destroy/vsphere/vsphere.go | 23 +++++--- pkg/destroy/vsphere/vsphere_test.go | 87 +++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 9 deletions(-) diff --git a/pkg/destroy/vsphere/client.go b/pkg/destroy/vsphere/client.go index f7fc5f1652c..b60d5e2b328 100644 --- a/pkg/destroy/vsphere/client.go +++ b/pkg/destroy/vsphere/client.go @@ -404,12 +404,15 @@ func (c *Client) GetCnsVolumes(ctx context.Context, infraID string) ([]cnstypes. } cnsVolumes := make([]cnstypes.CnsVolume, 0, len(volumes.Volumes)) + var errs []error for _, v := range volumes.Volumes { // This must be called to retrieve the ClusterId result, err := c.cnsClient.QueryVolume(ctx, &cnstypes.CnsQueryFilter{VolumeIds: []cnstypes.CnsVolumeId{v.VolumeId}}) if err != nil { - return nil, err + c.logger.WithField("VolumeId", v.VolumeId.Id).Debug(err) + errs = append(errs, err) + continue } // Confirm that the cluster id matches the infraid @@ -419,7 +422,7 @@ func (c *Client) GetCnsVolumes(ctx context.Context, infraID string) ([]cnstypes. } } } - return cnsVolumes, nil + return cnsVolumes, utilerrors.NewAggregate(errs) } // DeleteCnsVolumes deletes the CNS volume. diff --git a/pkg/destroy/vsphere/vsphere.go b/pkg/destroy/vsphere/vsphere.go index f1fc7de914d..b9e7b5aa25f 100644 --- a/pkg/destroy/vsphere/vsphere.go +++ b/pkg/destroy/vsphere/vsphere.go @@ -265,25 +265,34 @@ func (o *ClusterUninstaller) deleteCnsVolumes(ctx context.Context) error { ctx, cancel := context.WithTimeout(ctx, time.Minute*30) defer cancel() + var errs []error for _, client := range o.clients { - o.Logger.WithField("vCenter", client.GetVCenterName()).Debug("Delete CNS Volumes") + clientLogger := o.Logger.WithField("vCenter", client.GetVCenterName()) + clientLogger.Debug("Delete CNS Volumes") cnsVolumes, err := client.GetCnsVolumes(ctx, o.InfraID) if err != nil { - return err + errs = append(errs, err) } + found := len(cnsVolumes) + deleted := 0 for _, cv := range cnsVolumes { cnsVolumeLogger := o.Logger.WithField("CNSVolume", cv.VolumeId.Id).WithField("vCenter", client.GetVCenterName()) cnsVolumeLogger.Debug("Destroying") - err := client.DeleteCnsVolumes(ctx, cv) - if err != nil { - return err + if err := client.DeleteCnsVolumes(ctx, cv); err != nil { + cnsVolumeLogger.Debug(err) + errs = append(errs, err) + } else { + deleted++ + cnsVolumeLogger.Info("Destroyed") } - cnsVolumeLogger.Info("Destroyed") + } + if found > 0 { + clientLogger.Infof("Deleted %d of %d CNS volumes", deleted, found) } } - return nil + return utilerrors.NewAggregate(errs) } func (o *ClusterUninstaller) destroyCluster(ctx context.Context) (bool, error) { diff --git a/pkg/destroy/vsphere/vsphere_test.go b/pkg/destroy/vsphere/vsphere_test.go index c3efc5c0125..cf84a524c36 100644 --- a/pkg/destroy/vsphere/vsphere_test.go +++ b/pkg/destroy/vsphere/vsphere_test.go @@ -723,4 +723,91 @@ func TestDeleteCnsVolumes(t *testing.T) { err := uninstaller.deleteCnsVolumes(context.TODO()) assert.Regexp(t, "some vsphere error", err) }) + + t.Run("Delete CNS volumes continues after failure", func(t *testing.T) { + mockCtrl := gomock.NewController(t) + vsphereClient := mock.NewMockAPI(mockCtrl) + + vsphereClient.EXPECT(). + GetCnsVolumes(gomock.Any(), gomock.Eq(deleteFailsID)). + Return([]cnstypes.CnsVolume{failVolume, validVolume, failVolume}, nil). + Times(1) + vsphereClient.EXPECT(). + DeleteCnsVolumes(gomock.Any(), gomock.Eq(failVolume)). + Return(errors.New("some vsphere error deleting CNS volume")). + Times(2) + vsphereClient.EXPECT(). + DeleteCnsVolumes(gomock.Any(), gomock.Eq(validVolume)). + Return(nil). + Times(1) + vsphereClient.EXPECT(). + GetVCenterName(). + Return(""). + AnyTimes() + + metadata := newDefaultMetadata() + metadata.InfraID = deleteFailsID + uninstaller := newWithClient(nullLogger, &metadata, []API{vsphereClient}) + assert.NotNil(t, uninstaller) + err := uninstaller.deleteCnsVolumes(context.TODO()) + assert.Regexp(t, "some vsphere error deleting CNS volume", err) + }) + + t.Run("Delete CNS volumes processes all clients when first client fails", func(t *testing.T) { + mockCtrl := gomock.NewController(t) + failClient := mock.NewMockAPI(mockCtrl) + successClient := mock.NewMockAPI(mockCtrl) + + failClient.EXPECT(). + GetCnsVolumes(gomock.Any(), gomock.Eq(infraID)). + Return(nil, errors.New("vcenter connection error")). + Times(1) + failClient.EXPECT(). + GetVCenterName(). + Return("fail-vcenter"). + AnyTimes() + + successClient.EXPECT(). + GetCnsVolumes(gomock.Any(), gomock.Eq(infraID)). + Return([]cnstypes.CnsVolume{validVolume}, nil). + Times(1) + successClient.EXPECT(). + DeleteCnsVolumes(gomock.Any(), gomock.Eq(validVolume)). + Return(nil). + Times(1) + successClient.EXPECT(). + GetVCenterName(). + Return("success-vcenter"). + AnyTimes() + + metadata := newDefaultMetadata() + uninstaller := newWithClient(nullLogger, &metadata, []API{failClient, successClient}) + assert.NotNil(t, uninstaller) + err := uninstaller.deleteCnsVolumes(context.TODO()) + assert.Regexp(t, "vcenter connection error", err) + }) + + t.Run("Delete CNS volumes processes partial results from GetCnsVolumes", func(t *testing.T) { + mockCtrl := gomock.NewController(t) + vsphereClient := mock.NewMockAPI(mockCtrl) + + vsphereClient.EXPECT(). + GetCnsVolumes(gomock.Any(), gomock.Eq(infraID)). + Return([]cnstypes.CnsVolume{validVolume}, errors.New("partial query failure")). + Times(1) + vsphereClient.EXPECT(). + DeleteCnsVolumes(gomock.Any(), gomock.Eq(validVolume)). + Return(nil). + Times(1) + vsphereClient.EXPECT(). + GetVCenterName(). + Return(""). + AnyTimes() + + metadata := newDefaultMetadata() + uninstaller := newWithClient(nullLogger, &metadata, []API{vsphereClient}) + assert.NotNil(t, uninstaller) + err := uninstaller.deleteCnsVolumes(context.TODO()) + assert.Regexp(t, "partial query failure", err) + }) }