Skip to content

volume wait interval through settings #872

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
OmkarDeshpande7 merged 2 commits into from
Sep 8, 2025
Merged

volume wait interval through settings #872

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
698172b
volume wait interval through settings
OmkarDeshpande7 Sep 5, 2025
3aadf30
address bot feedback
OmkarDeshpande7 Sep 8, 2025
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
9 changes: 9 additions & 0 deletions k8s/migration/pkg/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,12 @@ const (
// VMActiveWaitRetryLimit is the number of retries to wait for vm to become active
VMActiveWaitRetryLimit = 15

// VolumeAvailableWaitIntervalSeconds is the interval to wait for volume to become available
VolumeAvailableWaitIntervalSeconds = 5

// VolumeAvailableWaitRetryLimit is the number of retries to wait for volume to become available
VolumeAvailableWaitRetryLimit = 15

// DefaultMigrationMethod is the default migration method
DefaultMigrationMethod = "hot"

Expand All @@ -195,6 +201,9 @@ const (
// CleanupVolumesAfterConvertFailure is the default value for cleanup volumes after convert failure
CleanupVolumesAfterConvertFailure = true

// PopulateVMwareMachineFlavors is the default value for populate vmware machine flavors
PopulateVMwareMachineFlavors = true

// VjailbreakSettingsConfigMapName is the name of the vjailbreak settings configmap
VjailbreakSettingsConfigMapName = "vjailbreak-settings"
)
Expand Down
44 changes: 30 additions & 14 deletions k8s/migration/pkg/utils/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ type VjailbreakSettings struct {
VMActiveWaitIntervalSeconds int
// VMActiveWaitRetryLimit is the number of retries to wait for VM to become active
VMActiveWaitRetryLimit int
// VolumeAvailableWaitIntervalSeconds is the interval to wait for volume to become available
VolumeAvailableWaitIntervalSeconds int
// VolumeAvailableWaitRetryLimit is the number of retries to wait for volume to become available
VolumeAvailableWaitRetryLimit int
// DefaultMigrationMethod is the default migration method (hot/cold)
DefaultMigrationMethod string
// VCenterScanConcurrencyLimit is the max number of vms to scan at the same time
Expand Down Expand Up @@ -63,37 +67,47 @@ func GetVjailbreakSettings(ctx context.Context, k8sClient client.Client) (*Vjail
}

if vjailbreakSettingsCM.Data["CHANGED_BLOCKS_COPY_ITERATION_THRESHOLD"] == "" {
vjailbreakSettingsCM.Data["CHANGED_BLOCKS_COPY_ITERATION_THRESHOLD"] = "20"
vjailbreakSettingsCM.Data["CHANGED_BLOCKS_COPY_ITERATION_THRESHOLD"] = strconv.Itoa(constants.ChangedBlocksCopyIterationThreshold)
}

if vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_INTERVAL_SECONDS"] == "" {
vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_INTERVAL_SECONDS"] = "20"
vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_INTERVAL_SECONDS"] = strconv.Itoa(constants.VMActiveWaitIntervalSeconds)
}

if vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_RETRY_LIMIT"] == "" {
vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_RETRY_LIMIT"] = "15"
vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_RETRY_LIMIT"] = strconv.Itoa(constants.VolumeAvailableWaitRetryLimit)
Comment thread
OmkarDeshpande7 marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review Bot Sep 5, 2025

Choose a reason for hiding this comment

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

Incorrect constant used for VM retry limit

Incorrect constant used for VM_ACTIVE_WAIT_RETRY_LIMIT. The code is using VolumeAvailableWaitRetryLimit instead of VMActiveWaitRetryLimit, which could lead to incorrect retry behavior.

Code suggestion
Check the AI-generated fix before applying
Suggested change
vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_RETRY_LIMIT"] = strconv.Itoa(constants.VolumeAvailableWaitRetryLimit)
vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_RETRY_LIMIT"] = strconv.Itoa(constants.VMActiveWaitRetryLimit)

Code Review Run #937f11

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

Copy link
Copy Markdown
Contributor

@windsurf-bot windsurf-bot Bot Sep 8, 2025

Choose a reason for hiding this comment

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

There's an incorrect constant being used here. This should be using constants.VMActiveWaitRetryLimit instead of constants.VolumeAvailableWaitRetryLimit to maintain consistency with the field name.

Suggested change
vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_RETRY_LIMIT"] = strconv.Itoa(constants.VolumeAvailableWaitRetryLimit)
vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_RETRY_LIMIT"] = strconv.Itoa(constants.VMActiveWaitRetryLimit)

}

if vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_INTERVAL_SECONDS"] == "" {
vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_INTERVAL_SECONDS"] = strconv.Itoa(constants.VolumeAvailableWaitIntervalSeconds)
}

if vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_RETRY_LIMIT"] == "" {
vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_RETRY_LIMIT"] = strconv.Itoa(constants.VolumeAvailableWaitRetryLimit)
}

if vjailbreakSettingsCM.Data["DEFAULT_MIGRATION_METHOD"] == "" {
vjailbreakSettingsCM.Data["DEFAULT_MIGRATION_METHOD"] = "hot"
vjailbreakSettingsCM.Data["DEFAULT_MIGRATION_METHOD"] = constants.DefaultMigrationMethod
}

if vjailbreakSettingsCM.Data["VCENTER_SCAN_CONCURRENCY_LIMIT"] == "" {
vjailbreakSettingsCM.Data["VCENTER_SCAN_CONCURRENCY_LIMIT"] = "10"
vjailbreakSettingsCM.Data["VCENTER_SCAN_CONCURRENCY_LIMIT"] = strconv.Itoa(constants.VCenterScanConcurrencyLimit)
}

if vjailbreakSettingsCM.Data["CLEANUP_VOLUMES_AFTER_CONVERT_FAILURE"] == "" {
vjailbreakSettingsCM.Data["CLEANUP_VOLUMES_AFTER_CONVERT_FAILURE"] = "false"
vjailbreakSettingsCM.Data["CLEANUP_VOLUMES_AFTER_CONVERT_FAILURE"] = strconv.FormatBool(constants.CleanupVolumesAfterConvertFailure)
}

if vjailbreakSettingsCM.Data["POPULATE_VMWARE_MACHINE_FLAVORS"] == "" {
vjailbreakSettingsCM.Data["POPULATE_VMWARE_MACHINE_FLAVORS"] = trueString
vjailbreakSettingsCM.Data["POPULATE_VMWARE_MACHINE_FLAVORS"] = strconv.FormatBool(constants.PopulateVMwareMachineFlavors)
}

return &VjailbreakSettings{
ChangedBlocksCopyIterationThreshold: atoi(vjailbreakSettingsCM.Data["CHANGED_BLOCKS_COPY_ITERATION_THRESHOLD"]),
VMActiveWaitIntervalSeconds: atoi(vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_INTERVAL_SECONDS"]),
VMActiveWaitRetryLimit: atoi(vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_RETRY_LIMIT"]),
VolumeAvailableWaitIntervalSeconds: atoi(vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_INTERVAL_SECONDS"]),
VolumeAvailableWaitRetryLimit: atoi(vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_RETRY_LIMIT"]),
DefaultMigrationMethod: vjailbreakSettingsCM.Data["DEFAULT_MIGRATION_METHOD"],
VCenterScanConcurrencyLimit: atoi(vjailbreakSettingsCM.Data["VCENTER_SCAN_CONCURRENCY_LIMIT"]),
CleanupVolumesAfterConvertFailure: vjailbreakSettingsCM.Data["CLEANUP_VOLUMES_AFTER_CONVERT_FAILURE"] == "true",
Expand All @@ -104,12 +118,14 @@ func GetVjailbreakSettings(ctx context.Context, k8sClient client.Client) (*Vjail
// getDefaultSettings returns default vjailbreak settings
func getDefaultSettings() *VjailbreakSettings {
return &VjailbreakSettings{
ChangedBlocksCopyIterationThreshold: 20,
VMActiveWaitIntervalSeconds: 20,
VMActiveWaitRetryLimit: 15,
DefaultMigrationMethod: "hot",
VCenterScanConcurrencyLimit: 10,
CleanupVolumesAfterConvertFailure: false,
PopulateVMwareMachineFlavors: true,
ChangedBlocksCopyIterationThreshold: constants.ChangedBlocksCopyIterationThreshold,
VMActiveWaitIntervalSeconds: constants.VMActiveWaitIntervalSeconds,
VMActiveWaitRetryLimit: constants.VMActiveWaitRetryLimit,
VolumeAvailableWaitIntervalSeconds: constants.VolumeAvailableWaitIntervalSeconds,
VolumeAvailableWaitRetryLimit: constants.VolumeAvailableWaitRetryLimit,
DefaultMigrationMethod: constants.DefaultMigrationMethod,
VCenterScanConcurrencyLimit: constants.VCenterScanConcurrencyLimit,
CleanupVolumesAfterConvertFailure: constants.CleanupVolumesAfterConvertFailure,
PopulateVMwareMachineFlavors: constants.PopulateVMwareMachineFlavors,
}
}
2 changes: 1 addition & 1 deletion v2v-helper/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ require (
github.com/gophercloud/gophercloud v1.14.1
github.com/hashicorp/go-retryablehttp v0.7.7
github.com/pkg/errors v0.9.1
github.com/platform9/vjailbreak/k8s/migration v0.0.0-20250718102048-de8740c10909
github.com/platform9/vjailbreak/k8s/migration v0.0.0-20250904115639-c2134e9ef3b9
github.com/prometheus-community/pro-bing v0.4.1
github.com/stretchr/testify v1.10.0
github.com/vmware/govmomi v0.51.0
Expand Down
2 changes: 1 addition & 1 deletion v2v-helper/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ func main() {
<-ackChan
}
utils.PrintLog(msg)
return
}
Copy link
Copy Markdown
Contributor

@windsurf-bot windsurf-bot Bot Sep 5, 2025

Choose a reason for hiding this comment

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

Removing the return statement from the handleError function changes its behavior significantly. Without the return, execution will continue after reporting an error, which could lead to unexpected behavior or crashes when trying to use uninitialized resources later in the code. The function appears to be designed to report errors and then terminate execution.

Copy link
Copy Markdown
Contributor

@windsurf-bot windsurf-bot Bot Sep 8, 2025

Choose a reason for hiding this comment

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

The removal of the return statement in the handleError function changes its behavior significantly. Without the return, execution will continue after logging an error, which could lead to unexpected behavior or crashes later in the code when operating with invalid state. Consider keeping the return statement or ensuring proper error handling flow.


client, err := utils.GetInclusterClient()
Expand Down Expand Up @@ -93,6 +92,7 @@ func main() {
if err != nil {
handleError(fmt.Sprintf("Failed to validate OpenStack connection: %v", err))
}
openstackclients.K8sClient = client
utils.PrintLog("Connected to OpenStack")

// Get thumbprint
Expand Down
1 change: 1 addition & 0 deletions v2v-helper/openstack/openstackops.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ func validateOpenStack(insecure bool) (*migrateutils.OpenStackClients, error) {
BlockStorageClient: blockStorageClient,
ComputeClient: computeClient,
NetworkingClient: networkingClient,
K8sClient: nil,
}, nil
}

Expand Down
6 changes: 6 additions & 0 deletions v2v-helper/pkg/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,12 @@ echo "$(date '+%Y-%m-%d %H:%M:%S') - Network fix script completed" >> "$LOG_FILE
// VMActiveWaitRetryLimit is the number of retries to wait for vm to become active
VMActiveWaitRetryLimit = 15

// VolumeAvailableWaitIntervalSeconds is the interval to wait for volume to become available
VolumeAvailableWaitIntervalSeconds = 5

// VolumeAvailableWaitRetryLimit is the number of retries to wait for volume to become available
VolumeAvailableWaitRetryLimit = 15

// DefaultMigrationMethod is the default migration method
DefaultMigrationMethod = "hot"

Expand Down
20 changes: 16 additions & 4 deletions v2v-helper/pkg/utils/migrateutils/openstackopsutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package migrateutils

import (
"context"
"encoding/json"
"fmt"
"io"
Expand All @@ -17,6 +18,7 @@ import (
"github.com/platform9/vjailbreak/v2v-helper/pkg/constants"
"github.com/platform9/vjailbreak/v2v-helper/pkg/utils"
"github.com/platform9/vjailbreak/v2v-helper/vm"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/gophercloud/gophercloud"
"github.com/gophercloud/gophercloud/openstack"
Expand All @@ -36,6 +38,7 @@ type OpenStackClients struct {
BlockStorageClient *gophercloud.ServiceClient
ComputeClient *gophercloud.ServiceClient
NetworkingClient *gophercloud.ServiceClient
K8sClient client.Client
}

type OpenStackMetadata struct {
Expand Down Expand Up @@ -129,7 +132,12 @@ func (osclient *OpenStackClients) DeleteVolume(volumeID string) error {
}

func (osclient *OpenStackClients) WaitForVolume(volumeID string) error {
for i := 0; i < constants.MaxIntervalCount; i++ {
// Get vjailbreak settings
vjailbreakSettings, err := utils.GetVjailbreakSettings(context.Background(), osclient.K8sClient)
if err != nil {
return errors.Wrap(err, "failed to get vjailbreak settings")
}
for i := 0; i < vjailbreakSettings.VolumeAvailableWaitRetryLimit; i++ {
volume, err := volumes.Get(osclient.BlockStorageClient, volumeID).Extract()
if err != nil {
return fmt.Errorf("failed to get volume: %s", err)
Expand Down Expand Up @@ -162,7 +170,7 @@ func (osclient *OpenStackClients) WaitForVolume(volumeID string) error {
return nil
}
fmt.Printf("Volume %s is still attached to server retrying %d times\n", volumeID, i)
time.Sleep(5 * time.Second) // Wait for 5 seconds before checking again
time.Sleep(time.Duration(vjailbreakSettings.VolumeAvailableWaitIntervalSeconds) * time.Second) // Wait for 5 seconds before checking again
}
return fmt.Errorf("volume did not become available within %d seconds", constants.MaxIntervalCount*5)
Comment thread
OmkarDeshpande7 marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Contributor

@windsurf-bot windsurf-bot Bot Sep 8, 2025

Choose a reason for hiding this comment

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

The error message still references constants.MaxIntervalCount*5 seconds, but should be updated to use the configurable settings: vjailbreakSettings.VolumeAvailableWaitRetryLimit*vjailbreakSettings.VolumeAvailableWaitIntervalSeconds.

Copy link
Copy Markdown
Collaborator Author

@OmkarDeshpande7 OmkarDeshpande7 Sep 8, 2025

Choose a reason for hiding this comment

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

ok

}
Expand All @@ -173,7 +181,11 @@ func (osclient *OpenStackClients) AttachVolumeToVM(volumeID string) error {
return fmt.Errorf("failed to get instance ID: %s", err)
}

for i := 0; i < constants.MaxIntervalCount; i++ {
vjailbreakSettings, err := utils.GetVjailbreakSettings(context.Background(), osclient.K8sClient)
if err != nil {
return errors.Wrap(err, "failed to get vjailbreak settings")
}
for i := 0; i < vjailbreakSettings.VolumeAvailableWaitRetryLimit; i++ {
_, err = volumeattach.Create(osclient.ComputeClient, instanceID, volumeattach.CreateOpts{
VolumeID: volumeID,
DeleteOnTermination: false,
Expand All @@ -182,7 +194,7 @@ func (osclient *OpenStackClients) AttachVolumeToVM(volumeID string) error {
err = nil
break
}
time.Sleep(5 * time.Second) // Wait for 5 seconds before checking again
time.Sleep(time.Duration(vjailbreakSettings.VolumeAvailableWaitIntervalSeconds) * time.Second) // Wait for 5 seconds before checking again
}
if err != nil {
return fmt.Errorf("failed to attach volume to VM: %s", err)
Expand Down
2 changes: 2 additions & 0 deletions v2v-helper/pkg/utils/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@ type VjailbreakSettings struct {
VCenterScanConcurrencyLimit int
CleanupVolumesAfterConvertFailure bool
PopulateVMwareMachineFlavors bool
VolumeAvailableWaitIntervalSeconds int
VolumeAvailableWaitRetryLimit int
}
12 changes: 12 additions & 0 deletions v2v-helper/pkg/utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ func GetVjailbreakSettings(ctx context.Context, k8sClient client.Client) (*Vjail
VCenterScanConcurrencyLimit: constants.VCenterScanConcurrencyLimit,
CleanupVolumesAfterConvertFailure: constants.CleanupVolumesAfterConvertFailure,
PopulateVMwareMachineFlavors: constants.PopulateVMwareMachineFlavors,
VolumeAvailableWaitIntervalSeconds: constants.VolumeAvailableWaitIntervalSeconds,
VolumeAvailableWaitRetryLimit: constants.VolumeAvailableWaitRetryLimit,
}, nil
}

Expand All @@ -146,6 +148,14 @@ func GetVjailbreakSettings(ctx context.Context, k8sClient client.Client) (*Vjail
vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_RETRY_LIMIT"] = strconv.Itoa(constants.VMActiveWaitRetryLimit)
}

if vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_INTERVAL_SECONDS"] == "" {
vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_INTERVAL_SECONDS"] = strconv.Itoa(constants.VolumeAvailableWaitIntervalSeconds)
}

if vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_RETRY_LIMIT"] == "" {
vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_RETRY_LIMIT"] = strconv.Itoa(constants.VolumeAvailableWaitRetryLimit)
}

if vjailbreakSettingsCM.Data["DEFAULT_MIGRATION_METHOD"] == "" {
vjailbreakSettingsCM.Data["DEFAULT_MIGRATION_METHOD"] = constants.DefaultMigrationMethod
}
Expand All @@ -166,6 +176,8 @@ func GetVjailbreakSettings(ctx context.Context, k8sClient client.Client) (*Vjail
ChangedBlocksCopyIterationThreshold: atoi(vjailbreakSettingsCM.Data["CHANGED_BLOCKS_COPY_ITERATION_THRESHOLD"]),
VMActiveWaitIntervalSeconds: atoi(vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_INTERVAL_SECONDS"]),
VMActiveWaitRetryLimit: atoi(vjailbreakSettingsCM.Data["VM_ACTIVE_WAIT_RETRY_LIMIT"]),
VolumeAvailableWaitIntervalSeconds: atoi(vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_INTERVAL_SECONDS"]),
VolumeAvailableWaitRetryLimit: atoi(vjailbreakSettingsCM.Data["VOLUME_AVAILABLE_WAIT_RETRY_LIMIT"]),
DefaultMigrationMethod: vjailbreakSettingsCM.Data["DEFAULT_MIGRATION_METHOD"],
VCenterScanConcurrencyLimit: atoi(vjailbreakSettingsCM.Data["VCENTER_SCAN_CONCURRENCY_LIMIT"]),
CleanupVolumesAfterConvertFailure: vjailbreakSettingsCM.Data["CLEANUP_VOLUMES_AFTER_CONVERT_FAILURE"] == "true",
Expand Down
Loading