-
Notifications
You must be signed in to change notification settings - Fork 183
qemu.go: retry if there is a race condition to bind port 2049 #4358
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.
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
Conversation
30b5cad to
ece4006
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a retry mechanism with a delay to handle a race condition when binding to a port. The change is logical and addresses the issue. I have one suggestion to improve the code's readability and maintainability by using constants for retry parameters and adopting the project's standard logger.
mantle/platform/qemu.go
Outdated
| maxRetries := 12 | ||
| for attempt := 1; attempt <= maxRetries; attempt++ { | ||
| l, err = net.Listen("tcp", address) | ||
| if err == nil { | ||
| l.Close() | ||
| break | ||
| } | ||
|
|
||
| fmt.Printf("Failed to listen on %s: %v, retrying (%d/%d)...\n", | ||
| address, err, attempt, maxRetries) | ||
| time.Sleep(time.Duration(5) * time.Second) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This retry logic is a good addition. To improve its readability and maintainability, I have a few suggestions:
- Define
maxRetriesand the sleep duration as constants. This makes them easier to find and change. - The explicit type conversion
time.Duration(5)is redundant since5is an untyped constant. - For consistency with the rest of the project, it would be better to use
plog.Warningffor logging instead offmt.Printf.
const maxRetries = 12
const retryDelay = 5 * time.Second
for attempt := 1; attempt <= maxRetries; attempt++ {
l, err = net.Listen("tcp", address)
if err == nil {
l.Close()
break
}
plog.Warningf("Failed to listen on %s: %v, retrying (%d/%d)...",
address, err, attempt, maxRetries)
time.Sleep(retryDelay)
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a retry loop to handle a race condition when binding to a network port, which is a sensible approach to the problem described. My feedback focuses on improving the code's readability and consistency with the project's existing style, specifically by using named constants for retry parameters and adhering to the established logging framework.
mantle/platform/qemu.go
Outdated
| maxRetries := 12 | ||
| for attempt := 1; attempt <= maxRetries; attempt++ { | ||
| l, err = net.Listen("tcp", address) | ||
| if err == nil { | ||
| l.Close() | ||
| break | ||
| } | ||
|
|
||
| fmt.Printf("Failed to listen on %s: %v, retrying (%d/%d)...\n", | ||
| address, err, attempt, maxRetries) | ||
| time.Sleep(time.Duration(5) * time.Second) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions to improve this retry logic:
- It's good practice to define magic numbers like
12and5as constants with descriptive names. This improves readability and maintainability. - The logging should use the package logger
ploginstead offmt.Printffor consistency.plog.Infofseems appropriate for a retry attempt. time.Duration(5) * time.Secondcan be simplified to5 * time.Second.
| maxRetries := 12 | |
| for attempt := 1; attempt <= maxRetries; attempt++ { | |
| l, err = net.Listen("tcp", address) | |
| if err == nil { | |
| l.Close() | |
| break | |
| } | |
| fmt.Printf("Failed to listen on %s: %v, retrying (%d/%d)...\n", | |
| address, err, attempt, maxRetries) | |
| time.Sleep(time.Duration(5) * time.Second) | |
| } | |
| const maxRetries = 12 | |
| const retryDelay = 5 * time.Second | |
| for attempt := 1; attempt <= maxRetries; attempt++ { | |
| l, err = net.Listen("tcp", address) | |
| if err == nil { | |
| l.Close() | |
| break | |
| } | |
| plog.Infof("Failed to listen on %s: %v, retrying (%d/%d)...", address, err, attempt, maxRetries) | |
| time.Sleep(retryDelay) | |
| } |
There is a race condition for `ostree.sync` and `kdump.crash.nfs` to bind port 2049, add retry if there is, instead of retrun err immediately. Fixes: coreos/rhel-coreos-config#89
ece4006 to
f8a3eac
Compare
|
/retest |
|
see #4117 |
|
Close this as there is a better workaround in #4361 |
There is a race condition for
ostree.syncandkdump.crash.nfsto bind port 2049, add retry if there is.Fixes: coreos/rhel-coreos-config#89