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.

Sign up for GitHub

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

Jump to bottom

status: catch a few more Progressing cases #143

Merged
merged 1 commit into from Apr 10, 2019
Merged

status: catch a few more Progressing cases #143

merged 1 commit into from Apr 10, 2019

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Apr 9, 2019

The easiest way to tell that a daemonset is progressing is that UpdatedNumberScheduled is less than Desired.

Also, catch the case where the daemonset update hasn't yet been observed by the controller.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 9, 2019
@squeed
Copy link
Contributor Author

squeed commented Apr 9, 2019

/hold
Wanted to submit this for review now, but ran out of time to completely test it.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 9, 2019
@squeed
Copy link
Contributor Author

squeed commented Apr 9, 2019

context: Clayton noticed in https://bugzilla.redhat.com/show_bug.cgi?id=1695200 that LastTransitionTime for Progressing wasn't bumped after an update.

@squeed
Copy link
Contributor Author

squeed commented Apr 10, 2019

Still not sure if / how to catch the (admittedly unlikely) case where we update a DS then go offline until the controller has completely rolled out the change. The only thing I can think of is persisting observedgeneration somewhere, which feels yucky.

@squeed
status: catch a few more Progressing cases
5693dbe 
The easiest way to tell that a daemonset is progressing is that
UpdatedNumberScheduled is less than Desired.

Also, catch the case where the daemonset update hasn't yet been observed
by the controller.
@squeed
Copy link
Contributor Author

squeed commented Apr 10, 2019

Updated, tested with a running cluster and verified that progressing had a useful status. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1695200

/hold cancel
@danwinship PTAL, thanks!

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2019
@danwinship
Copy link
Contributor

danwinship commented Apr 10, 2019 •
edited

Still not sure if / how to catch the (admittedly unlikely) case where we update a DS then go offline until the controller has completely rolled out the change. The only thing I can think of is persisting observedgeneration somewhere, which feels yucky.

Hm... compare ds.Status.Conditions[something].LastTransitionTime with operator.Status.Conditions["Progressing"].LastTransitionTime?

@squeed
Copy link
Contributor Author

squeed commented Apr 10, 2019

Hm... compare ds.Status.Conditions[something].LastTransitionTime with operator.Status.Conditions["Progressing"].LastTransitionTime?

It does't look like the DaemonSet controller is actually setting any Status.Condition[] entries yet.

@danwinship
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2019
@openshift-merge-robot openshift-merge-robot merged commit 6883bb6 into openshift:master Apr 10, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, squeed

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

phoracek added a commit to phoracek/cluster-network-addons-operator that referenced this pull request Apr 18, 2019
@phoracek
status: catch a few more Progressing cases
db34b79 
openshift/cluster-network-operator#143:

The easiest way to tell that a daemonset is progressing is that
UpdatedNumberScheduled is less than Desired.

Also, catch the case where the daemonset update hasn't yet been observed
by the controller.

Signed-off-by: Petr Horacek <phoracek@redhat.com>
@phoracek phoracek mentioned this pull request Apr 18, 2019
Merged
@danwinship danwinship mentioned this pull request Apr 23, 2019
Merged
@phoracek phoracek mentioned this pull request Jun 3, 2019
Closed
11 tasks
@squeed squeed deleted the progressing-time branch June 12, 2019 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship left review comments

@dcbw dcbw Awaiting requested review from dcbw

@knobunc knobunc Awaiting requested review from knobunc

Assignees

@danwinship danwinship

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@squeed @danwinship @openshift-ci-robot @openshift-merge-robot