Bug 1978662: Set a degraded message when persistent storage is not configured #1270

fpetkovski · 2021-07-08T07:32:46Z

When persistent storage is not configured for prometheus, upgrades and cluster disruptions can lead to data loss.
In order to make cluster admins aware of this problem, this PR adds a message to the degraded condition indicating this problem. The value of the condition is kept as degraded=false since there is no real degradation with the cluster monitoring.

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

openshift-ci · 2021-07-08T07:32:50Z

@fpetkovski: This pull request references Bugzilla bug 1978662, which is invalid:

expected the bug to target the "4.9.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

WIP: Bug 1978662: Set a degraded message when persistent storage is not configured

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fpetkovski · 2021-07-08T07:39:58Z

/bugzilla refresh

openshift-ci · 2021-07-08T07:40:06Z

@fpetkovski: This pull request references Bugzilla bug 1978662, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @juzhao

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-07-08T07:43:21Z

@fpetkovski: This pull request references Bugzilla bug 1978662, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @juzhao

In response to this:

WIP: Bug 1978662: Set a degraded message when persistent storage is not configured

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

arajkumar

Added a nit and minor comment, rest lgtm :)

pkg/operator/operator.go

pkg/manifests/config.go

jan--f

Two issues for this patch inline.

I'm wondering if we maybe should introduce a func (r *StatusReporter) SetDegraded function for this and set it explicitly where appropriate. We talked before about setting degraded on other conditions and generally improve the status reporting. Not needed for this PR but also wouldn't hurt I think.

jan--f · 2021-07-08T11:17:04Z

pkg/client/status_reporter.go

@@ -77,7 +77,7 @@ func (r *StatusReporter) SetDone() error {
 	conditions := newConditions(co.Status, r.version, time)
 	conditions.setCondition(v1.OperatorAvailable, v1.ConditionTrue, "Successfully rolled out the stack.", "RollOutDone", time)
 	conditions.setCondition(v1.OperatorProgressing, v1.ConditionFalse, "", "", time)
-	conditions.setCondition(v1.OperatorDegraded, v1.ConditionFalse, "", "", time)
+	conditions.setCondition(v1.OperatorDegraded, v1.ConditionFalse, degradedConditionMessage, "", time)


We probably should set the actual condition here depending on the message passed?

Also this should add a reason. Maybe PrometheusNoDataPersistence or something like it

We probably should set the actual condition here depending on the message passed?

Do you mean we should set the condition to true when a message other than "" is passed?
I probably should have provided context in the PR description, the requirement is to actually keep the condition to degraded=false but set a message for cluster admins to be aware of the state. I update the PR description to contain that information.

Also this should add a reason. Maybe PrometheusNoDataPersistence or something like it

Good point, added the reason as well.

Do you mean we should set the condition to true when a message other than "" is passed?

Yes that is the convention afaiu, at least in the other direction (don't set the condition without also setting a reason). To me it makes sense too, but I'll try to find some according docs.

@wking we could not find any documentation on this particular topic. Is it okay for an operator to set a message for the degraded condition but keep the value to false. This is what we discussed for CMO when there is no persistent storage enabled to indicate to the admin that metrics might be lost.

Based on Trevor's reply in slack, setting a message for the degraded condition while keeping the value to false is a valid state for the operator status. A good practice in these situations is to add a link to the documentation on how to fix the underlying problem.

openshift-ci · 2021-07-08T12:02:48Z

@fpetkovski: This pull request references Bugzilla bug 1978662, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @juzhao

In response to this:

Bug 1978662: Set a degraded message when persistent storage is not configured

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fpetkovski · 2021-07-08T12:25:26Z

@jan--f I agree that the workaround is not ideal, but also I think reworking the status reporting is a bit outside of the scope of what is requested from us in this BZ. I don't think introducing a func (r *StatusReporter) SetDegraded is really appropriate in this case because we need to set degraded to False and at the same time display message for the degraded condition.

What we might need is some sort of a status builder, or some other mechanism to decouple the status reporter from the status itself. Right now both the status state and the status update is handled through the same abstraction.

Do you know if we already have a ticket for reworking the status reporting? I would be more than happy to pair on that one and see how we can improve the current situation.

jan--f · 2021-07-09T07:40:03Z

@jan--f I agree that the workaround is not ideal, but also I think reworking the status reporting is a bit outside of the scope of what is requested from us in this BZ. I don't think introducing a func (r *StatusReporter) SetDegraded is really appropriate in this case because we need to set degraded to False and at the same time display message for the degraded condition.

What we might need is some sort of a status builder, or some other mechanism to decouple the status reporter from the status itself. Right now both the status state and the status update is handled through the same abstraction.

Do you know if we already have a ticket for reworking the status reporting? I would be more than happy to pair on that one and see how we can improve the current situation.

I don't think we have an overarching ticket. But agreed, this is really not in scope for this ticket.

simonpasquier · 2021-07-13T15:12:56Z

I don't find any ticket to rewrite the status reporter part but I agree that it can be improved (I presume that the code originates from times when the ClusterOperator semantics weren't well defined).

arajkumar

lgtm

fpetkovski · 2021-07-16T13:29:27Z

/hold until we get confirmation from @wking

fpetkovski · 2021-07-19T07:21:00Z

/unhold

arajkumar

lgtm

fpetkovski · 2021-07-19T09:27:40Z

/retest

fpetkovski · 2021-07-20T07:01:32Z

/retest

simonpasquier · 2021-07-20T07:42:02Z

/lgtm

pkg/client/status_reporter.go

Co-authored-by: Simon Pasquier <[email protected]>

simonpasquier

/lgtm

openshift-ci · 2021-07-20T10:22:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arajkumar, fpetkovski, simonpasquier

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~~ [arajkumar,fpetkovski,simonpasquier]

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

openshift-ci · 2021-07-20T10:25:55Z

@fpetkovski: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-single-node	`9c7cddc`	link	`/test e2e-aws-single-node`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2021-07-20T10:30:11Z

/retest-required