Skip to content

Commit 55827a6

Browse files
sarika-pf9claude
sarika-pf9
and
claude
authored
[1803] fix: Added retry for transient API errors (#1958)
Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 parent 85c6b0f commit 55827a6

6 files changed

Lines changed: 241 additions & 66 deletions

File tree

‎.githooks/pre-commit‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ cd "${REPO_ROOT}"
88
if [ -f graphify-out/graph.json ]; then
99
echo "[pre-commit] updating knowledge graph (graphify)"
1010
GRAPHIFY_PYTHON=""
11-
GRAPHIFY_BIN=$(command -v graphify 2>/dev/null)
11+
GRAPHIFY_BIN=$(command -v graphify 2>/dev/null) || true
1212
if [ -n "$GRAPHIFY_BIN" ]; then
1313
_SHEBANG=$(head -1 "$GRAPHIFY_BIN" | sed 's/^#![[:space:]]*//')
1414
case "$_SHEBANG" in

‎pkg/common/constants/constants.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,11 @@ const (
353353
// Number of intervals to wait for the volume to become available
354354
MaxIntervalCount = 60
355355

356+
// Retry attempts for delete operations (port, volume) during cleanup.
357+
// Kept small — cleanup should not block indefinitely, but must survive transient API errors.
358+
DeleteOperationRetryCount = 5
359+
DeleteOperationRetryIntervalSeconds = 5
360+
356361
InspectOSCommand = "inspect-os"
357362
LSBootCommand = "ls /boot"
358363
XMLFileName = "libxml.xml"

‎v2v-helper/migrate/migrate.go‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,10 @@ func (migobj *Migrate) DetachVolume(ctx context.Context, disk vm.VMDisk) error {
269269
func (migobj *Migrate) DetachAllVolumes(ctx context.Context, vminfo vm.VMInfo) error {
270270
openstackops := migobj.Openstackclients
271271
for _, vmdisk := range vminfo.VMDisks {
272+
if vmdisk.OpenstackVol == nil {
273+
migobj.logMessage(fmt.Sprintf("Skipping detach for disk %s: no OpenStack volume was created", vmdisk.Name))
274+
continue
275+
}
272276
migobj.logMessage(fmt.Sprintf("Detaching volume %s from VM", vmdisk.Name))
273277
if err := openstackops.DetachVolumeFromVM(ctx, vmdisk.OpenstackVol.ID); err != nil && !strings.Contains(err.Error(), "is not attached to volume") {
274278
return errors.Wrap(err, "failed to detach volume from VM")
@@ -287,6 +291,10 @@ func (migobj *Migrate) DetachAllVolumes(ctx context.Context, vminfo vm.VMInfo) e
287291
func (migobj *Migrate) DeleteAllVolumes(ctx context.Context, vminfo vm.VMInfo) error {
288292
openstackops := migobj.Openstackclients
289293
for _, vmdisk := range vminfo.VMDisks {
294+
if vmdisk.OpenstackVol == nil {
295+
migobj.logMessage(fmt.Sprintf("Skipping delete for disk %s: no OpenStack volume was created", vmdisk.Name))
296+
continue
297+
}
290298
err := openstackops.DeleteVolume(ctx, vmdisk.OpenstackVol.ID)
291299
if err != nil {
292300
return errors.Wrap(err, "failed to delete volume")
@@ -1881,6 +1889,9 @@ func (migobj *Migrate) MigrateVM(ctx context.Context) error {
18811889
if migobj.StorageCopyMethod == constants.StorageCopyMethod {
18821890
// Initialize storage provider if using StorageAcceleratedCopy migration
18831891
if err := migobj.InitializeStorageProvider(ctx); err != nil {
1892+
if cleanuperror := migobj.cleanup(ctx, vminfo, fmt.Sprintf("failed to initialize storage provider: %s", err), portids, vcenterSettings); cleanuperror != nil {
1893+
return errors.Wrapf(err, "failed to cleanup after storage provider init failure: %s", cleanuperror)
1894+
}
18841895
return errors.Wrap(err, "failed to initialize storage provider")
18851896
}
18861897
defer func() {
@@ -1889,15 +1900,24 @@ func (migobj *Migrate) MigrateVM(ctx context.Context) error {
18891900
}
18901901
}()
18911902
if err := migobj.ValidateStorageAcceleratedCopyPrerequisites(ctx); err != nil {
1903+
if cleanuperror := migobj.cleanup(ctx, vminfo, fmt.Sprintf("StorageAcceleratedCopy prerequisites validation failed: %s", err), portids, vcenterSettings); cleanuperror != nil {
1904+
return errors.Wrapf(err, "failed to cleanup after prerequisites validation failure: %s", cleanuperror)
1905+
}
18921906
return errors.Wrap(err, "StorageAcceleratedCopy prerequisites validation failed")
18931907
}
18941908

18951909
// Perform the copy here.
18961910
if _, err := migobj.StorageAcceleratedCopyCopyDisks(ctx, vminfo); err != nil {
1911+
if cleanuperror := migobj.cleanup(ctx, vminfo, fmt.Sprintf("failed to perform StorageAcceleratedCopy copy: %s", err), portids, vcenterSettings); cleanuperror != nil {
1912+
return errors.Wrapf(err, "failed to cleanup after StorageAcceleratedCopy failure: %s", cleanuperror)
1913+
}
18971914
return errors.Wrap(err, "failed to perform StorageAcceleratedCopy copy")
18981915
}
18991916
// Apply image tags to the volumes we cinder managed.
19001917
if err := migobj.applyImageMetadataForXCOPYVolumes(ctx, vminfo); err != nil {
1918+
if cleanuperror := migobj.cleanup(ctx, vminfo, fmt.Sprintf("failed to apply image metadata to XCOPY volumes: %s", err), portids, vcenterSettings); cleanuperror != nil {
1919+
return errors.Wrapf(err, "failed to cleanup after image metadata failure: %s", cleanuperror)
1920+
}
19011921
return errors.Wrap(err, "failed to apply image metadata to XCOPY volumes")
19021922
}
19031923

@@ -1906,12 +1926,15 @@ func (migobj *Migrate) MigrateVM(ctx context.Context) error {
19061926
// Create and Add Volumes to Host
19071927
vminfo, err = migobj.CreateVolumes(ctx, vminfo)
19081928
if err != nil {
1929+
if cleanuperror := migobj.cleanup(ctx, vminfo, fmt.Sprintf("failed to create volumes: %s", err), portids, vcenterSettings); cleanuperror != nil {
1930+
return errors.Wrapf(err, "failed to cleanup after volume creation failure: %s", cleanuperror)
1931+
}
19091932
return errors.Wrap(err, "failed to add volumes to host")
19101933
}
19111934
// Enable CBT
19121935
err = migobj.EnableCBTWrapper()
19131936
if err != nil {
1914-
migobj.cleanup(ctx, vminfo, fmt.Sprintf("CBT Failure: %s", err), portids, nil)
1937+
migobj.cleanup(ctx, vminfo, fmt.Sprintf("CBT Failure: %s", err), portids, vcenterSettings)
19151938
return errors.Wrap(err, "CBT Failure")
19161939
}
19171940

@@ -1923,7 +1946,7 @@ func (migobj *Migrate) MigrateVM(ctx context.Context) error {
19231946
// Live Replicate Disks
19241947
vminfo, err = migobj.LiveReplicateDisks(ctx, vminfo)
19251948
if err != nil {
1926-
if cleanuperror := migobj.cleanup(ctx, vminfo, fmt.Sprintf("failed to live replicate disks: %s", err), portids, nil); cleanuperror != nil {
1949+
if cleanuperror := migobj.cleanup(ctx, vminfo, fmt.Sprintf("failed to live replicate disks: %s", err), portids, vcenterSettings); cleanuperror != nil {
19271950
// combine both errors
19281951
return errors.Wrapf(err, "failed to cleanup disks: %s", cleanuperror)
19291952
}

‎v2v-helper/migrate/migrate_test.go‎

Lines changed: 81 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/platform9/vjailbreak/pkg/common/constants"
1111
"github.com/platform9/vjailbreak/v2v-helper/nbd"
1212
"github.com/platform9/vjailbreak/v2v-helper/openstack"
13+
"github.com/platform9/vjailbreak/v2v-helper/pkg/k8sutils"
1314
"github.com/platform9/vjailbreak/v2v-helper/vm"
1415

1516
"github.com/golang/mock/gomock"
@@ -517,7 +518,7 @@ func TestCreateTargetInstance(t *testing.T) {
517518
mockOpenStackOps.EXPECT().CreatePort(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&ports.Port{
518519
MACAddress: "mac-address",
519520
}, nil).AnyTimes()
520-
mockOpenStackOps.EXPECT().CreateVM(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&servers.Server{}, nil).AnyTimes()
521+
mockOpenStackOps.EXPECT().CreateVM(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&servers.Server{}, nil).AnyTimes()
521522
mockOpenStackOps.EXPECT().WaitUntilVMActive(gomock.Any(), gomock.Any()).Return(true, nil).AnyTimes()
522523
mockOpenStackOps.EXPECT().GetFlavor(gomock.Any(), "flavor-id").Return(&flavors.Flavor{
523524
VCPUs: 2,
@@ -571,7 +572,7 @@ func TestCreateTargetInstance_AdvancedMapping_Ports(t *testing.T) {
571572
ID: "port-2-id",
572573
NetworkID: "network-2",
573574
}, nil).AnyTimes()
574-
mockOpenStackOps.EXPECT().CreateVM(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&servers.Server{}, nil).AnyTimes()
575+
mockOpenStackOps.EXPECT().CreateVM(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&servers.Server{}, nil).AnyTimes()
575576
mockOpenStackOps.EXPECT().WaitUntilVMActive(gomock.Any(), gomock.Any()).Return(true, nil).AnyTimes()
576577
mockOpenStackOps.EXPECT().GetFlavor(gomock.Any(), "flavor-id").Return(&flavors.Flavor{
577578
VCPUs: 2,
@@ -619,7 +620,7 @@ func TestCreateTargetInstance_AdvancedMapping_InsufficientPorts(t *testing.T) {
619620
RAM: 2048,
620621
}, nil).AnyTimes()
621622
mockOpenStackOps.EXPECT().GetSecurityGroupIDs(gomock.Any(), gomock.Any(), gomock.Any()).Return([]string{}, nil).AnyTimes()
622-
mockOpenStackOps.EXPECT().CreateVM(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&servers.Server{}, nil).AnyTimes()
623+
mockOpenStackOps.EXPECT().CreateVM(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&servers.Server{}, nil).AnyTimes()
623624
mockOpenStackOps.EXPECT().WaitUntilVMActive(gomock.Any(), gomock.Any()).Return(true, nil).AnyTimes()
624625
inputvminfo := vm.VMInfo{
625626
Name: "test-vm",
@@ -652,3 +653,80 @@ func TestCreateTargetInstance_AdvancedMapping_InsufficientPorts(t *testing.T) {
652653
// This test now just verifies that CreateTargetInstance can handle mismatched Networkports config
653654
assert.NoError(t, err)
654655
}
656+
657+
func TestDetachAllVolumes_SkipsNilOpenstackVol(t *testing.T) {
658+
ctrl := gomock.NewController(t)
659+
defer ctrl.Finish()
660+
ctx := context.Background()
661+
662+
mockOpenStackOps := openstack.NewMockOpenstackOperations(ctrl)
663+
// Only disk1 has a volume — expect exactly one detach+wait, not two.
664+
mockOpenStackOps.EXPECT().DetachVolumeFromVM(gomock.Any(), "id1").Return(nil).Times(1)
665+
mockOpenStackOps.EXPECT().WaitForVolume(gomock.Any(), "id1").Return(nil).Times(1)
666+
667+
vminfo := vm.VMInfo{
668+
VMDisks: []vm.VMDisk{
669+
{Name: "disk1", OpenstackVol: &volumes.Volume{ID: "id1"}},
670+
{Name: "disk2", OpenstackVol: nil},
671+
},
672+
}
673+
migobj := Migrate{Openstackclients: mockOpenStackOps, InPod: false}
674+
err := migobj.DetachAllVolumes(ctx, vminfo)
675+
assert.NoError(t, err)
676+
}
677+
678+
func TestDeleteAllVolumes_SkipsNilOpenstackVol(t *testing.T) {
679+
ctrl := gomock.NewController(t)
680+
defer ctrl.Finish()
681+
ctx := context.Background()
682+
683+
mockOpenStackOps := openstack.NewMockOpenstackOperations(ctrl)
684+
// Only disk1 has a volume — expect exactly one delete call.
685+
mockOpenStackOps.EXPECT().DeleteVolume(gomock.Any(), "id1").Return(nil).Times(1)
686+
687+
vminfo := vm.VMInfo{
688+
VMDisks: []vm.VMDisk{
689+
{Name: "disk1", OpenstackVol: &volumes.Volume{ID: "id1"}},
690+
{Name: "disk2", OpenstackVol: nil},
691+
},
692+
}
693+
migobj := Migrate{Openstackclients: mockOpenStackOps, InPod: false}
694+
err := migobj.DeleteAllVolumes(ctx, vminfo)
695+
assert.NoError(t, err)
696+
}
697+
698+
// TestCleanup_PartialVolumes verifies that when CreateVolumes fails partway through,
699+
// cleanup correctly handles the mix of created (non-nil) and uncreated (nil) volumes,
700+
// deletes only the created volume, and deletes ports when the setting is enabled.
701+
func TestCleanup_PartialVolumes_DeletesCreatedVolumeAndPorts(t *testing.T) {
702+
ctrl := gomock.NewController(t)
703+
defer ctrl.Finish()
704+
ctx := context.Background()
705+
706+
mockOpenStackOps := openstack.NewMockOpenstackOperations(ctrl)
707+
mockVMOps := vm.NewMockVMOperations(ctrl)
708+
709+
// disk1 was created; disk2 was never created (nil OpenstackVol).
710+
mockOpenStackOps.EXPECT().DetachVolumeFromVM(gomock.Any(), "id1").Return(nil).Times(1)
711+
mockOpenStackOps.EXPECT().WaitForVolume(gomock.Any(), "id1").Return(nil).Times(1)
712+
mockOpenStackOps.EXPECT().DeleteVolume(gomock.Any(), "id1").Return(nil).Times(1)
713+
mockOpenStackOps.EXPECT().DeletePort(gomock.Any(), "port-1").Return(nil).Times(1)
714+
mockVMOps.EXPECT().CleanUpSnapshots(true).Return(nil).Times(1)
715+
716+
vminfo := vm.VMInfo{
717+
VMDisks: []vm.VMDisk{
718+
{Name: "disk1", OpenstackVol: &volumes.Volume{ID: "id1"}},
719+
{Name: "disk2", OpenstackVol: nil},
720+
},
721+
}
722+
settings := &k8sutils.VjailbreakSettings{
723+
CleanupPortsAfterMigrationFailure: true,
724+
}
725+
migobj := Migrate{
726+
Openstackclients: mockOpenStackOps,
727+
VMops: mockVMOps,
728+
InPod: false,
729+
}
730+
err := migobj.cleanup(ctx, vminfo, "test partial volume failure", []string{"port-1"}, settings)
731+
assert.NoError(t, err)
732+
}

‎v2v-helper/openstack/openstackops_mock.go‎

Lines changed: 18 additions & 18 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)