-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Retry Pod/RC updates in kubectl rolling-update #27509
Retry Pod/RC updates in kubectl rolling-update #27509
Conversation
} | ||
} | ||
|
||
const MaxRetries = 3 | ||
|
||
func AddDeploymentKeyToReplicationController(oldRc *api.ReplicationController, client client.Interface, deploymentKey, deploymentValue, namespace string, out io.Writer) (*api.ReplicationController, error) { | ||
fmt.Printf("calling AddDeploymentKeyToReplicationController\n") |
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.
remove this?
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.
Done
285b8b3
to
43c096d
Compare
} | ||
|
||
// retry retries fn using exponential backoff | ||
func retry(delay time.Duration, maxRetries int, fn func() bool) error { |
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.
Take a look at
func RetryOnConflict(backoff wait.Backoff, fn func() error) error { |
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.
Also we should make it clear under what scenario we should retry.
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.
done
a7a7c75
to
8b1dcdc
Compare
@pwittrock @Kargakis ptal |
// 2. applyUpdate | ||
// 3. Update the resource | ||
func updatePodWithRetries(c client.Interface, namespace string, pod *api.Pod, applyUpdate updatePodFunc) (*api.Pod, error) { | ||
oldPod := pod |
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.
I think you need to deep-copy here
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.
Fixed
@janetkuo squash and reapply lgtm |
98f2df7
to
ee81e5e
Compare
GCE e2e build/test passed for commit ee81e5e. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit ee81e5e. |
Automatic merge from submit-queue |
@Kargakis Thanks for doing the review |
…27509-upstream-release-1.2 Automated cherry pick of #27509
@janetkuo I think this is already in release-1.3 branch since it merged several days ago. I'm removing the cherrypick-candidate label. Re-add if i am mistaken. |
@erictune - fyi the label was also there because we wanted to cherry pick this into 1.2 (which was done yesterday). It's fine to remove it as this point. |
…-pick-of-#27509-upstream-release-1.2 Automated cherry pick of kubernetes#27509
…-pick-of-#27509-upstream-release-1.2 Automated cherry pick of kubernetes#27509
Fixes #27328
@kubernetes/kubectl