OCPBUGS-55217: CombineCABundleConfigMaps: use optimistic create/update #1936

vrutkovs · 2025-02-19T07:37:29Z

Instead of re-creating configmap from scratch every time this function should attempt to use existing configmap and replace the contents only. This would prevent extra configmap updates when metadata changes.

Tested in openshift/cluster-kube-apiserver-operator#1812

openshift-ci-robot · 2025-02-25T09:47:03Z

@vrutkovs: This pull request references API-1802 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target either version "4.19." or "openshift-4.19.", but it targets "4.18.0" instead.

In response to this:

Instead of re-creating configmap from scratch every time this function should attempt to use existing configmap and replace the contents only. This would prevent extra configmap updates when metadata changes.

Tested in openshift/cluster-kube-apiserver-operator#1812

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.

openshift-ci-robot · 2025-04-22T12:42:19Z

@vrutkovs: This pull request references Jira Issue OCPBUGS-55217, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.19.0) matches configured target version for branch (4.19.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @wangke19

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

In response to this:

Instead of re-creating configmap from scratch every time this function should attempt to use existing configmap and replace the contents only. This would prevent extra configmap updates when metadata changes.

Tested in openshift/cluster-kube-apiserver-operator#1812

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.

benluddy · 2025-04-22T12:59:23Z

/cc

benluddy · 2025-04-23T14:45:38Z

pkg/operator/resourcesynccontroller/core.go

-			"ca-bundle.crt": string(caBytes),
-		},
+	modified := additionalAnnotations.EnsureTLSMetadataUpdate(&cm.ObjectMeta)
+	cm.Data = map[string]string{
+		"ca-bundle.crt": string(caBytes),
 	}
-	return cm, nil
+	return cm, modified, nil


How is the bool return value supposed to be interpreted? It's unclear to me what it means when EnsureTLSMetadataUpdate returns false (and so the returned bool value is false) but Data has been mutated.

modified here returns if update is necessary:

requiredConfigMap, updateRequired, err := resourcesynccontroller.CombineCABundleConfigMaps(

It means either metadata or contents has changed. I think I missed "content has changed, but not the metadata", so I'll add some unittests to verify that

benluddy · 2025-04-23T14:46:55Z

pkg/operator/resourcesynccontroller/resourcesync_controller.go

+	applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"
+


Could this make backporting more painful?

This may complicate it, yes, but I'd prefer to avoid packports to 4.18 and earlier as we don't have stable rotation tests for them anyway

benluddy · 2025-04-23T14:49:31Z

pkg/operator/resourcesynccontroller/core.go

@@ -14,15 +14,22 @@ import (
 	"github.com/openshift/library-go/pkg/operator/certrotation"
 )

-func CombineCABundleConfigMaps(destinationConfigMap ResourceLocation, lister corev1listers.ConfigMapLister, additionalAnnotations certrotation.AdditionalAnnotations, inputConfigMaps ...ResourceLocation) (*corev1.ConfigMap, error) {
+func CombineCABundleConfigMaps(destinationConfigMap *corev1.ConfigMap, lister corev1listers.ConfigMapLister, additionalAnnotations certrotation.AdditionalAnnotations, inputConfigMaps ...ResourceLocation) (*corev1.ConfigMap, bool, error) {


This must not have any unit test coverage... could you add some to cover the behavior change being made please?

openshift-ci · 2025-04-24T08:24:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vrutkovs
Once this PR has been reviewed and has the lgtm label, please assign p0lyn0mial 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

benluddy · 2025-05-05T16:34:06Z

pkg/operator/resourcesynccontroller/core.go

@@ -14,15 +14,21 @@ import (
 	"github.com/openshift/library-go/pkg/operator/certrotation"
 )

-func CombineCABundleConfigMaps(destinationConfigMap ResourceLocation, lister corev1listers.ConfigMapLister, additionalAnnotations certrotation.AdditionalAnnotations, inputConfigMaps ...ResourceLocation) (*corev1.ConfigMap, error) {
+func CombineCABundleConfigMaps(destinationConfigMap *corev1.ConfigMap, lister corev1listers.ConfigMapLister, additionalAnnotations certrotation.AdditionalAnnotations, inputConfigMaps ...ResourceLocation) (*corev1.ConfigMap, bool, error) {


Is there a reasonable way to introduce this as a function with a different name, and deprecate the existing one, to avoid lockstep updates to hypershift, kas-o, kcm-o, and scheduler-o?

Yeah, I think it would be a good idea

benluddy · 2025-05-05T16:41:03Z

pkg/operator/resourcesynccontroller/core.go

+	if !reflect.DeepEqual(cm.Data, newCMData) {
+		modified = true
 	}
-	return cm, nil
+
+	cm.Data = newCMData


Nit: I expected to see the assignment to cm.Data inside the "then" block with the assignment to modified. It looks correct to me either way.

benluddy · 2025-05-05T16:42:21Z

pkg/operator/resourcesynccontroller/core_test.go

+}
+
+func (m *mockConfigMapNamespaceLister) List(selector labels.Selector) ([]*corev1.ConfigMap, error) {
+	return nil, nil


Nit: Panic in unimplemented stubs to prevent someone later using them by mistake.

benluddy · 2025-05-05T16:47:39Z

pkg/operator/resourcesynccontroller/core_test.go

+			if err == nil && result == nil {
+				t.Errorf("Expected result to not be nil when no error occurred")
+			}
+


Any reason not to compare the full returned CM against an expected CM?

Right, it would verify removal of expired certs too

It should be <namespace>-<name>, not the other way around

Instead of re-creating configmap from scratch every time this function should attempt to use existing configmap and replace the contents only. This would prevent extra configmap updates when metadata changes

openshift-ci · 2025-05-13T14:36:13Z

@vrutkovs: all tests passed!

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.

openshift-ci bot requested review from deads2k and hexfusion February 19, 2025 07:37

vrutkovs force-pushed the combine-cas-optimistic-update branch from 5fd5ebc to 7eaf2a0 Compare February 19, 2025 07:49

vrutkovs closed this Feb 24, 2025

vrutkovs reopened this Feb 25, 2025

vrutkovs force-pushed the combine-cas-optimistic-update branch from 7eaf2a0 to 7af439c Compare February 25, 2025 09:46

vrutkovs changed the title ~~CombineCABundleConfigMaps: use optimistic create/update~~ API-1802: CombineCABundleConfigMaps: use optimistic create/update Feb 25, 2025

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 25, 2025

vrutkovs changed the title ~~API-1802: CombineCABundleConfigMaps: use optimistic create/update~~ WIP API-1802: CombineCABundleConfigMaps: use optimistic create/update Feb 27, 2025

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 Feb 27, 2025

vrutkovs force-pushed the combine-cas-optimistic-update branch from 7af439c to d5dd901 Compare April 22, 2025 08:19

vrutkovs changed the title ~~WIP API-1802: CombineCABundleConfigMaps: use optimistic create/update~~ API-1802: CombineCABundleConfigMaps: use optimistic create/update Apr 22, 2025

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 Apr 22, 2025

vrutkovs changed the title ~~API-1802: CombineCABundleConfigMaps: use optimistic create/update~~ OCPBUGS-55217: CombineCABundleConfigMaps: use optimistic create/update Apr 22, 2025

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Apr 22, 2025

openshift-ci bot requested a review from wangke19 April 22, 2025 12:42

openshift-ci bot requested a review from benluddy April 22, 2025 12:59

vrutkovs mentioned this pull request Apr 22, 2025

OCPBUGS-55217: Optimistically update Kube Server and Client CA bundles openshift/cluster-kube-apiserver-operator#1812

Draft

benluddy reviewed Apr 23, 2025

View reviewed changes

vrutkovs force-pushed the combine-cas-optimistic-update branch from d5dd901 to f4c068a Compare April 24, 2025 08:23

benluddy reviewed May 5, 2025

View reviewed changes

vrutkovs force-pushed the combine-cas-optimistic-update branch from f4c068a to f1ee606 Compare May 6, 2025 08:53

vrutkovs added 2 commits May 13, 2025 16:07

Fix order of elements in "Updating annotation" log message

e6d5f65

It should be <namespace>-<name>, not the other way around

crypto: error message should use duration instead of years

42666df

vrutkovs added 2 commits May 13, 2025 16:07

CombineCABundleConfigMaps: use optimistic create/update

b50e914

Instead of re-creating configmap from scratch every time this function should attempt to use existing configmap and replace the contents only. This would prevent extra configmap updates when metadata changes

Add unit test for TestCombineCABundleConfigMaps

855d77f

vrutkovs force-pushed the combine-cas-optimistic-update branch from f1ee606 to 855d77f Compare May 13, 2025 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-55217: CombineCABundleConfigMaps: use optimistic create/update #1936

OCPBUGS-55217: CombineCABundleConfigMaps: use optimistic create/update #1936

vrutkovs commented Feb 19, 2025 •

edited

Loading

openshift-ci-robot commented Feb 25, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Apr 22, 2025

benluddy commented Apr 22, 2025

benluddy Apr 23, 2025

vrutkovs Apr 23, 2025

benluddy Apr 23, 2025

vrutkovs Apr 24, 2025

benluddy Apr 23, 2025

openshift-ci bot commented Apr 24, 2025

benluddy May 5, 2025

vrutkovs May 5, 2025

benluddy May 5, 2025

benluddy May 5, 2025

benluddy May 5, 2025

vrutkovs May 6, 2025

openshift-ci bot commented May 13, 2025

		applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"

OCPBUGS-55217: CombineCABundleConfigMaps: use optimistic create/update #1936

Are you sure you want to change the base?

OCPBUGS-55217: CombineCABundleConfigMaps: use optimistic create/update #1936

Conversation

vrutkovs commented Feb 19, 2025 • edited Loading

openshift-ci-robot commented Feb 25, 2025 • edited by openshift-ci bot Loading

openshift-ci-robot commented Apr 22, 2025

benluddy commented Apr 22, 2025

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

openshift-ci bot commented Apr 24, 2025

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

openshift-ci bot commented May 13, 2025

vrutkovs commented Feb 19, 2025 •

edited

Loading

openshift-ci-robot commented Feb 25, 2025 •

edited by openshift-ci bot

Loading