WRKLDS-728: Capabilities: drop build/apps APIService when capabilities are not enabled #532
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,15 +20,18 @@ import ( | |
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| utilruntime "k8s.io/apimachinery/pkg/util/runtime" | ||
| "k8s.io/apimachinery/pkg/util/sets" | ||
| "k8s.io/client-go/kubernetes" | ||
| appsclientv1 "k8s.io/client-go/kubernetes/typed/apps/v1" | ||
| coreclientv1 "k8s.io/client-go/kubernetes/typed/core/v1" | ||
| "k8s.io/klog/v2" | ||
|
|
||
| openshiftapi "github.com/openshift/api" | ||
| configv1 "github.com/openshift/api/config/v1" | ||
| openshiftcontrolplanev1 "github.com/openshift/api/openshiftcontrolplane/v1" | ||
| operatorv1 "github.com/openshift/api/operator/v1" | ||
| openshiftconfigclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" | ||
| configlisterv1 "github.com/openshift/client-go/config/listers/config/v1" | ||
| operatorv1client "github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1" | ||
| "github.com/openshift/cluster-openshift-apiserver-operator/pkg/operator/operatorclient" | ||
| "github.com/openshift/cluster-openshift-apiserver-operator/pkg/operator/v311_00_assets" | ||
|
|
@@ -58,6 +61,7 @@ type OpenShiftAPIServerWorkload struct { | |
| operatorClient v1helpers.OperatorClient | ||
| operatorConfigClient operatorv1client.OpenShiftAPIServersGetter | ||
| openshiftConfigClient openshiftconfigclientv1.ConfigV1Interface | ||
| clusterVersionLister configlisterv1.ClusterVersionLister | ||
| kubeClient kubernetes.Interface | ||
|
|
||
| // countNodes a function to return count of nodes on which the workload will be installed | ||
|
|
@@ -79,6 +83,7 @@ func NewOpenShiftAPIServerWorkload( | |
| operatorClient v1helpers.OperatorClient, | ||
| operatorConfigClient operatorv1client.OpenShiftAPIServersGetter, | ||
| openshiftConfigClient openshiftconfigclientv1.ConfigV1Interface, | ||
| clusterVersionLister configlisterv1.ClusterVersionLister, | ||
| countNodes nodeCountFunc, | ||
| ensureAtMostOnePodPerNode ensureAtMostOnePodPerNodeFunc, | ||
| targetNamespace string, | ||
|
|
@@ -91,6 +96,7 @@ func NewOpenShiftAPIServerWorkload( | |
| operatorClient: operatorClient, | ||
| operatorConfigClient: operatorConfigClient, | ||
| openshiftConfigClient: openshiftConfigClient, | ||
| clusterVersionLister: clusterVersionLister, | ||
| countNodes: countNodes, | ||
| ensureAtMostOnePodPerNode: ensureAtMostOnePodPerNode, | ||
| targetNamespace: targetNamespace, | ||
|
|
@@ -147,7 +153,7 @@ func (c *OpenShiftAPIServerWorkload) Sync(ctx context.Context, syncContext facto | |
| } | ||
| operatorConfig := originalOperatorConfig.DeepCopy() | ||
|
|
||
| _, _, err = manageOpenShiftAPIServerConfigMap_v311_00_to_latest(ctx, c.kubeClient.CoreV1(), c.clusterVersionLister, syncContext.Recorder(), operatorConfig) | ||
| if err != nil { | ||
| errors = append(errors, fmt.Errorf("%q: %v", "configmap", err)) | ||
| } | ||
|
|
@@ -273,15 +279,49 @@ func manageOpenShiftAPIServerImageImportCA_v311_00_to_latest(ctx context.Context | |
| return resourceapply.ApplyConfigMap(ctx, client, recorder, requiredConfigMap) | ||
| } | ||
|
|
||
| func manageOpenShiftAPIServerConfigMap_v311_00_to_latest(ctx context.Context, client coreclientv1.ConfigMapsGetter, clusterVersionLister configlisterv1.ClusterVersionLister, recorder events.Recorder, operatorConfig *operatorv1.OpenShiftAPIServer) (*corev1.ConfigMap, bool, error) { | ||
| configMap := resourceread.ReadConfigMapV1OrDie(v311_00_assets.MustAsset("v3.11.0/openshift-apiserver/cm.yaml")) | ||
| defaultConfig := v311_00_assets.MustAsset("v3.11.0/config/defaultconfig.yaml") | ||
|
|
||
| clusterVersion, err := clusterVersionLister.Get("version") | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
|
|
||
| knownCaps := sets.New[configv1.ClusterVersionCapability](clusterVersion.Status.Capabilities.KnownCapabilities...) | ||
| capsEnabled := sets.New[configv1.ClusterVersionCapability](clusterVersion.Status.Capabilities.EnabledCapabilities...) | ||
|
|
||
| apiServers := openshiftcontrolplanev1.APIServers{ | ||
| PerGroupOptions: []openshiftcontrolplanev1.PerGroupOptions{}, | ||
| } | ||
|
|
||
| if knownCaps.Has(configv1.ClusterVersionCapabilityBuild) && !capsEnabled.Has(configv1.ClusterVersionCapabilityBuild) { | ||
|
Contributor
There was a problem hiding this comment. Will you have a test that will test the disabling of this APIs?
Member
Author
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment. I think that we should have an e2e test for it, wdyt?
Member
Author
There was a problem hiding this comment. Updating/adding e2es is the next step once this PR and openshift/cluster-openshift-controller-manager-operator#291 are merged.
Contributor
There was a problem hiding this comment. where are you planing to add that test ? will it be this repo or in the origin ?
Member
Author
There was a problem hiding this comment. I am planning origin as we need to check for disabled controllers as well. E.g. read the config or parse the controllers logs. On the other hand we first need to update the origin tests to react on missing API so other tests (e.g. running in parallel) for Builds/DCs do not fail. Ultimately, each operator repo might have its own version of the e2e. The e2e (going here or into origin) will need a separate PR.
Contributor
There was a problem hiding this comment. Ideally if we could add a test here and then also run it from the origin repo. Are you planing to add the test before we ship 4.14 ?
Member
Author
There was a problem hiding this comment. Yes. All tests need to land before 4.14 ships.
Contributor
There was a problem hiding this comment. OK. I would like to avoid shipping a feature without the tests. Thanks. |
||
| klog.V(4).Infof("Capability %q not enabled, disabling 'openshift.io/build' controller", configv1.ClusterVersionCapabilityBuild) | ||
| apiServers.PerGroupOptions = append(apiServers.PerGroupOptions, openshiftcontrolplanev1.PerGroupOptions{Name: openshiftcontrolplanev1.OpenShiftBuildAPIserver, DisabledVersions: []string{"v1"}}) | ||
| } | ||
|
|
||
| if knownCaps.Has(configv1.ClusterVersionCapabilityDeploymentConfig) && !capsEnabled.Has(configv1.ClusterVersionCapabilityDeploymentConfig) { | ||
| klog.V(4).Infof("Capability %q not enabled, disabling 'openshift.io/apps' controller", configv1.ClusterVersionCapabilityDeploymentConfig) | ||
|
Contributor
There was a problem hiding this comment. mhm, I still think it will be logged on every sync. What is the value of spamming the log file with this information ? Ideally if we could log it just once, right ?
Contributor
There was a problem hiding this comment. Maybe we need a new condition ? Does the service controller set a condition ?
Member
Author
There was a problem hiding this comment. With log 4 it is good to know the operator properly reads the capabilities. Otherwise, I'd need to get the CM, decode it, read the list of disabled apiservers and compare it. Too much additional code for just a single log line.
Member
Author
There was a problem hiding this comment. The API service controller sets the new Degraded condition for API service endpoints until the disabled APIService objects are deleted. The test is reported through the already existing conditions for operands. |
||
| apiServers.PerGroupOptions = append(apiServers.PerGroupOptions, openshiftcontrolplanev1.PerGroupOptions{Name: openshiftcontrolplanev1.OpenShiftAppsAPIserver, DisabledVersions: []string{"v1"}}) | ||
| } | ||
|
|
||
| bytes, err := json.Marshal(apiServers) | ||
| if err != nil { | ||
| return nil, false, fmt.Errorf("unable to marshal APIServers struct: %v", err) | ||
| } | ||
|
|
||
| configYaml, err := yaml.JSONToYAML([]byte(fmt.Sprintf("{\"apiVersion\": \"openshiftcontrolplane.config.openshift.io/v1\", \"kind\": \"OpenShiftAPIServerConfig\", \"apiServers\": %v}\n", string(bytes)))) | ||
| if err != nil { | ||
| return nil, false, fmt.Errorf("unable to marshal OpenShiftAPIServerConfig struct: %v", err) | ||
| } | ||
|
|
||
| requiredConfigMap, _, err := resourcemerge.MergePrunedConfigMap( | ||
| &openshiftcontrolplanev1.OpenShiftAPIServerConfig{}, | ||
| configMap, | ||
| "config.yaml", | ||
| nil, | ||
| defaultConfig, | ||
| configYaml, | ||
| operatorConfig.Spec.ObservedConfig.Raw, | ||
| operatorConfig.Spec.UnsupportedConfigOverrides.Raw, | ||
| ) | ||
|
|
||
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.
why this doesn't have to operate only on the enabled services?
Uh oh!
There was an error while loading. Please reload this page.
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.
apiServicesReferences()is consumed by.WithClusterOperatorStatusControllerwhich uses the references to build a list of related objects. In case a reference has no relevant object, it gets ignored.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.
ok, it looks like this is purely informational. It doesn't have any impact on the core logic.