OCPBUGS-45027: No Events Generated after changing cluster-monitoring Label

[mansikulkarni96]

This commit fixes the controller to generate events when the cluster-monitoring label is not present or removed from the operator namespace.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@mansikulkarni96: This pull request references Jira Issue OCPBUGS-45027, which is invalid:

• expected the bug to target the "4.19.0" version, but no target version was set

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

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This commit fixes the controller to generate events when the cluster-monitoring label is not present or removed from the operator namespace.

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 openshift-eng/jira-lifecycle-plugin repository.

[mansikulkarni96]

/approve cancel

[mansikulkarni96]

/test aws-e2e-operator

[openshift-ci]

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

Test name	Commit	Details	Required	Rerun command
ci/prow/aws-e2e-operator	269583c	link	true	/test aws-e2e-operator

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-sigs/prow repository. I understand the commands that are listed here.

[jrvaldes]

@mansikulkarni96 thanks for fixing this.

Do you anticipate adding e2e test coverage for this scenario?

[jrvaldes]

consider adding a const for openshift.io/cluster-monitoring, and reuse it

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jrvaldes
Once this PR has been reviewed and has the lgtm label, please assign mansikulkarni96 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[mansikulkarni96]

@mansikulkarni96 thanks for fixing this.

Do you anticipate adding e2e test coverage for this scenario?

I don't lean towards it, as adding an e2e for an event log message may not be useful . But I am open to it and need inputs from the team.

[sebsoto]

I'm not following why the label check is no longer being made

[mansikulkarni96]

@sebsoto label check is being made in the reconciler, so we can generate events if the label has not been added to the namespace.

[sebsoto]

I guess I am not understanding the issue that is being solved.
It seems like the underlying bug is complaining about event generation indicating that monitoring is being disabled/enabled.

[sebsoto]

So in possibly in addition to this work, you would want to keep track of the current state, and if it changes, generate an event indicating it has changed. Right now an event is generated indicating it is not enabled.

windows-machine-config-operator/controllers/metric_controller.go

Lines 93 to 110 in 641adab

	labelValue := false
	wmcoNamespace, err := r.k8sclientset.CoreV1().Namespaces().Get(ctx, r.watchNamespace, metav1.GetOptions{})
	if err != nil {
	return false, fmt.Errorf("error getting operator namespace: %w", err)
	}
	// if the label exists, update value from default of false
	if value, ok := wmcoNamespace.Labels["openshift.io/cluster-monitoring"]; ok {
	labelValue, err = strconv.ParseBool(value)
	if err != nil {
	return false, fmt.Errorf("monitoring label must have a boolean value: %w", err)
	}
	}
	if !labelValue {
	r.recorder.Eventf(wmcoNamespace, v1.EventTypeWarning, "labelValidationFailed",
	"Cluster monitoring openshift.io/cluster-monitoring=true label is not enabled in %s namespace",
	r.watchNamespace)
	}
	return labelValue, nil

The followup question is, is this something that we want to do. I see some value in it.

[mansikulkarni96]

Correct that's the bug, this event was generated before the controller changes went in. Agreed, I see value in generating an event when there is a change.