ACM-6596: Initialize controller for OperatorPolicy #160

JeffeyL · 2023-08-22T19:36:59Z

ref: https://issues.redhat.com/browse/ACM-6596

JustinKuli

I think this is a good scaffold for adding more features bit by bit! The main thing I'd like to see is to put it behind a feature flag, if that's not too much to ask for this PR (if it ends up being hard, we can talk about delaying it).

JustinKuli · 2023-08-23T13:53:27Z

controllers/operatorpolicy_controller.go

+ ctx context.Context, freq uint, elected <-chan struct{},
+) {
+ _ = ctx // currently unused


You can use an underscore in the function signature for the same effect:

Suggested change

ctx context.Context, freq uint, elected <-chan struct{},

) {

_ = ctx // currently unused

_ context.Context, freq uint, elected <-chan struct{},

) {

But actually, you can use the given context in the r.List( call below.

JustinKuli · 2023-08-23T14:05:22Z

controllers/operatorpolicy_controller.go

+ maxRetries := 3
+ for i := 1; i <= maxRetries; i++ {


I think we've tried this kind of retry thing before, but it eventually got removed (you've probably been digging through the code more recently than me though, so feel free to correct me). In my opinion, retries here aren't necessary - if it fails, it will just get retried next loop anyway.

Especially as the status becomes more complex (when there are more things that could cause errors) I think it will just be easier to return the error immediately, and handle the retry at a higher level.

I found where this kind of thing is in the normal config-policy-controller... it looks like it was useful there because of some non-idempotent stuff: if the config-policy should create something, but then can't record that it created it in its status, then there will be buggy behavior on a later reconcile because it incorrectly thinks it didn't create the resource.

So... let's keep an eye out for that kind of situation in the operator policy controller, and see if this ends up being helpful. I think a better approach to the config-policy situation might have been to add an annotation or label to the object that the config-policy-controller created, that way it could determine later whether it had created it or not even if the status didn't get updated correctly.

Thanks for pointing this out, I also believe that it would get retried the next loop. I'll keep an eye out for the situations you described in your last comment.

JustinKuli · 2023-08-23T14:11:45Z

controllers/operatorpolicy_controller.go

+ // when testing, evaluate at every interval regardless of policy status
+ if testFlag {
+ policy.Status.ComplianceState = policyv1.Compliant
+
+ err := r.updatePolicyStatus(policy)


I don't think this interacts with the shouldEvaluate logic the way you might want... It looks like it will only ever update the status if this testFlag is true, and there is no current way to turn that on. I think the flag can just be removed.

Oh yes definitely, having that if statement in there was an oversight from me, I had originally thought of using it in some way but it ended up not being of significant use.

JustinKuli · 2023-08-23T14:14:23Z

controllers/operatorpolicy_controller.go

+ // OperatorPlcChan a channel used to pass operator policies ready for update.
+ OperatorPlcChan chan *policyv1beta1.OperatorPolicy


I know I've seen this in other controllers, but I don't think it's used. Until I see a possible use for it, I'd rather keep it out of here.

JustinKuli · 2023-08-23T14:22:37Z

main.go

+ if err = OpReconciler.SetupWithManager(mgr); err != nil {
+ log.Error(err, "Unable to create controller", "controller", "OperatorPolicy")
+ os.Exit(1)
+ }


Can we put this behind a feature flag? Something that would default to false in parseOpts. Then when we want to test stuff with it, we can turn it on by editing the deployment or something.

I'll look into this.

JustinKuli · 2023-08-23T14:22:50Z

main.go

+ go func() {
+ OpReconciler.PeriodicallyExecOperatorPolicies(terminatingCtx, opts.frequency, mgr.Elected())
+ managerCancel()
+ }()


This should also go behind the feature flag.

JeffeyL · 2023-08-24T21:34:23Z

Changes:

Removed ctx from PeriodicallyExecOperatorPolicies, I checked out the commit that added it (here) and it seems to be related to having cleaner exit/uninstall behavior. It states in a comment that ctx should not be passed so I opted to not include it. May revisit this in the future if the operator policy controller requires similar behavior.
Removed the retries
Added a feature flag --enable-operator-policy, to enable put --enable-operator-policy=true in the launch script

JustinKuli

/hold

Looks good enough to me! Some minor things you could address, but if you want to just move forward with the next steps, feel free to unhold this.

JustinKuli · 2023-08-25T14:37:35Z

controllers/operatorpolicy_controller.go

+ OperatorControllerName string = "configuration-policy-controller"
+ OperatorCRDName string = "configurationpolicies.policy.open-cluster-management.io"


Nit: OperatorCRDName isn't used, and OperatorControllerName should probably be "operator-policy-controller"

Whoops, that's slightly embarrassing

JustinKuli · 2023-08-25T14:39:10Z

controllers/operatorpolicy_controller.go

+ Scheme *runtime.Scheme
+ Recorder record.EventRecorder


Nit: I don't know if this reconciler will need these, I think the recorder in particular emits events asynchronously (and unreliably).

Did a quick scan through the other controller repos and they all seem to include a scheme in the controller, but proceed not to use it within the reconcile logic (some of the test code does use it, however). The kubebuilder docs have schemes in their controllers as well, but they actually use them for some function calls. Either way, the inclusion/exclusion does not seem to affect the functionality of the controller in any way that I can tell, so it seems to be safe to remove for now, and may be brought back for testing later once that is a concern.

JustinKuli · 2023-08-25T17:47:13Z

It looks like it's failing test coverage, probably because with the feature flag, the new code never runs 😆 sorry about that.

Try adding the new flag to be true in https://github.com/open-cluster-management-io/config-policy-controller/blob/main/main_test.go#L19, that should get it running in the tests, which might just push it over the coverage threshold.

ref: https://issues.redhat.com/browse/ACM-6596 Signed-off-by: Jeffrey Luo <jeluo@redhat.com>

JustinKuli

/unhold

LGTM!

openshift-ci · 2023-08-25T19:28:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JeffeyL, JustinKuli

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~~ [JustinKuli]

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

openshift-ci bot added do-not-merge/work-in-progress dco-signoff: yes labels Aug 22, 2023

JeffeyL marked this pull request as ready for review August 22, 2023 19:43

openshift-ci bot removed the do-not-merge/work-in-progress label Aug 22, 2023

openshift-ci bot requested review from gparvin and JustinKuli August 22, 2023 19:43

JeffeyL force-pushed the ACM-6596 branch from faf8e59 to 9f5cb42 Compare August 22, 2023 19:45

JustinKuli requested changes Aug 23, 2023

View reviewed changes

openshift-ci bot assigned JustinKuli Aug 23, 2023

JeffeyL force-pushed the ACM-6596 branch from 9f5cb42 to 88df54e Compare August 24, 2023 21:28

JeffeyL requested a review from JustinKuli August 24, 2023 21:34

JeffeyL force-pushed the ACM-6596 branch from 88df54e to 728310d Compare August 25, 2023 06:40

JustinKuli previously approved these changes Aug 25, 2023

View reviewed changes

openshift-ci bot added do-not-merge/hold lgtm approved labels Aug 25, 2023

JeffeyL dismissed JustinKuli’s stale review via 97ce752 August 25, 2023 15:11

JeffeyL force-pushed the ACM-6596 branch from 728310d to 97ce752 Compare August 25, 2023 15:11

openshift-ci bot removed lgtm approved labels Aug 25, 2023

JeffeyL requested a review from JustinKuli August 25, 2023 15:43

ACM-6596: Initialize controller for OperatorPolicy

f52f4a9

ref: https://issues.redhat.com/browse/ACM-6596 Signed-off-by: Jeffrey Luo <jeluo@redhat.com>

JeffeyL force-pushed the ACM-6596 branch from 97ce752 to f52f4a9 Compare August 25, 2023 18:42

JustinKuli approved these changes Aug 25, 2023

View reviewed changes

openshift-ci bot added lgtm and removed do-not-merge/hold labels Aug 25, 2023

openshift-ci bot added the approved label Aug 25, 2023

openshift-merge-robot merged commit 4f3c09a into open-cluster-management-io:main Aug 25, 2023
4 checks passed

magic-mirror-bot bot mentioned this pull request Aug 25, 2023

🤖 Sync from open-cluster-management-io/config-policy-controller: #160 stolostron/config-policy-controller#572

Merged

JeffeyL mentioned this pull request Sep 7, 2023

OCM Member application open-cluster-management-io/community#184

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ACM-6596: Initialize controller for OperatorPolicy #160

ACM-6596: Initialize controller for OperatorPolicy #160

JeffeyL commented Aug 22, 2023

JustinKuli left a comment

JustinKuli Aug 23, 2023

JustinKuli Aug 23, 2023

JustinKuli Aug 23, 2023

JustinKuli Aug 23, 2023

JustinKuli Aug 23, 2023

JeffeyL Aug 23, 2023

JustinKuli Aug 23, 2023

JeffeyL Aug 23, 2023

JustinKuli Aug 23, 2023

JustinKuli Aug 23, 2023

JeffeyL Aug 23, 2023

JustinKuli Aug 23, 2023

JeffeyL commented Aug 24, 2023

JustinKuli left a comment

JustinKuli Aug 25, 2023

JeffeyL Aug 25, 2023

JustinKuli Aug 25, 2023

JeffeyL Aug 25, 2023

JustinKuli commented Aug 25, 2023

JustinKuli left a comment

openshift-ci bot commented Aug 25, 2023

		// OperatorPlcChan a channel used to pass operator policies ready for update.
		OperatorPlcChan chan *policyv1beta1.OperatorPolicy

		OperatorControllerName string = "configuration-policy-controller"
		OperatorCRDName string = "configurationpolicies.policy.open-cluster-management.io"

ACM-6596: Initialize controller for OperatorPolicy #160

ACM-6596: Initialize controller for OperatorPolicy #160

Conversation

JeffeyL commented Aug 22, 2023

JustinKuli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffeyL commented Aug 24, 2023

JustinKuli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinKuli commented Aug 25, 2023

JustinKuli left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 25, 2023