Skip to content
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

Merged

Conversation

janetkuo
Copy link
Member

Fixes #27328

@kubernetes/kubectl

Analytics

@janetkuo janetkuo added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/flake Categorizes issue or PR as related to a flaky test. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 16, 2016
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 16, 2016
}
}

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 16, 2016
@janetkuo janetkuo force-pushed the retry-update-e2e-rolling-update branch from 285b8b3 to 43c096d Compare June 16, 2016 18:03
}

// retry retries fn using exponential backoff
func retry(delay time.Duration, maxRetries int, fn func() bool) error {
Copy link
Member

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 {
. We probably want to copy this or move it into a util pkg.

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@janetkuo janetkuo force-pushed the retry-update-e2e-rolling-update branch 3 times, most recently from a7a7c75 to 8b1dcdc Compare June 17, 2016 21:58
@janetkuo
Copy link
Member Author

@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
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@0xmichalis 0xmichalis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2016
@0xmichalis
Copy link
Contributor

@janetkuo squash and reapply lgtm

@0xmichalis 0xmichalis removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2016
@janetkuo janetkuo force-pushed the retry-update-e2e-rolling-update branch from 98f2df7 to ee81e5e Compare June 21, 2016 23:08
@janetkuo janetkuo added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jun 21, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 21, 2016

GCE e2e build/test passed for commit ee81e5e.

@janetkuo janetkuo added this to the v1.3 milestone Jun 22, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jun 22, 2016

GCE e2e build/test passed for commit ee81e5e.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 24c3be1 into kubernetes:master Jun 22, 2016
@pwittrock
Copy link
Member

@Kargakis Thanks for doing the review

@roberthbailey roberthbailey added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 24, 2016
roberthbailey added a commit that referenced this pull request Jun 24, 2016
…27509-upstream-release-1.2

Automated cherry pick of #27509
@erictune
Copy link
Member

@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.

@roberthbailey
Copy link
Contributor

@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.

@roberthbailey roberthbailey added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jun 25, 2016
@roberthbailey roberthbailey modified the milestones: v1.2, v1.3 Jun 25, 2016
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
…-pick-of-#27509-upstream-release-1.2

Automated cherry pick of kubernetes#27509
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
…-pick-of-#27509-upstream-release-1.2

Automated cherry pick of kubernetes#27509
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. kind/flake Categorizes issue or PR as related to a flaky test. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants