Make hardcoded timeout and configuration values configurable#180
Make hardcoded timeout and configuration values configurable#180davidjpeacock wants to merge 2 commits intoos-migrate:mainfrom
Conversation
Replace hardcoded values with configurable parameters to improve flexibility across different deployment environments. All changes preserve original default values for backward compatibility. Changes: - Log file path now respects osmdatadir instead of hardcoded /tmp/ - NBDKit port configurable via 'port' parameter (default: "10809") - OpenStack operation timeout configurable via 'timeout_seconds' (default: 15000) - NBDKit startup timeout configurable via 'nbdkit_timeout' (default: 30) - NBDKit retry delay configurable via 'nbdkit_retry_delay' (default: 2) Users can now customize timeouts and paths without modifying source code, enabling better adaptation to slower/faster infrastructure and custom deployment requirements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
matbu
left a comment
matbu
left a comment
There was a problem hiding this comment.
This early feedbacks because I guess it's still WIP.
Overall I think exposing timeout parameters to the user for all the OpenStack operations is very nice but going deeper at the nbdkit level, I dont see the value and maybe it's more risky.
For nbdkit I think only moving the hardcoded timeout at the package level is safer for now. We will see in the futur if the users really need to tweak them, but we did a lot of tests (performance in particular) and the reaction of the nbdkit server was correct and the expected timing was sane.
| Password string | ||
| Server string | ||
| Libdir string | ||
| Port string |
There was a problem hiding this comment.
I dont think all these values should be configurable via the modules.
The port can be useful in some specific cases, but we specified in the network configuration (doc) which ports needs to be open & so on and since we have the total control of the conversion host because it's a host created by us and dedicated to the migration, the nbdkit port can remain what we decided. But if we want to make it as a variable then it should be renamed as NbdkitPort (because here Port can be confused with the Server port)
| func WaitForVolumeStatus(client *gophercloud.ServiceClient, volumeID, status string, timeout int) error { | ||
| for i := 0; i < timeout; i++ { | ||
| func WaitForVolumeStatus(client *gophercloud.ServiceClient, volumeID, status string, timeoutSeconds int) error { | ||
| timeoutIterations := timeoutSeconds / 5 |
There was a problem hiding this comment.
Why just not use the timeout as it was ?
It seems to me confusing, if user set a timeout (600 for example) and then this value is divided by 5, so the timeout won't be what they set.
|
|
||
| func WaitForServerStatus(client *gophercloud.ServiceClient, serverID, status string, timeout int) error { | ||
| for i := 0; i < timeout; i++ { | ||
| func WaitForServerStatus(client *gophercloud.ServiceClient, serverID, status string, timeoutSeconds int) error { |
| } | ||
|
|
||
| func AttachVolume(client *gophercloud.ProviderClient, volumeID string, instanceName string, instanceUUID string) error { | ||
| func AttachVolume(client *gophercloud.ProviderClient, volumeID string, instanceName string, instanceUUID string, timeoutSeconds int) error { |
There was a problem hiding this comment.
I think just naming "timeout" is fine.
| } | ||
|
|
||
| func CreateServer(provider *gophercloud.ProviderClient, args ServerArgs) (string, error) { | ||
| func CreateServer(provider *gophercloud.ProviderClient, args ServerArgs, timeoutSeconds int) (string, error) { |
There was a problem hiding this comment.
here too: s/timeoutSeconds/timeout/
|
|
||
| // DeleteVolume deletes a volume by ID | ||
| func DeleteVolume(provider *gophercloud.ProviderClient, volumeID string) error { | ||
| func DeleteVolume(provider *gophercloud.ProviderClient, volumeID string, timeoutSeconds int) error { |
There was a problem hiding this comment.
here too: s/timeoutSeconds/timeout/
| Compression string | ||
| UUID string | ||
| Port string | ||
| Timeout int |
There was a problem hiding this comment.
Here, As I said in the module migrate.go, I dont think we should expose to the user the Timeout for nbdkit and the retry as well. It could be configurable at the package level, but I think it can be risky to expose those settings for several reasons (we know those retry limits are sane even in a stressed environment).
|
|
||
| time.Sleep(100 * time.Millisecond) | ||
| err = WaitForNbdkitURI("localhost", "10809", 30*time.Second) | ||
| err = WaitForNbdkitURI("localhost", c.Port, c.Timeout, c.RetryDelay) |
There was a problem hiding this comment.
If you make the port as a variable, you have to set for nbdkit as well otherwise it's just waiting for the wrong port.
|
Thank you Mathieu. I'm consider all this feedback, rework, and resubmit. |
2 participants
Replace hardcoded values with configurable parameters to improve flexibility across different deployment environments. All changes preserve original default values for backward compatibility.
Changes:
Users can now customize timeouts and paths without modifying source code, enabling better adaptation to slower/faster infrastructure and custom deployment requirements.
🤖 Generated with Claude Code