Adjust clusteroperator msgs to https://github.com/openshift/cluster-v… #70

gabemontero · 2018-12-18T19:11:37Z

…ersion-operator/blob/master/docs/dev/clusteroperator.md#what-should-an-operator-report-with-clusteroperator-custom-resource

Fixes #69

@bparees @smarterclayton

gabemontero · 2018-12-18T21:09:57Z

getting ci setup errors so far, but have some changes coming based on local testing

/hold

gabemontero · 2018-12-18T22:43:06Z

yeah @bparees @smarterclayton don't bother reviewing until I push my next commit and announce this is viable

pkg/operatorstatus/operatorstatus.go

bparees · 2018-12-18T22:56:02Z

yeah @bparees @smarterclayton don't bother reviewing until I push my next commit and announce this is viable

sorry, didn't see this in time :)

gabemontero · 2018-12-18T22:58:55Z

On Tue, Dec 18, 2018 at 5:56 PM Ben Parees ***@***.***> wrote: yeah @bparees <https://github.com/bparees> @smarterclayton <https://github.com/smarterclayton> don't bother reviewing until I push my next commit and announce this is viable sorry, didn't see this in time :)

no worries ... I was a bit tardy in posting it ... and as a suggestion, read and cross reference with @smarterclayton 's examples at https://github.com/openshift/cluster-version-operator/blob/master/docs/dev/clusteroperator.md#conditions Hopefully it helps explain some of the choices made here (assuming you and I interpret that doc the same way).

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADbadAjyUBteKU8195Nsqn1aaF3Ployaks5u6XKDgaJpZM4ZY544> .

gabemontero · 2018-12-19T02:08:35Z

OK the installer (both aws and libvirt) quit working for me between the office and home, so I couldn't finish testing, but I think I got far enough along to at least start reviewing.

Hopefully I can test tomorrow AM in conjunction with iterating over any review we get to over PTO (which starts tomorrow PM for me, but I plan on spending some cycles over PTO).

@bparees ^^

gabemontero · 2018-12-19T03:56:56Z

got cluster install working again @bparees but have found some things that need to fix ... so please consider "paused" again until I update

gabemontero · 2018-12-19T18:51:49Z

OK things look good in my local testing now. Both initial startup and release upgrade flows.

@bparees ptal whenever you have the chance

pkg/operatorstatus/operatorstatus.go

…ersion-operator/blob/master/docs/dev/clusteroperator.md#what-should-an-operator-report-with-clusteroperator-custom-resource Fixes openshift#69

gabemontero · 2018-12-27T04:46:07Z

OK I've pushed updates @bparees .

overall better organized and cleaner I think ... see if you agree
Also a little more settled on the logic around determining state after re-review
Added a few Conditions per discussion above to help manage state and errors
I don't have a "truth table" documented in one place, but I think the comments around various decisions are a reasonable substitute ... see if you agree
some minor fixes,
and also a change to the imagestream retry between samples upserts and setting of the image import condition to true ... it worked better in local testing (started seeing fragility in the counter based retry), and the operator state being maintained can be lost with no ill consequence

gabemontero · 2019-01-02T15:00:34Z

bump @bparees .... happy new year

bparees

i'm loving all these comments :)

bparees · 2019-01-02T15:54:38Z

pkg/apis/samplesoperator/v1alpha1/types.go

+	// scheduled imports for example to fail ... the imagestream "state" could
+	// be sufficiently compromised, so we'll flag false there
+	if needCreds {
+		return falseRC, falseMsg


still not clear on why this isn't based on whether the samplesexist==true && importcomplete? isn't that what can resolve whether the imagestream state actually is compromised or not?

(sorry if i'm dragging us back into a discussion we resolved pre-holiday)

(or per your comments below, shouldn't it be checking notAtAnyVersionYet? (if we're at some version, then we're available, even if we currently "needCreds" to move to the desired version)

Hmmmmmm ... so in a) taking in your point, and b) rereading (yet again) https://github.com/openshift/cluster-version-operator/blob/master/docs/dev/clusteroperator.md#conditions, I think there are these considerations here:

An API error during the initial install

We have installed at least once, are still at the installed version, but now have an error of some sort (could be bad api server interactions, needs creds for rhel, or the config is invalid

Item 2), but are migrating to a new version vs. staying at the installed version

For 1), I'll keep the empty message, and false ... details in the other two conditions

For 3), per the guideline, we need to set available to false, with the unable to reach message

For 2), as you say, we should set available to true, with the ok at the version in question, where again the error will show up in the failing/progressing messages

To get to this behavior, I think we

add spec.Version/status.Version comparison, accounting for not initially installed, to determine if we are migrating versions

if migrating, ANY error condition means we return false with the unable to reach message

if not migrating, we just leverage not any version, etc. as you note below

Thoughts? ... I'll work on the update if we agree here ...

thanks

per hallway discussion we'll revisit this after there is org-wide agreement on how failing/progressing/available should be used.

OK assuming openshift/cluster-version-operator#73 holds up, scenario 3) goes away @bparees

If so, I'll create a new PR to get rid of the apiError and needsCreds conditionals.

bparees · 2019-01-02T18:34:42Z

/lgtm

openshift-ci-robot · 2019-01-02T18:34:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, gabemontero

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:

~~OWNERS~~ [bparees,gabemontero]

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

openshift-ci-robot · 2019-01-02T18:34:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, gabemontero

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:

~~OWNERS~~ [bparees,gabemontero]

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

openshift-bot · 2019-01-02T20:15:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

gabemontero · 2019-01-02T21:48:44Z

/retest

openshift-bot · 2019-01-03T00:17:06Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-01-03T02:18:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 18, 2018

openshift-ci-robot requested review from coreydaley, legionus and dmage December 18, 2018 19:11

gabemontero force-pushed the msgs-clayton-will-like branch from 137f4b7 to 15983d3 Compare December 18, 2018 20:27

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 18, 2018

gabemontero added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 18, 2018

bparees reviewed Dec 18, 2018

View reviewed changes

pkg/operatorstatus/operatorstatus.go Outdated Show resolved Hide resolved

bparees requested changes Dec 18, 2018

View reviewed changes

pkg/operatorstatus/operatorstatus.go Outdated Show resolved Hide resolved

gabemontero force-pushed the msgs-clayton-will-like branch from 15983d3 to 8ae1a7d Compare December 19, 2018 02:04

openshift-ci-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 Dec 19, 2018

gabemontero force-pushed the msgs-clayton-will-like branch from 8ae1a7d to c97f4c8 Compare December 19, 2018 18:50

bparees requested changes Dec 19, 2018

View reviewed changes

pkg/operatorstatus/operatorstatus.go Outdated Show resolved Hide resolved

pkg/operatorstatus/operatorstatus.go Outdated Show resolved Hide resolved

gabemontero added 2 commits December 26, 2018 19:07

Adjust clusteroperator msgs to https://github.com/openshift/cluster-v…

a8850a5

…ersion-operator/blob/master/docs/dev/clusteroperator.md#what-should-an-operator-report-with-clusteroperator-custom-resource Fixes openshift#69

add import image error condition; improve imagestream watch event retry

a8ea52b

gabemontero force-pushed the msgs-clayton-will-like branch from c97f4c8 to a8ea52b Compare December 27, 2018 04:40

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 27, 2018

bparees requested changes Jan 2, 2019

View reviewed changes

openshift-ci-robot assigned bparees Jan 2, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. labels Jan 2, 2019

gabemontero removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 2, 2019

openshift-merge-robot merged commit 85ee5a9 into openshift:master Jan 3, 2019

gabemontero deleted the msgs-clayton-will-like branch January 3, 2019 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust clusteroperator msgs to https://github.com/openshift/cluster-v… #70

Adjust clusteroperator msgs to https://github.com/openshift/cluster-v… #70

gabemontero commented Dec 18, 2018

gabemontero commented Dec 18, 2018

gabemontero commented Dec 18, 2018

bparees commented Dec 18, 2018

gabemontero commented Dec 18, 2018 via email

gabemontero commented Dec 19, 2018

gabemontero commented Dec 19, 2018

gabemontero commented Dec 19, 2018

gabemontero commented Dec 27, 2018

gabemontero commented Jan 2, 2019

bparees left a comment

bparees Jan 2, 2019

bparees Jan 2, 2019

gabemontero Jan 2, 2019

bparees Jan 2, 2019

gabemontero Jan 2, 2019

bparees commented Jan 2, 2019

openshift-ci-robot commented Jan 2, 2019

openshift-ci-robot commented Jan 2, 2019

openshift-bot commented Jan 2, 2019

gabemontero commented Jan 2, 2019

openshift-bot commented Jan 3, 2019

openshift-bot commented Jan 3, 2019

Adjust clusteroperator msgs to https://github.com/openshift/cluster-v… #70

Adjust clusteroperator msgs to https://github.com/openshift/cluster-v… #70

Conversation

gabemontero commented Dec 18, 2018

gabemontero commented Dec 18, 2018

gabemontero commented Dec 18, 2018

bparees commented Dec 18, 2018

gabemontero commented Dec 18, 2018 via email

gabemontero commented Dec 19, 2018

gabemontero commented Dec 19, 2018

gabemontero commented Dec 19, 2018

gabemontero commented Dec 27, 2018

gabemontero commented Jan 2, 2019

bparees left a comment

Choose a reason for hiding this comment

bparees Jan 2, 2019

Choose a reason for hiding this comment

bparees Jan 2, 2019

Choose a reason for hiding this comment

gabemontero Jan 2, 2019

Choose a reason for hiding this comment

bparees Jan 2, 2019

Choose a reason for hiding this comment

gabemontero Jan 2, 2019

Choose a reason for hiding this comment

bparees commented Jan 2, 2019

openshift-ci-robot commented Jan 2, 2019

openshift-ci-robot commented Jan 2, 2019

openshift-bot commented Jan 2, 2019

gabemontero commented Jan 2, 2019

openshift-bot commented Jan 3, 2019

openshift-bot commented Jan 3, 2019