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

Fix deployment timeout reporting #52127

Merged
merged 1 commit into from
Sep 8, 2017
Merged

Fix deployment timeout reporting #52127

merged 1 commit into from
Sep 8, 2017

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented Sep 7, 2017

If the previous condition has been a successful rollout then we
shouldn't try to estimate any progress. Scenario:

  • progressDeadlineSeconds is smaller than the difference between
    now and the time the last rollout finished in the past.
  • the creation of a new ReplicaSet triggers a resync of the
    Deployment prior to the cached copy of the Deployment getting
    updated with the status.condition that indicates the creation
    of the new ReplicaSet.

The Deployment will be resynced and eventually its Progressing
condition will catch up with the state of the world.

Fixes #49637

I will also cherry-pick this back to 1.7.

Release note:

@0xmichalis 0xmichalis added this to the v1.8 milestone Sep 7, 2017
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 7, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kargakis

Associated issue: 49637

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note-none Denotes a PR that doesn't merit a release note. labels Sep 7, 2017
Copy link
Member

@kow3ns kow3ns left a comment

Choose a reason for hiding this comment

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

LGTM accept for comment nit.

// cached copy of the Deployment getting updated with the status.condition that indicates
// the creation of the new ReplicaSet.
//
// In that case, we are going to falsely indicate that the Deployment has failed syncing.
Copy link
Member

Choose a reason for hiding this comment

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

The last line of this comment is a little unclear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, now that I am reading it again, indeed it is. Updated.

If the previous condition has been a successful rollout then we
shouldn't try to estimate any progress. Scenario:

* progressDeadlineSeconds is smaller than the difference between
  now and the time the last rollout finished in the past.
* the creation of a new ReplicaSet triggers a resync of the
  Deployment prior to the cached copy of the Deployment getting
  updated with the status.condition that indicates the creation
   of the new ReplicaSet.

The Deployment will be resynced and eventually its Progressing
condition will catch up with the state of the world.

Signed-off-by: Michail Kargakis <mkargaki@redhat.com>
@0xmichalis 0xmichalis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 629fea3 into kubernetes:master Sep 8, 2017
@0xmichalis 0xmichalis deleted the do-not-report-timeout branch September 8, 2017 18:39
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Sep 11, 2017
Automatic merge from submit-queue

UPSTREAM: 52127: Fix deployment timeout reporting

If the previous condition has been a successful rollout then we
shouldn't try to estimate any progress. Scenario:

* progressDeadlineSeconds is smaller than the difference between
  now and the time the last rollout finished in the past.
* the creation of a new ReplicaSet triggers a resync of the
  Deployment prior to the cached copy of the Deployment getting
  updated with the status.condition that indicates the creation
   of the new ReplicaSet.

The Deployment will be resynced and eventually its Progressing
condition will catch up with the state of the world.

Signed-off-by: Michail Kargakis <mkargaki@redhat.com>

Cherry-pick of kubernetes/kubernetes#52127
/assign @mfojtik @tnozicka 
/area apps
/kind bug
@wojtek-t wojtek-t added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate labels Sep 15, 2017
@wojtek-t wojtek-t modified the milestones: v1.8, v1.7 Sep 15, 2017
k8s-github-robot pushed a commit that referenced this pull request Sep 16, 2017
Automatic merge from submit-queue.

Fix deployment timeout reporting

If the previous condition has been a successful rollout then we
shouldn't try to estimate any progress. Scenario:

* progressDeadlineSeconds is smaller than the difference between
  now and the time the last rollout finished in the past.
* the creation of a new ReplicaSet triggers a resync of the
  Deployment prior to the cached copy of the Deployment getting
  updated with the status.condition that indicates the creation
   of the new ReplicaSet.

The Deployment will be resynced and eventually its Progressing
condition will catch up with the state of the world.

Signed-off-by: Michail Kargakis <mkargaki@redhat.com>

Cherry-pick of #52127 to 1.7

Fixes #49637

@kubernetes/sig-apps-pr-reviews 

```release-note
Fixed an issue reporting lack of progress for a deployment prematurely
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants