Bug 1965283: Static Resources Controller for Sync #216

adambkaplan · 2021-06-01T18:53:46Z

Refactor ocm-o to use the static resources controller to sync static
YAML assets.
Add infomers for cluster-scoped objects, openshift-infra, and
kube-system as a namespace to use an informer to fetch core k8s
objects.
Use cached ConfigMapsGetter in the main workload sync controller
Don't set unmanaged/removed messages. The status controller is now
responsible for setting the cluster operator states for managed/
unmanaged. The existing code now collides with the new static
resource sync controller.
Increase rate limit for k8s API calls. This ensures the static
resource sync controller is not throttled when reconciling RBAC
objects.

- Update openshift/library-go to latest on master (b9c317a319e0) - go mod vendor, add static resource sync controller

openshift-ci · 2021-06-02T17:23:37Z

@adambkaplan: This pull request references Bugzilla bug 1965283, 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.8.0) matches configured target release for branch (4.8.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @wewang58

In response to this:

Bug 1965283: Static Resources Controller for Sync

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.

adambkaplan

/assign @gabemontero

I believe you had last touched the bits around the status reporting - this was changed a bit with the addition of the new controller.

adambkaplan · 2021-06-02T17:28:09Z

pkg/operator/operator.go

@@ -51,7 +50,7 @@ func NewOpenShiftControllerManagerOperator(
 	targetImagePullSpec string,
 	operatorConfigInformer operatorinformersv1.OpenShiftControllerManagerInformer,
 	proxyInformer configinformerv1.ProxyInformer,
-	kubeInformersForOpenshiftControllerManager informers.SharedInformerFactory,
+	kubeInformers v1helpers.KubeInformersForNamespaces,


We use this struct with the static resource sync controller and the resource sync controller, too.

@adambkaplan - would it be possible to specify the repos(s) / packages(s) where the static resource sync controller and resource sync controller are located? I'm guessing they are in library-go, but some quick scans are not proving fruitful to me.

I'd like to do some cross referencing as part of the review.

Thanks

wait I may have found it later in the review ... 09eb355#diff-0d623dfd885adb20f991bda4c2453aebd732ca6dbb4d1d4be6e79805c3b48de6R129

yep, that is it

adambkaplan · 2021-06-02T17:29:54Z

pkg/operator/operator.go

-		if _, err := c.operatorConfigClient.OpenShiftControllerManagers().UpdateStatus(context.TODO(), operatorConfig, metav1.UpdateOptions{}); err != nil {
-			return err
-		}
-	}


The status sync controller takes care of the CVO status reporting for us if the operator moves to Unamanged or Removed.

adambkaplan · 2021-06-02T17:32:19Z

pkg/operator/starter.go

+	if highRateLimitProtoKubeConfig.Burst < 100 {
+		highRateLimitProtoKubeConfig.Burst = 100
+	}
+	kubeClient, err := kubernetes.NewForConfig(highRateLimitProtoKubeConfig)


The static resource sync controller unfortunately doesn't use listers when reconciling RBAC objects.
This is problematic for ocm because we reconcile a lot of roles/clusterroles and their role bindings.
Long term we should be the ones to improve this in library-go, but for the interim we can increase the QPS and reduce the instances where the sync controller is throttled.

So you are specifically referring to https://github.com/openshift/library-go/blob/c537a5b2af0196a9a235ccecc0d05c2583899f3d/pkg/operator/resource/resourceapply/generic.go#L150 as an example @adambkaplan, correct ?

If so, a commit specific link to the file in question might help us to quickly remember where to go if we ever get to this TODO

I think if I update the comment to use StaticResourceController then we should be ok (it is initialized later in this method).

sorry I'm not following how updating the comment to use StaticResourceController pertains to my question around confirming where the shortcoming in library-go is and ask to somehow point to the location in that code for future reference

Updated the comments - hopefully things are a little bit clearer.

adambkaplan · 2021-06-02T17:32:56Z

pkg/operator/starter.go

+		"",
+		util.TargetNamespace,
+		util.OperatorNamespace,
+		util.UserSpecifiedGlobalConfigNamespace,
+		util.InfraNamespace,
+		metav1.NamespaceSystem,


New additions to this list:

Cluster scoped ("")

openshift-infra

kube-system

adambkaplan · 2021-06-02T17:36:47Z

pkg/operator/starter.go

@@ -105,10 +126,40 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller
 		controllerConfig.EventRecorder,
 	)

+	staticResourceController := staticresourcecontroller.NewStaticResourceController(


This is a simple controller that takes a bag of YAMLs and applies them, and reports status to the operator config object's status. That gets rolled up to the CVO object via the status sync controller.

added the TODO here to make it clear where in library-go we need to add a future enhancement.

gabemontero

Nice pulling in a relatively newer library-go function @adambkaplan

Minor comment / constant suggestions around the parameter for cluster scoped and the future TODO wrt rbac listers in the static resource controller. Otherwise just a clarifying question or two.

And per you comment that I was last in this code, I certainly did not see anything that disrupted my recent Upgradeable change.

gabemontero · 2021-06-02T18:10:09Z

pkg/operator/starter.go

@@ -39,15 +53,22 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller
 		return err
 	}

-	kubeInformers := v1helpers.NewKubeInformersForNamespaces(kubeClient, util.TargetNamespace, util.OperatorNamespace, util.UserSpecifiedGlobalConfigNamespace)
+	kubeInformers := v1helpers.NewKubeInformersForNamespaces(kubeClient,
+		"",


let's put a comment in code to indicate this means cluster scoped, as you clarified in the PR comment 09eb355#r644180187

Or go with #216 (comment) instead

I'll add a comment to say "empty means cluster-scoped"

sounds good thanks

gabemontero · 2021-06-02T18:11:26Z

pkg/operator/operator.go

@@ -51,7 +50,7 @@ func NewOpenShiftControllerManagerOperator(
 	targetImagePullSpec string,
 	operatorConfigInformer operatorinformersv1.OpenShiftControllerManagerInformer,
 	proxyInformer configinformerv1.ProxyInformer,
-	kubeInformersForOpenshiftControllerManager informers.SharedInformerFactory,
+	kubeInformers v1helpers.KubeInformersForNamespaces,


wait I may have found it later in the review ... 09eb355#diff-0d623dfd885adb20f991bda4c2453aebd732ca6dbb4d1d4be6e79805c3b48de6R129

gabemontero · 2021-06-02T18:17:06Z

pkg/operator/starter.go

+	if highRateLimitProtoKubeConfig.Burst < 100 {
+		highRateLimitProtoKubeConfig.Burst = 100
+	}
+	kubeClient, err := kubernetes.NewForConfig(highRateLimitProtoKubeConfig)


So you are specifically referring to https://github.com/openshift/library-go/blob/c537a5b2af0196a9a235ccecc0d05c2583899f3d/pkg/operator/resource/resourceapply/generic.go#L150 as an example @adambkaplan, correct ?

gabemontero · 2021-06-02T18:19:03Z

pkg/operator/sync_openshiftcontrollermanager_v311_00.go

-			forceRollout = true
-		}
-	}
+	// TODO - use labels/annotations to force a daemonset rollout


IIRC this is a different TODO comment from what you had in your last PR in the same spot, and this is a TODO comment you want to keep, correct @adambkaplan ?

Yes - we are passing forceRollout to a method that has been deprecated. We should use labels or annotations to drive rollouts.

thanks for the confirmation

gabemontero · 2021-06-02T18:21:08Z

pkg/util/consts.go

@@ -6,6 +6,7 @@ const (
 	MachineSpecifiedGlobalConfigNamespace = "openshift-config-managed"
 	TargetNamespace                       = "openshift-controller-manager"
 	OperatorNamespace                     = "openshift-controller-manager-operator"
+	InfraNamespace                        = "openshift-infra"


As an alternative to my comment at 09eb355#diff-0d623dfd885adb20f991bda4c2453aebd732ca6dbb4d1d4be6e79805c3b48de6R57 we could add a constant here named something like ClusterScoped that is set to the emtpy string ""

gabemontero · 2021-06-02T18:22:44Z

pkg/operator/starter.go

+	if highRateLimitProtoKubeConfig.Burst < 100 {
+		highRateLimitProtoKubeConfig.Burst = 100
+	}
+	kubeClient, err := kubernetes.NewForConfig(highRateLimitProtoKubeConfig)


If so, a commit specific link to the file in question might help us to quickly remember where to go if we ever get to this TODO

- Refactor ocm-o to use the static resources controller to sync static YAML assets. - Add infomers for cluster-scoped objects, openshift-infra, and kube-system as a namespace to use an informer to fetch core k8s objects. - Use cached ConfigMapsGetter in the main workload sync controller - Don't set unmanaged/removed messages. The status controller is now responsible for setting the cluster operator states for managed/ unmanaged. The existing code now collides with the new static resource sync controller. - Increase rate limit for k8s API calls. This ensures the static resource sync controller is not throttled when reconciling RBAC objects.

adambkaplan

Added additional doc comments to clarify what I added (and why)

adambkaplan · 2021-06-02T23:11:54Z

pkg/operator/starter.go

+	if highRateLimitProtoKubeConfig.Burst < 100 {
+		highRateLimitProtoKubeConfig.Burst = 100
+	}
+	kubeClient, err := kubernetes.NewForConfig(highRateLimitProtoKubeConfig)


Updated the comments - hopefully things are a little bit clearer.

adambkaplan · 2021-06-02T23:12:03Z

pkg/operator/starter.go

@@ -39,15 +53,22 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller
 		return err
 	}

-	kubeInformers := v1helpers.NewKubeInformersForNamespaces(kubeClient, util.TargetNamespace, util.OperatorNamespace, util.UserSpecifiedGlobalConfigNamespace)
+	kubeInformers := v1helpers.NewKubeInformersForNamespaces(kubeClient,
+		"",


adambkaplan · 2021-06-02T23:12:31Z

pkg/operator/starter.go

@@ -105,10 +126,40 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller
 		controllerConfig.EventRecorder,
 	)

+	staticResourceController := staticresourcecontroller.NewStaticResourceController(


added the TODO here to make it clear where in library-go we need to add a future enhancement.

gabemontero · 2021-06-03T00:24:14Z

/lgtm

openshift-ci · 2021-06-03T00:24:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, gabemontero

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

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

openshift-bot · 2021-06-03T00:41:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-03T00:54:43Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-03T02:06:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-06-03T04:38:05Z

@adambkaplan: All pull requests linked via external trackers have merged:

openshift/cluster-openshift-controller-manager-operator#216

Bugzilla bug 1965283 has been moved to the MODIFIED state.

In response to this:

Bug 1965283: Static Resources Controller for Sync

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-bot · 2023-11-26T12:35:40Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-openshift-controller-manager-operator-container-v4.8.0-202311261141.p0.g286c157.assembly.stream for distgit ose-cluster-openshift-controller-manager-operator.
All builds following this will include this PR.

adambkaplan mentioned this pull request Jun 1, 2021

Bug 1965283: Sync Static Resources with Cache #215

Closed

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 1, 2021

openshift-ci bot requested review from deads2k and soltysh June 1, 2021 18:56

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2021

bump(*):

b2bc7a6

- Update openshift/library-go to latest on master (b9c317a319e0) - go mod vendor, add static resource sync controller

adambkaplan force-pushed the ocmo-fix-throttle-2 branch from 4dac35c to 09eb355 Compare June 2, 2021 17:22

adambkaplan changed the title ~~WIP - fix Upgradeable = "Unknown" condition~~ Bug 1965283: Static Resources Controller for Sync Jun 2, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 2, 2021

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jun 2, 2021

openshift-ci bot requested a review from wewang58 June 2, 2021 17:23

adambkaplan commented Jun 2, 2021

View reviewed changes

openshift-ci bot assigned gabemontero Jun 2, 2021

gabemontero reviewed Jun 2, 2021

View reviewed changes

adambkaplan force-pushed the ocmo-fix-throttle-2 branch from 09eb355 to c6bcf70 Compare June 2, 2021 23:07

adambkaplan force-pushed the ocmo-fix-throttle-2 branch from c6bcf70 to 0698c47 Compare June 2, 2021 23:11

adambkaplan commented Jun 2, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2021

openshift-merge-robot merged commit 286c157 into openshift:master Jun 3, 2021

Bug 1965283: Static Resources Controller for Sync #216

Bug 1965283: Static Resources Controller for Sync #216

Conversation

adambkaplan commented Jun 1, 2021 • edited

openshift-ci bot commented Jun 2, 2021

adambkaplan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero 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

adambkaplan 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

gabemontero commented Jun 3, 2021

openshift-ci bot commented Jun 3, 2021

openshift-bot commented Jun 3, 2021

openshift-bot commented Jun 3, 2021

openshift-bot commented Jun 3, 2021

openshift-ci bot commented Jun 3, 2021

openshift-bot commented Nov 26, 2023

adambkaplan commented Jun 1, 2021 •

edited