Bug 1659351 - Should prompt explicitly ‘Management State’ invalid in samplesresources.status
Summary: Should prompt explicitly ‘Management State’ invalid in samplesresources.status
Alias: None
Product: OpenShift Container Platform
Classification: Red Hat
Component: ImageStreams
Version: 4.1.0
Hardware: Unspecified
OS: Unspecified
Target Milestone: ---
: 4.1.0
Assignee: Gabe Montero
QA Contact: XiuJuan Wang
Depends On:
TreeView+ depends on / blocked
Reported: 2018-12-14 07:37 UTC by XiuJuan Wang
Modified: 2019-06-04 10:41 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: No Doc Update
Doc Text:
Clone Of:
Last Closed: 2019-06-04 10:41:16 UTC
Target Upstream Version:

Attachments (Terms of Use)

System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2019:0758 0 None None None 2019-06-04 10:41:21 UTC

Description XiuJuan Wang 2018-12-14 07:37:04 UTC
Description of problem:
Set ‘Management State‘ to an undefined value, only warns this in operator pod logs about "Unknown management state test specified; switch to Managed". Should prompt explicitly ‘Management State’ invalid in samplesresources.status

Version-Release number of selected component (if applicable):

version   0.0.1-2018-12-08-172651   True        False         33m       Cluster version is 0.0.1-2018-12-08-172651


How reproducible:

Steps to Reproduce:
1.Set managementState to an undefined value, such as 'test'
2.Check samplesresources
3.Check pod logs of operator

Actual results:

Only warns this in operator pod logs about "Unknown management state test specified; switch to Managed".

Expected results:
Should prompt explicitly ‘Management State’ invalid in samplesresources.status

Additional info:

Comment 1 Gabe Montero 2018-12-14 21:38:42 UTC
There is no "Invalid" value for management state in the api.  These are the only valid choices:

var (
	// Force means that the operator is actively managing its resources but will not block an upgrade
	// if unmet prereqs exist. This state puts the operator at risk for unsuccessful upgrades
	Force ManagementState = "Force"
	// Managed means that the operator is actively managing its resources and trying to keep the component active.
	// It will only upgrade the component if it is safe to do so
	Managed ManagementState = "Managed"
	// Unmanaged means that the operator will not take any action related to the component
	Unmanaged ManagementState = "Unmanaged"
	// Removed means that the operator is actively managing its resources and trying to remove all traces of the component
	Removed ManagementState = "Removed"

My thinking here was to force a default, workable state for the operator, vs. leaving the operator in limbo based on user error.

I'm inclined to keep the behavior as is .... @Ben - do you disagree?

Comment 2 Ben Parees 2018-12-14 21:44:37 UTC
QE is suggesting we report a status.condition of "invalid management state", not that we set the management state value to invalid.

I don't have a strong opinion on whether we treat an unknown value as "managed" or "unmanaged" but we should definitely update the samples resource status condition to indicate the specified value is unknown (and indicate how we are treating it instead).  Logs are for us, status conditions are for users.

I can see arguments for "defaulting" to both managed and unmanaged in the face of an unknown value.

Comment 3 Gabe Montero 2018-12-14 21:59:39 UTC
Yes I understood that as well ...should have elaborated ... since I was fixing their value, I saw no need of putting a condition that said the value you provided was bad and we fixed it and it is no longer present.

If we do need to further indicate the fixing of the value, there are a couple of ways to go:
1) use the existing ConfigValid condition, mark things as true, but add a message saying we changed the value on you
   a) the fixing of the value to make setting configvalid to unknown in this case unnecessary
2) add a new condition ... maybe "FixedManagementField" ... not a big fan at first blush
3) some other option i'm not thinking of

Comment 4 Ben Parees 2018-12-14 22:03:00 UTC
I can live with (1).

Comment 5 Gabe Montero 2019-01-08 21:05:36 UTC
OK I've got a branch up with commit https://github.com/gabemontero/cluster-samples-operator/commit/d35172c54bdeecc8eda0ea32579e33c550c65c16 that 
implements item 1) from https://bugzilla.redhat.com/show_bug.cgi?id=1659351#c3

It is based off of PR https://github.com/openshift/cluster-samples-operator/pull/71 which is still under review, so I'm holding off on creating 
the PR.

Comment 6 Gabe Montero 2019-01-09 14:12:16 UTC
PR https://github.com/openshift/cluster-samples-operator/pull/73 has been created

Comment 7 XiuJuan Wang 2019-01-11 12:38:41 UTC
Can't reproduce this bug 
#oc get clusterversion 
NAME      VERSION                           AVAILABLE   PROGRESSING   SINCE     STATUS
version   4.0.0-0.alpha-2019-01-11-075335   True        False         1h        Cluster version is 4.0.0-0.alpha-2019-01-11-075335

$ oc describe configs.samples.operator.openshift.io instance 

 Management State:  test
  Skipped Imagestreams:
  Skipped Templates:
  Version:  4.0.0-alpha1-137b53463
    Last Transition Time:  2019-01-11T11:57:32Z
    Last Update Time:      2019-01-11T11:57:32Z
    Status:                True
    Type:                  SamplesExist
    Last Transition Time:  2019-01-11T11:55:42Z
    Last Update Time:      2019-01-11T11:55:42Z
    Status:                True
    Type:                  ImportCredentialsExist
    Last Transition Time:  2019-01-11T12:35:32Z
    Last Update Time:      2019-01-11T12:35:32Z
    Message:               Unexpected management state test received, switching to Managed
    Status:                True
    Type:                  ConfigurationValid

Comment 10 errata-xmlrpc 2019-06-04 10:41:16 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.


Note You need to log in before you can comment on or make changes to this bug.