enhancement: Replace UpdateStatus() for ApplyStatus() to avoid informer errors

[marioferh]

Description

To set the status subresource, the operator make use of ApplyStatus() instead of UpdateStatus() to avoid informer errors.

Type of change

• CHANGE (fix or feature that would cause existing functionality to not work as expected)
• FEATURE (non-breaking change which adds functionality)
• BUGFIX (non-breaking change which fixes an issue)
• ENHANCEMENT (non-breaking change which improves existing functionality)
• NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

• replace UpdateStatus() for ApplyStatus() in order to avoid informer errors.

[simonpasquier]

To avoid too many back-and-forth with the e2e tests you might want to modify only one controller (e.g. Alertmanager) first.

[danielmellado]

getting a bit cryptic with all these 'trac', 'aac' and so but they come from the applyconfiguration changes 🗡️

[simonpasquier]

The call to stsReporter.Update() at L806 also sets some of the status fields so we need to adapt it.

[ArthurSens]

👋 -- Just out of curiosity, what errors are we seeing in UpdateStatus() that we're fixing here? :)

[marioferh]

👋 -- Just out of curiosity, what errors are we seeing in UpdateStatus() that we're fixing here? :)

if _, err = c.mclient.MonitoringV1().Alertmanagers(a.Namespace).UpdateStatus(ctx, a, metav1.UpdateOptions{}); err != nil { return errors.Wrap(err, "failed to update status subresource") }

Trying to update the operator status, the error: "failed to update status subresource" is shown and could be confused.

[simonpasquier]

To add more context, we're seeing this type of messages in the OpenShift's prometheus operator logs (I've seen in plain Kind clusters too):

level=error ts=2023-09-11T07:11:06.75456597Z caller=klog.go:116 component=k8s_client_runtime func=ErrorDepth msg="status \"prominstancens-allowlist-s0t7xg-2-137e5aff/instance\" failed: faile
d to update prometheus status subresource: Operation cannot be fulfilled on prometheuses.monitoring.coreos.com \"instance\": the object has been modified; please apply your changes to the la
test version and try again"

My guess is that since UpdateStatus() reads the Prometheus object from the cache, it may have a slightly outdated version with an old versionNumber. While the status eventually converges (e.g. the operator will requeue the status update), it's somehow disturbing for users to see errors in the logs. Using an apply operation of update should get rid of this annoyance.

[ArthurSens]

My guess is that since UpdateStatus() reads the Prometheus object from the cache, it may have a slightly outdated version with an old versionNumber. While the status eventually converges (e.g. the operator will requeue the status update), it's somehow disturbing for users to see errors in the logs. Using an apply operation of update should get rid of this annoyance.

Would it be possible to have the Status set to an older version if we use apply, even if for a little bit?

I don't really see a problem with that, just thinking out loud at the moment, but do you see that as a problem?

[simonpasquier]

@ArthurSens my assumption is that the update will retrigger a status update if required. In most cases, it should be a no-op.

[danielmellado]

nit: It doesn't hurt, but if it has only one usage I wouldn't bother creating a var for it. Do you plan to use this in any follow-up PRs? Don't bother changing it back just because of this, just curious.

[simonpasquier]

The same change will be applied to the Prometheus and Thanos controllers so we'll need a constant in the end.

[simonpasquier]

Great! Can you proceed with the other UpdateStatus() functions?

[simonpasquier]

The same change will be applied to the Prometheus and Thanos controllers so we'll need a constant in the end.

[simonpasquier]

Clarified offline with @marioferh. Let's merge this and PRs will follow for the other controllers.

[simonpasquier]

Thanks!