OTA-1531: [4/x] cvo: extract config informer creation #1189

petr-muller · 2025-05-07T18:23:36Z

The code to set up informers is messy so this change attempts to make it more consistent:

Alias all informer-related imports to somethinginformers
Everything made by NewSharedInformerFactory methods is now called somethingInformerFactory
Name factories in Context consistently: SomethingFactory
Document the purpose of individual factories in Context
Reorder code into clusters that deal with individual factory types

OTA-1531 will need the config.openshift.io group lister early in the CVO execution and outside of the Context structure, so this commit moves the necessary preparation code outside of NewControllerContext and passes these factories into it as parameters. Constructing factories at separate places is a little awkward but for now I try keeping the change small instead of trying to invent some centralized informer factory management. I think it is reasonable to treat config.openshift.io informers separately if they are actually used separately.

NewFilteredSharedInformerFactory is deprecated and can be replaced by NewSharedInformerFactoryWithOptions, where the filter is expressed as TweakListOption. The direct FieldSelector equality can be replaced with the OneTermEqualSelector helper.

Address an earlier review comment: #1188 (comment)

openshift-ci-robot · 2025-05-07T18:23:40Z

@petr-muller: This pull request references OTA-1531 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 story to target the "4.20.0" version, but no target version was set.

In response to this:

refactor(payload): separate tasks and metadata loading

refactor(payload): separate arch load from metadata and runtime

refactor(payload): rename loadReleaseFromMetadata->loadReleaseMetadata

cvo: read version from release metadata on startup

refactor: unify names in informer-related code

refactor: extract config informer factory prep

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 · 2025-05-07T18:23:40Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

petr-muller · 2025-05-07T18:32:00Z

/test unit lint images e2e-agnostic-operator

petr-muller · 2025-05-08T20:52:16Z

/test e2e-agnostic-operator

petr-muller · 2025-05-09T09:11:01Z

/test e2e-agnostic-operator e2e-agnostic-ovn-upgrade-into-change e2e-agnostic-ovn-upgrade-into-change-techpreview

petr-muller · 2025-05-09T13:46:32Z

/test e2e-agnostic-operator e2e-agnostic-ovn-upgrade-into-change e2e-agnostic-ovn-upgrade-into-change-techpreview

petr-muller · 2025-05-12T14:59:08Z

/test e2e-agnostic-operator e2e-agnostic-ovn-upgrade-into-change e2e-agnostic-ovn-upgrade-into-change-techpreview lint images unit

petr-muller · 2025-05-12T15:12:30Z

/test e2e-agnostic-operator e2e-agnostic-ovn-upgrade-into-change e2e-agnostic-ovn-upgrade-into-change-techpreview lint images unit

petr-muller · 2025-05-12T15:15:47Z

/test e2e-agnostic-operator e2e-agnostic-ovn-upgrade-into-change e2e-agnostic-ovn-upgrade-into-change-techpreview lint images unit

openshift-ci-robot · 2025-05-13T13:32:27Z

@petr-muller: This pull request references OTA-1531 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 story to target the "4.20.0" version, but no target version was set.

In response to this:

The code to set up informers is messy so this change attempts to make it more consistent:

Alias all informer-related imports to somethinginformers

Everything made by NewSharedInformerFactory methods is now called somethingInformerFactory

Name factories in Context consistently: SomethingFactory

Document the purpose of individual factories in Context

Reorder code into clusters that deal with individual factory types

OTA-1531 will need the config.openshift.io group lister early in the CVO execution and outside of the Context structure, so this commit moves the necessary preparation code outside of NewControllerContext and passes these factories into it as parameters. Constructing factories at separate places is a little awkward but for now I try keeping the change small instead of trying to invent some centralized informer factory management. I think it is reasonable to treat config.openshift.io informers separately if they are actually used separately.

NewFilteredSharedInformerFactory is deprecated and can be replaced by NewSharedInformerFactoryWithOptions, where the filter is expressed as TweakListOption. The direct FieldSelector equality can be replaced with the OneTermEqualSelector helper.

Address an earlier review comment: #1188 (comment)

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-05-13T13:32:49Z

@petr-muller: This pull request references OTA-1531 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 story to target the "4.20.0" version, but no target version was set.

In response to this:

The code to set up informers is messy so this change attempts to make it more consistent:

Alias all informer-related imports to somethinginformers

Everything made by NewSharedInformerFactory methods is now called somethingInformerFactory

Name factories in Context consistently: SomethingFactory

Document the purpose of individual factories in Context

Reorder code into clusters that deal with individual factory types

OTA-1531 will need the config.openshift.io group lister early in the CVO execution and outside of the Context structure, so this commit moves the necessary preparation code outside of NewControllerContext and passes these factories into it as parameters. Constructing factories at separate places is a little awkward but for now I try keeping the change small instead of trying to invent some centralized informer factory management. I think it is reasonable to treat config.openshift.io informers separately if they are actually used separately.

NewFilteredSharedInformerFactory is deprecated and can be replaced by NewSharedInformerFactoryWithOptions, where the filter is expressed as TweakListOption. The direct FieldSelector equality can be replaced with the OneTermEqualSelector helper.

Address an earlier review comment: #1188 (comment)

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.

petr-muller · 2025-05-13T13:33:49Z

/test lint

petr-muller · 2025-05-13T13:39:37Z

/cc @hongkailiu

DavidHurta · 2025-05-13T13:57:20Z

Happy to review this today/tomorrow.

/cc

petr-muller · 2025-05-13T15:55:32Z

pkg/start/start.go


-	coInformer := sharedInformers.Config().V1().ClusterOperators()
+	coInformer := clusterVersionConfigInformerFactory.Config().V1().ClusterOperators()


this is wrong, this should be configInformerFactory, not the cv one

but now the names at least make that visible

The code to set up informers is messy so this change attempts to make it more consistent: - Alias all informer-related imports to `somethinginformers` - Everything made by `NewSharedInformerFactory` methods is now called `somethingInformerFactory` - Name factories in `Context` consistently: `SomethingFactory` - Document the purpose of individual factories in `Context` - Reorder code into clusters that deal with individual factory types

OTA-1531 will need the config.openshift.io group lister early in the CVO execution and outside of the `Context` structure, so this commit moves the necessary preparation code outside of `NewControllerContext` and passes these factories into it as parameters. Constructing factories at separate places is a little awkward but for now I try keeping the change small instead of trying to invent some centralized informer factory management. I think it is reasonable to treat config.openshift.io informers separately if they are actually used separately.

`NewFilteredSharedInformerFactory` is deprecated and can be replaced by `NewSharedInformerFactoryWithOptions`, where the filter is expressed as `TweakListOption`. The direct `FieldSelector` equality can be replaced with the `OneTermEqualSelector` helper.

Address an earlier review comment: openshift#1188 (comment)

openshift-ci · 2025-05-13T19:40:02Z

@petr-muller: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-agnostic-ovn-upgrade-into-change-techpreview	`0c045a3`	link	true	`/test e2e-agnostic-ovn-upgrade-into-change-techpreview`

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.

petr-muller · 2025-05-14T11:17:13Z

/retest

DavidHurta

Looks good to me.

/hold

Putting on hold in case @hongkailiu is in mid-review or wanted to review these changes personally. However, two approvals are plenty for such changes, IMO. Feel free to unhold 🤷

DavidHurta · 2025-05-14T14:34:04Z

/lgtm

openshift-ci · 2025-05-14T14:36:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DavidHurta, petr-muller

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~~ [DavidHurta,petr-muller]

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

hongkailiu

LGTM

hongkailiu · 2025-05-14T14:25:28Z

pkg/start/start.go

-	OpenshiftConfigManagedInformerFactory informers.SharedInformerFactory
-	InformerFactory                       externalversions.SharedInformerFactory
-	OperatorInformerFactory               operatorexternalversions.SharedInformerFactory
+	// ClusterVersionInformerFactory should be used to get informers / listers for code that works with ClusterVersion resource


I love those comments. Thanks.

hongkailiu · 2025-05-14T14:44:48Z

pkg/start/start.go

+	// singleton in the cluster.
+	ClusterVersionInformerFactory configinformers.SharedInformerFactory
+	// ConfigInformerFactory should be used to get informers / listers for code that works with resources from the
+	// config.openshift.io group, _except_ the ClusterVersion resource singleton.


I did not understand why we use another factory for CV. But this is me, need more time to read the code.
My native guess is that their configurations are different for some reason.

hongkailiu · 2025-05-14T14:46:00Z

/hold cancel

petr-muller · 2025-05-14T15:52:05Z

/label no-qe

Still covered by the "will be tested on the last PR" agreement with QE

openshift-ci-robot · 2025-05-14T18:21:53Z

/retest-required

Remaining retests: 0 against base HEAD facd6a4 and 2 for PR HEAD 61ca445 in total

openshift-bot · 2025-05-15T11:27:47Z

[ART PR BUILD NOTIFIER]

Distgit: cluster-version-operator
This PR has been included in build cluster-version-operator-container-v4.20.0-202505151012.p0.g80bd498.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 7, 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 May 7, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2025

petr-muller force-pushed the ota-1531-04-extract-informer-factories branch from edc4c45 to e3c7151 Compare May 9, 2025 13:46

petr-muller force-pushed the ota-1531-04-extract-informer-factories branch from e3c7151 to 3fa96aa Compare May 12, 2025 14:58

petr-muller force-pushed the ota-1531-04-extract-informer-factories branch from 3fa96aa to 1fb5c12 Compare May 12, 2025 15:12

petr-muller force-pushed the ota-1531-04-extract-informer-factories branch from 1fb5c12 to 0c045a3 Compare May 12, 2025 15:15

petr-muller force-pushed the ota-1531-04-extract-informer-factories branch 4 times, most recently from 6121d53 to 5ec3e49 Compare May 13, 2025 13:31

petr-muller marked this pull request as ready for review May 13, 2025 13:32

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 May 13, 2025

openshift-ci bot requested a review from hongkailiu May 13, 2025 13:39

openshift-ci bot requested a review from DavidHurta May 13, 2025 13:57

petr-muller commented May 13, 2025

View reviewed changes

petr-muller added 4 commits May 13, 2025 17:58

fix: do not log a nil error

61ca445

Address an earlier review comment: openshift#1188 (comment)

petr-muller force-pushed the ota-1531-04-extract-informer-factories branch from 5ec3e49 to 61ca445 Compare May 13, 2025 16:00

DavidHurta approved these changes May 14, 2025

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 14, 2025

openshift-ci bot assigned DavidHurta May 14, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2025

hongkailiu reviewed May 14, 2025

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 14, 2025

openshift-ci bot added the no-qe Allows PRs to merge without qe-approved label label May 14, 2025

openshift-merge-bot bot merged commit 80bd498 into openshift:main May 14, 2025
15 checks passed

petr-muller deleted the ota-1531-04-extract-informer-factories branch May 16, 2025 21:29


		coInformer := sharedInformers.Config().V1().ClusterOperators()
		coInformer := clusterVersionConfigInformerFactory.Config().V1().ClusterOperators()

OTA-1531: [4/x] cvo: extract config informer creation #1189

OTA-1531: [4/x] cvo: extract config informer creation #1189

Uh oh!

Conversation

petr-muller commented May 7, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 7, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented May 7, 2025

Uh oh!

petr-muller commented May 7, 2025

Uh oh!

petr-muller commented May 8, 2025

Uh oh!

petr-muller commented May 9, 2025

Uh oh!

petr-muller commented May 9, 2025

Uh oh!

petr-muller commented May 12, 2025

Uh oh!

petr-muller commented May 12, 2025

Uh oh!

petr-muller commented May 12, 2025

Uh oh!

openshift-ci-robot commented May 13, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 13, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petr-muller commented May 13, 2025

Uh oh!

petr-muller commented May 13, 2025

Uh oh!

DavidHurta commented May 13, 2025

Uh oh!

petr-muller May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petr-muller May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petr-muller commented May 14, 2025

Uh oh!

DavidHurta left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidHurta commented May 14, 2025

Uh oh!

openshift-ci bot commented May 14, 2025

Uh oh!

hongkailiu left a comment

Choose a reason for hiding this comment

Uh oh!

hongkailiu May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongkailiu May 14, 2025

Choose a reason for hiding this comment

Uh oh!

hongkailiu commented May 14, 2025

Uh oh!

petr-muller commented May 14, 2025

Uh oh!

openshift-ci-robot commented May 14, 2025

Uh oh!

Uh oh!

openshift-bot commented May 15, 2025

Uh oh!

Uh oh!

petr-muller commented May 7, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 7, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 13, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 13, 2025 •

edited by openshift-ci bot

Loading

petr-muller May 13, 2025 •

edited

Loading

petr-muller May 13, 2025 •

edited

Loading

openshift-ci bot commented May 13, 2025 •

edited

Loading

DavidHurta left a comment •

edited

Loading

hongkailiu May 14, 2025 •

edited

Loading