Fix leak in parallel resource creation workflows#2109
Fix leak in parallel resource creation workflows#2109cb-github-robot merged 1 commit intocloud-barista:mainfrom
Conversation
Signed-off-by: Seokho Son <shsongist@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a resource leak issue in parallel resource creation workflows by implementing VNet readiness checks and reducing unnecessary logging overhead.
- Added VNet readiness validation with retry mechanisms
- Reduced random sleep duration in resource deletion from 100ms to 1ms
- Commented out debug logging for empty label data
- Updated Docker image version from 0.11.7 to 0.11.8
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/core/infra/provisioning.go | Adds waitForVNetReady function and VNet status checks to prevent resource leaks |
| src/core/resource/common.go | Reduces random sleep duration to minimize deletion delays |
| src/core/common/label/label.go | Comments out debug logging for empty label data |
| docker-compose.yaml | Updates Docker image version to 0.11.8 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // waitForVNetReady waits for VNet to be in a ready state with timeout and retry mechanism | ||
| func waitForVNetReady(nsId string, vNetId string, reqID string) error { | ||
| const ( | ||
| maxRetries = 200 |
There was a problem hiding this comment.
The maxRetries value of 200 is a magic number. Consider making this configurable or define it as a named constant with documentation explaining the rationale for this specific value.
| maxRetries = 200 |
| const ( | ||
| maxRetries = 200 | ||
| retryInterval = 5 * time.Second | ||
| progressUpdateInterval = 10 // Update progress every 10 attempts (50 seconds) |
There was a problem hiding this comment.
The progressUpdateInterval value of 10 is a magic number. Consider making this configurable or defining it as a named constant to improve maintainability.
| time.Sleep(retryInterval) | ||
| } | ||
|
|
||
| return fmt.Errorf("timeout waiting for VNet '%s' to be ready after %d minutes", vNetId, (maxRetries*int(retryInterval.Seconds()))/60) |
There was a problem hiding this comment.
The timeout calculation (maxRetries*int(retryInterval.Seconds()))/60 is complex and hard to read. Consider extracting this calculation into a variable or constant for better readability.
| return fmt.Errorf("timeout waiting for VNet '%s' to be ready after %d minutes", vNetId, (maxRetries*int(retryInterval.Seconds()))/60) | |
| timeoutMinutes := (maxRetries * int(retryInterval.Seconds())) / 60 | |
| return fmt.Errorf("timeout waiting for VNet '%s' to be ready after %d minutes", vNetId, timeoutMinutes) |
| } | ||
| if len(labelData) == 0 { | ||
| log.Debug().Msg("labelData is empty") | ||
| //log.Debug().Msg("labelData is empty") |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in place. If this debug logging needs to be conditionally enabled, consider using a feature flag or configuration setting instead.
| //log.Debug().Msg("labelData is empty") |
|
/approve |
fix #2105