OCPBUGS-17157: label non-OLM resources #3017

stevekuznetsov · 2023-08-22T18:03:18Z

HACK: add a round-tripper to ensure we label non-OLM resources

We can either flag-gate this to dev only, or delete it before merge.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

*: label non-OLM resources

Today, our controllers use un-filtered LIST+WATCH calls to monitor the
state of the cluster. For OLM-specific resource types, that's fine,
since we need to know (for instance) about every CSV. For non-OLM
resource groups, though, that is needlessly wasteful in memory
consumption and makes our controller's footprint scale with the size of
the cluster itself, irrespective of the usage of OLM. Adding a label to
every resource we create is the first step in being able to filter down
all of those requests to only those objects with our label.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

openshift-ci-robot · 2023-08-22T18:03:22Z

@stevekuznetsov: This pull request references Jira Issue OCPBUGS-17157, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @jianzhangbjz

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

In response to this:

HACK: add a round-tripper to ensure we label non-OLM resources

We can either flag-gate this to dev only, or delete it before merge.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

*: label non-OLM resources

Today, our controllers use un-filtered LIST+WATCH calls to monitor the
state of the cluster. For OLM-specific resource types, that's fine,
since we need to know (for instance) about every CSV. For non-OLM
resource groups, though, that is needlessly wasteful in memory
consumption and makes our controller's footprint scale with the size of
the cluster itself, irrespective of the usage of OLM. Adding a label to
every resource we create is the first step in being able to filter down
all of those requests to only those objects with our label.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

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.

ncdc · 2023-08-22T20:01:36Z

Was the hack commit for your local testing, to confirm things were doing what you expected, and that you didn't miss adding labels anywhere in the code base?

cmd/olm/main.go

pkg/controller/bundle/bundle_unpacker.go

perdasilva

lgtm - just a couple of non-blocking questions. If, in your judgement, they are not worth addressing, I'm happy to merge

I didn't notice CI is failing - code lgtm, get CI to green and we're good =D

perdasilva · 2023-08-23T09:51:45Z

/approve cancel (see review dismissal for details)

stevekuznetsov · 2023-08-23T12:32:05Z

@ncdc yes, but I could see it being generally useful moving forward to not mistakenly re-introduce the bug. Once the informers are filtered, you'd end up knowing anyway since your controller would not do the right thing but the round-tripper failure would be much more direct.

stevekuznetsov · 2023-08-23T16:19:32Z

@ncdc @perdasilva this should be ready for review

stevekuznetsov · 2023-08-23T16:20:23Z

test/e2e/operator_groups_e2e_test.go

@@ -442,34 +442,38 @@ var _ = Describe("Operator Group", func() {
 			}(ns)
 		}

-		// Generate operatorGroupA - OwnNamespace


If you're going to have concepts in the code like "operatorGroupA" but the value in the cluster will be auto-generated, it's imperative to print that relationship for folks that debug your test.

stevekuznetsov · 2023-08-23T16:20:44Z

test/e2e/operator_groups_e2e_test.go

@@ -426,8 +426,8 @@ var _ = Describe("Operator Group", func() {
 		// Create crd so csv succeeds
 		// Ensure clusterroles created and aggregated for access provided APIs

-		// Generate namespaceA


If you're going to have phases of a test, use a logger to make them appear in the output to make debugging failures easier.

stevekuznetsov · 2023-08-23T16:21:10Z

test/e2e/util.go

@@ -346,7 +346,7 @@ func catalogSourceRegistryPodSynced(catalog *operatorsv1alpha1.CatalogSource) bo
 	if connState != nil {
 		state = connState.LastObservedState
 	}
-	fmt.Printf("waiting for catalog pod %v to be available (for sync) - %s\n", catalog.GetName(), state)
+	fmt.Printf("waiting for catalog pod %s/%s to be available (for sync) - %s\n", catalog.GetNamespace(), catalog.GetName(), state)


Always fully qualify the object you're waiting on so that folks debugging a failing test know where to look.

stevekuznetsov · 2023-08-23T16:21:37Z

test/e2e/csv_e2e_test.go

@@ -4432,17 +4432,18 @@ func fetchCSV(c versioned.Interface, name, namespace string, checker csvConditio
 	var fetchedCSV *operatorsv1alpha1.ClusterServiceVersion
 	var err error

+	ctx.Ctx().Logf("waiting for CSV %s/%s to reach condition", namespace, name)


If a function like fetchCSV is going to validate conditions, at least log something to make it less surprising.

ncdc · 2023-08-23T20:31:13Z

/lgtm

perdasilva

lgtm

This round-tripper is added to our *rest.Config when it's possible to detect that we're in a CI environment. Developers should set $CI=true to get this behavior locally. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

Today, our controllers use un-filtered LIST+WATCH calls to monitor the state of the cluster. For OLM-specific resource types, that's fine, since we need to know (for instance) about every CSV. For non-OLM resource groups, though, that is needlessly wasteful in memory consumption and makes our controller's footprint scale with the size of the cluster itself, irrespective of the usage of OLM. Adding a label to every resource we create is the first step in being able to filter down all of those requests to only those objects with our label. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

openshift-ci · 2023-08-24T12:52:58Z

New changes are detected. LGTM label has been removed.

awgreene

This looks good to me and I wish that we had done this earlier. @perdasilva and I were recently discussing how OLM "adopts/overwrites" existing k8s resources if there is a name collision. Had we considered using labels in the past we could have written OLM such that it wouldn't delete/update resources missing these labels. Now that OLM is out in the wild there isn't a good migration path.

openshift-ci-robot · 2023-08-24T13:43:54Z

@stevekuznetsov: Jira Issue OCPBUGS-17157: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

operator-framework/operator-marketplace#530 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-17157 has not been moved to the MODIFIED state.

In response to this:

HACK: add a round-tripper to ensure we label non-OLM resources

We can either flag-gate this to dev only, or delete it before merge.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

*: label non-OLM resources

Today, our controllers use un-filtered LIST+WATCH calls to monitor the
state of the cluster. For OLM-specific resource types, that's fine,
since we need to know (for instance) about every CSV. For non-OLM
resource groups, though, that is needlessly wasteful in memory
consumption and makes our controller's footprint scale with the size of
the cluster itself, irrespective of the usage of OLM. Adding a label to
every resource we create is the first step in being able to filter down
all of those requests to only those objects with our label.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

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.

tmshort

/lgtm
Some minor comments that don't really need changes.
EDIT: Didn't notice this already merged!

tmshort · 2023-08-24T13:56:21Z

pkg/controller/install/webhook.go

+		return i.createOrUpdateValidatingWebhook(ogNamespacelabelSelector, caPEM, desc)
 	case v1alpha1.MutatingAdmissionWebhook:
-		i.createOrUpdateMutatingWebhook(ogNamespacelabelSelector, caPEM, desc)
+		return i.createOrUpdateMutatingWebhook(ogNamespacelabelSelector, caPEM, desc)
 	case v1alpha1.ConversionWebhook:
-		i.createOrUpdateConversionWebhook(caPEM, desc)
+		return i.createOrUpdateConversionWebhook(caPEM, desc)


Possible change in behavior? Is this to return a possible error in these handlers?

tmshort · 2023-08-24T13:58:51Z

pkg/controller/operators/catalog/operator.go

+					if rb.Labels == nil {
+						rb.Labels = map[string]string{}
+					}
+					rb.Labels[install.OLMManagedLabelKey] = install.OLMManagedLabelValue


I am seeing this idiom so much, it almost makes sense to create a utility API to initialize the labels and then add the label.

e.g.

addlabel(&rbLabels, install.OLMManagedLabelKey, install.OLMManagedLabelValue)

openshift-ci · 2023-08-24T14:00:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, ncdc, perdasilva, stevekuznetsov, tmshort

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~~ [awgreene,perdasilva,tmshort]

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

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

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

openshift-ci bot requested review from asmacdo, oceanc80 and jianzhangbjz August 22, 2023 18:03

stevekuznetsov mentioned this pull request Aug 22, 2023

Improve Memory Utilization for Microshift Deployment openshift/operator-framework-olm#530

Closed

38 tasks

stevekuznetsov force-pushed the skuznets/label-for-rbac branch 2 times, most recently from f1b5e49 to de75e19 Compare August 22, 2023 19:50

perdasilva reviewed Aug 23, 2023

View reviewed changes

cmd/olm/main.go Outdated Show resolved Hide resolved

perdasilva reviewed Aug 23, 2023

View reviewed changes

pkg/controller/bundle/bundle_unpacker.go Show resolved Hide resolved

perdasilva previously approved these changes Aug 23, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2023

openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2023

awgreene mentioned this pull request Aug 23, 2023

[WIP] refactor operator group cluster role name #2991

Closed

11 tasks

stevekuznetsov force-pushed the skuznets/label-for-rbac branch 3 times, most recently from 64345e9 to 7e445f6 Compare August 23, 2023 16:19

stevekuznetsov commented Aug 23, 2023

View reviewed changes

stevekuznetsov force-pushed the skuznets/label-for-rbac branch from 7e445f6 to feb1779 Compare August 23, 2023 17:05

openshift-ci bot assigned ncdc Aug 23, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2023

perdasilva approved these changes Aug 24, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2023

stevekuznetsov added 2 commits August 24, 2023 06:52

add a round-tripper to ensure we label non-OLM resources

3b6c7a2

This round-tripper is added to our *rest.Config when it's possible to detect that we're in a CI environment. Developers should set $CI=true to get this behavior locally. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

stevekuznetsov force-pushed the skuznets/label-for-rbac branch from feb1779 to 561b097 Compare August 24, 2023 12:52

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2023

ncdc approved these changes Aug 24, 2023

View reviewed changes

awgreene approved these changes Aug 24, 2023

View reviewed changes

stevekuznetsov added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2023

openshift-merge-robot merged commit 9e7031f into operator-framework:master Aug 24, 2023

tmshort approved these changes Aug 24, 2023

View reviewed changes

stevekuznetsov mentioned this pull request Aug 31, 2023

OCPBUGS-17157: *: label k8s objects we own #3020

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-17157: label non-OLM resources #3017

OCPBUGS-17157: label non-OLM resources #3017

stevekuznetsov commented Aug 22, 2023

openshift-ci-robot commented Aug 22, 2023

ncdc commented Aug 22, 2023

perdasilva left a comment

perdasilva commented Aug 23, 2023

stevekuznetsov commented Aug 23, 2023

stevekuznetsov commented Aug 23, 2023

stevekuznetsov Aug 23, 2023

stevekuznetsov Aug 23, 2023

stevekuznetsov Aug 23, 2023

stevekuznetsov Aug 23, 2023

ncdc commented Aug 23, 2023

perdasilva left a comment

openshift-ci bot commented Aug 24, 2023

awgreene left a comment

openshift-ci-robot commented Aug 24, 2023

tmshort left a comment •

edited

Loading

tmshort Aug 24, 2023

tmshort Aug 24, 2023

openshift-ci bot commented Aug 24, 2023

OCPBUGS-17157: label non-OLM resources #3017

OCPBUGS-17157: label non-OLM resources #3017

Conversation

stevekuznetsov commented Aug 22, 2023

openshift-ci-robot commented Aug 22, 2023

ncdc commented Aug 22, 2023

perdasilva left a comment

Choose a reason for hiding this comment

perdasilva commented Aug 23, 2023

stevekuznetsov commented Aug 23, 2023

stevekuznetsov commented Aug 23, 2023

stevekuznetsov Aug 23, 2023

Choose a reason for hiding this comment

stevekuznetsov Aug 23, 2023

Choose a reason for hiding this comment

stevekuznetsov Aug 23, 2023

Choose a reason for hiding this comment

stevekuznetsov Aug 23, 2023

Choose a reason for hiding this comment

ncdc commented Aug 23, 2023

perdasilva left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 24, 2023

awgreene left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 24, 2023

tmshort left a comment • edited Loading

Choose a reason for hiding this comment

tmshort Aug 24, 2023

Choose a reason for hiding this comment

tmshort Aug 24, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Aug 24, 2023

tmshort left a comment •

edited

Loading