Bug 1965283: Static Resources Controller for Sync #216
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import ( | |
|
|
||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/client-go/kubernetes" | ||
| "k8s.io/client-go/rest" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| configclient "github.com/openshift/client-go/config/clientset/versioned" | ||
|
|
@@ -16,20 +17,33 @@ import ( | |
| operatorclientv1 "github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1" | ||
| operatorinformers "github.com/openshift/client-go/operator/informers/externalversions" | ||
| "github.com/openshift/library-go/pkg/controller/controllercmd" | ||
| "github.com/openshift/library-go/pkg/operator/resource/resourceapply" | ||
| "github.com/openshift/library-go/pkg/operator/resourcesynccontroller" | ||
| "github.com/openshift/library-go/pkg/operator/staticresourcecontroller" | ||
| "github.com/openshift/library-go/pkg/operator/status" | ||
| "github.com/openshift/library-go/pkg/operator/v1helpers" | ||
|
|
||
| configobservationcontroller "github.com/openshift/cluster-openshift-controller-manager-operator/pkg/operator/configobservation/configobservercontroller" | ||
| "github.com/openshift/cluster-openshift-controller-manager-operator/pkg/operator/usercaobservation" | ||
| "github.com/openshift/cluster-openshift-controller-manager-operator/pkg/operator/v311_00_assets" | ||
| "github.com/openshift/cluster-openshift-controller-manager-operator/pkg/util" | ||
| ) | ||
|
|
||
| func RunOperator(ctx context.Context, controllerConfig *controllercmd.ControllerContext) error { | ||
| // Increase QPS and burst to avoid client-side rate limits when reconciling RBAC API objects. | ||
| // See TODO below for the StaticResourceController | ||
| highRateLimitProtoKubeConfig := rest.CopyConfig(controllerConfig.ProtoKubeConfig) | ||
| if highRateLimitProtoKubeConfig.QPS < 50 { | ||
| highRateLimitProtoKubeConfig.QPS = 50 | ||
| } | ||
| if highRateLimitProtoKubeConfig.Burst < 100 { | ||
| highRateLimitProtoKubeConfig.Burst = 100 | ||
| } | ||
| kubeClient, err := kubernetes.NewForConfig(highRateLimitProtoKubeConfig) | ||
|
There was a problem hiding this comment. The static resource sync controller unfortunately doesn't use listers when reconciling RBAC objects. There was a problem hiding this comment. 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 ? There was a problem hiding this comment. 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 There was a problem hiding this comment. I think if I update the comment to use There was a problem hiding this comment. sorry I'm not following how updating the comment to use There was a problem hiding this comment. Updated the comments - hopefully things are a little bit clearer. |
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| operatorClient, err := operatorclient.NewForConfig(controllerConfig.KubeConfig) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -39,15 +53,26 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller | |
| return err | ||
| } | ||
|
|
||
| // Create kube informers for namespaces that the operator reconciles content from or to. | ||
| // The empty string "" adds informers for cluster-scoped resources. | ||
| kubeInformers := v1helpers.NewKubeInformersForNamespaces(kubeClient, | ||
| "", | ||
|
There was a problem hiding this comment. let's put a comment in code to indicate this means cluster scoped, as you clarified in the PR comment 09eb355#r644180187 There was a problem hiding this comment. Or go with #216 (comment) instead There was a problem hiding this comment. I'll add a comment to say "empty means cluster-scoped" |
||
| util.TargetNamespace, | ||
| util.OperatorNamespace, | ||
| util.UserSpecifiedGlobalConfigNamespace, | ||
| util.InfraNamespace, | ||
| metav1.NamespaceSystem, | ||
|
Comment on lines
+59
to
+64
There was a problem hiding this comment. New additions to this list:
|
||
| ) | ||
| operatorConfigInformers := operatorinformers.NewSharedInformerFactory(operatorClient, 10*time.Minute) | ||
| configInformers := configinformers.NewSharedInformerFactory(configClient, 10*time.Minute) | ||
|
|
||
| // OpenShiftControlllerManagerOperator reconciles the state of the openshift-controller-manager | ||
| // DaemonSet and associated ConfigMaps. | ||
| operator := NewOpenShiftControllerManagerOperator( | ||
| os.Getenv("IMAGE"), | ||
| operatorConfigInformers.Operator().V1().OpenShiftControllerManagers(), | ||
| configInformers.Config().V1().Proxies(), | ||
| kubeInformers, | ||
| operatorClient.OperatorV1(), | ||
| kubeClient, | ||
| controllerConfig.EventRecorder, | ||
|
|
@@ -69,6 +94,8 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller | |
| controllerConfig.EventRecorder, | ||
| ) | ||
|
|
||
| // ConfigObserver observes the configuration state from cluster config objects and transforms | ||
| // them into configuration used by openshift-controller-manager | ||
| configObserver := configobservationcontroller.NewConfigObserver( | ||
| opClient, | ||
| operatorConfigInformers, | ||
|
|
@@ -89,6 +116,9 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller | |
| openshiftControllerManagers: operatorClient.OperatorV1().OpenShiftControllerManagers(), | ||
| version: os.Getenv("RELEASE_VERSION"), | ||
| } | ||
|
|
||
| // ClusterOperatorStatusController aggregates the conditions in our openshiftcontrollermanager | ||
| // object to the corresponding ClusterOperator object. | ||
| clusterOperatorStatus := status.NewClusterOperatorStatusController( | ||
| util.ClusterOperatorName, | ||
| []configv1.ObjectReference{ | ||
|
|
@@ -105,10 +135,43 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller | |
| controllerConfig.EventRecorder, | ||
| ) | ||
|
|
||
| // StaticResourceController uses library-go's resourceapply package to reconcile a set of YAML | ||
| // manifests against a cluster. | ||
| // TODO: enhance resourceapply to use listers for RBAC APIs. | ||
| staticResourceController := staticresourcecontroller.NewStaticResourceController( | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. added the TODO here to make it clear where in library-go we need to add a future enhancement. |
||
| "OpenshiftControllerManagerStaticResources", | ||
| v311_00_assets.Asset, | ||
| []string{ | ||
| "v3.11.0/openshift-controller-manager/informer-clusterrole.yaml", | ||
| "v3.11.0/openshift-controller-manager/informer-clusterrolebinding.yaml", | ||
| "v3.11.0/openshift-controller-manager/ingress-to-route-controller-clusterrole.yaml", | ||
| "v3.11.0/openshift-controller-manager/ingress-to-route-controller-clusterrolebinding.yaml", | ||
| "v3.11.0/openshift-controller-manager/tokenreview-clusterrole.yaml", | ||
| "v3.11.0/openshift-controller-manager/tokenreview-clusterrolebinding.yaml", | ||
| "v3.11.0/openshift-controller-manager/leader-role.yaml", | ||
| "v3.11.0/openshift-controller-manager/leader-rolebinding.yaml", | ||
| "v3.11.0/openshift-controller-manager/ns.yaml", | ||
| "v3.11.0/openshift-controller-manager/old-leader-role.yaml", | ||
| "v3.11.0/openshift-controller-manager/old-leader-rolebinding.yaml", | ||
| "v3.11.0/openshift-controller-manager/separate-sa-role.yaml", | ||
| "v3.11.0/openshift-controller-manager/separate-sa-rolebinding.yaml", | ||
| "v3.11.0/openshift-controller-manager/sa.yaml", | ||
| "v3.11.0/openshift-controller-manager/svc.yaml", | ||
| "v3.11.0/openshift-controller-manager/servicemonitor-role.yaml", | ||
| "v3.11.0/openshift-controller-manager/servicemonitor-rolebinding.yaml", | ||
| "v3.11.0/openshift-controller-manager/buildconfigstatus-clusterrole.yaml", | ||
| "v3.11.0/openshift-controller-manager/buildconfigstatus-clusterrolebinding.yaml", | ||
| }, | ||
| resourceapply.NewKubeClientHolder(kubeClient), | ||
| opClient, | ||
| controllerConfig.EventRecorder, | ||
| ).AddKubeInformers(kubeInformers) | ||
|
|
||
| operatorConfigInformers.Start(ctx.Done()) | ||
| kubeInformers.Start(ctx.Done()) | ||
| configInformers.Start(ctx.Done()) | ||
|
|
||
| go staticResourceController.Run(ctx, 1) | ||
| go operator.Run(ctx, 1) | ||
| go resourceSyncer.Run(ctx, 1) | ||
| go configObserver.Run(ctx, 1) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,6 @@ import ( | |
| "k8s.io/apimachinery/pkg/api/equality" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/client-go/kubernetes" | ||
| appsclientv1 "k8s.io/client-go/kubernetes/typed/apps/v1" | ||
| coreclientv1 "k8s.io/client-go/kubernetes/typed/core/v1" | ||
|
|
@@ -38,43 +37,11 @@ func syncOpenShiftControllerManager_v311_00_to_latest(c OpenShiftControllerManag | |
| errors := []error{} | ||
| var err error | ||
| operatorConfig := originalOperatorConfig.DeepCopy() | ||
|
|
||
| // TODO - use labels/annotations to force a daemonset rollout | ||
|
There was a problem hiding this comment. 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 ? There was a problem hiding this comment. Yes - we are passing There was a problem hiding this comment. thanks for the confirmation |
||
| forceRollout := false | ||
|
|
||
| _, configMapModified, err := manageOpenShiftControllerManagerConfigMap_v311_00_to_latest(c.kubeClient, c.configMapsGetter, c.recorder, operatorConfig) | ||
| if err != nil { | ||
| errors = append(errors, fmt.Errorf("%q: %v", "configmap", err)) | ||
| } | ||
|
|
@@ -84,12 +51,12 @@ func syncOpenShiftControllerManager_v311_00_to_latest(c OpenShiftControllerManag | |
| errors = append(errors, fmt.Errorf("%q: %v", "client-ca", err)) | ||
| } | ||
|
|
||
| _, serviceCAModified, err := manageOpenShiftServiceCAConfigMap_v311_00_to_latest(c.kubeClient, c.configMapsGetter, c.recorder) | ||
| if err != nil { | ||
| errors = append(errors, fmt.Errorf("%q: %v", "openshift-service-ca", err)) | ||
| } | ||
|
|
||
| _, globalCAModified, err := manageOpenShiftGlobalCAConfigMap_v311_00_to_latest(c.kubeClient, c.configMapsGetter, c.recorder) | ||
| if err != nil { | ||
| errors = append(errors, fmt.Errorf("%q: %v", "openshift-global-ca", err)) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ const ( | |
| MachineSpecifiedGlobalConfigNamespace = "openshift-config-managed" | ||
| TargetNamespace = "openshift-controller-manager" | ||
| OperatorNamespace = "openshift-controller-manager-operator" | ||
| InfraNamespace = "openshift-infra" | ||
|
There was a problem hiding this comment. As an alternative to my comment at 09eb355#diff-0d623dfd885adb20f991bda4c2453aebd732ca6dbb4d1d4be6e79805c3b48de6R57 we could add a constant here named something like |
||
| VersionAnnotation = "release.openshift.io/version" | ||
| ClusterOperatorName = "openshift-controller-manager" | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use this struct with the static resource sync controller and the resource sync controller, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait I may have found it later in the review ... 09eb355#diff-0d623dfd885adb20f991bda4c2453aebd732ca6dbb4d1d4be6e79805c3b48de6R129
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, that is it