MULTIARCH-4557: Sync import mode image config status field in the observed config #582

Prashanth684 · 2024-08-16T17:01:49Z

As per openshift/enhancements#1605, there was a
new field introduced in the image config spec and status which reflects
the global value to be set for imagestream import mode which is behind a
featuregate. This PR reads the image config status, checks if the importmode
string is present and syncs that value in the imagepolicyconfig of the
observed config to be used by the apiserver.

API changes: openshift/api#1928
openshift-apiserver changes: openshift/openshift-apiserver#443
image-registry-operator changes: openshift/cluster-image-registry-operator#1090

openshift-ci-robot · 2024-08-16T17:01:53Z

@Prashanth684: This pull request references MULTIARCH-4557 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.18.0" version, but no target version was set.

Details

In response to this:

As per openshift/enhancements#1605, there was a
new field introduced in the image config spec and status which reflects
the global value to be set for imagestream import mode which is behind a
featuregate. This PR reads the image config status, checks if the importmode
string is present and syncs that value in the imagepolicyconfig of the
observed config to be used by the apiserver.

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 · 2024-08-20T17:07:35Z

@Prashanth684: This pull request references MULTIARCH-4557 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.18.0" version, but no target version was set.

Details

In response to this:

As per openshift/enhancements#1605, there was a
new field introduced in the image config spec and status which reflects
the global value to be set for imagestream import mode which is behind a
featuregate. This PR reads the image config status, checks if the importmode
string is present and syncs that value in the imagepolicyconfig of the
observed config to be used by the apiserver.

API changes: openshift/api#1928
openshift-apiserver changes: openshift/openshift-apiserver#443
image-registry-operator changes: openshift/cluster-image-registry-operator#1090

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.

dinhxuanvu

Overall, looks good to me.

go.mod

Prashanth684 · 2024-08-22T14:06:32Z

/test e2e-aws-ovn

dinhxuanvu

/lgtm

dinhxuanvu · 2024-08-22T16:04:31Z

@tkashem @p0lyn0mial For final revie and approval.

Prashanth684 · 2024-08-26T14:41:28Z

/test e2e-aws-ovn

tkashem

Have a few minor comments
/approve
Thanks

tkashem · 2024-09-19T22:10:16Z

pkg/operator/configobservation/images/observe_images.go

 }

+func ObserveImagestreamImportMode(genericListers configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (map[string]interface{}, []error) {
+	listers := genericListers.(configobservation.Listers)


is it possible to use lister := configobserver.ImageConfigLister directly?

the ImageConfigLister is under the Listers object here which is passed through here. I believe that is why it is done this way?

@tkashem is my observation correct, or is there another way to do it?

pkg/operator/configobservation/images/observe_images.go

tkashem · 2024-09-19T22:16:58Z

pkg/operator/configobservation/images/observe_images.go

+	configImage, err := listers.ImageConfigLister.Get("cluster")
+	if errors.IsNotFound(err) {
+		klog.Warningf("image.config.openshift.io/cluster: not found")
+		return observedConfig, errs


can you please, perhaps with a comment, clarify why we have to special case NotFound here, and return observedConfig (empty) instead of prevObservedConfig?

honestly, i followed the pattern of the other observer functions in this file. Going back, it looks like #57 introduced it. That is a good question but I don't know what the motivation for that was.

maybe because if the object is not found, it has been potentially removed and in that case we want it to be empty?

probably yes, @sanchezl if the cluster object (the thing we are observing for config) is missing, is it an error, or the operator does its best to continue with the old configuration?

pkg/operator/configobservation/images/observe_images.go

pkg/operator/configobservation/images/observe_images_test.go

…erved config As per openshift/enhancements#1605, there was a new field introduced in the image config spec and status which reflects the global value to be set for imagestream import mode which is behind a featuregate. This PR reads the image config status, checks if the importmode string is present and syncs that value in the imagepolicyconfig of the observed config to be used by the apiserver.

openshift-ci · 2024-09-24T19:37:47Z

@Prashanth684: 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-gcp-operator-encryption-rotation-aesgcm	`0f0e3a7`	link	false	`/test e2e-gcp-operator-encryption-rotation-aesgcm`

Full PR test history. Your PR dashboard.

Details

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.

Prashanth684 · 2024-09-25T16:36:10Z

/test e2e-upgrade

aleskandro · 2024-09-26T16:02:11Z

/lgtm

tkashem · 2024-09-26T16:03:10Z

/lgtm

/hold
(cancel if you are not waiting for others to review)

Prashanth684 · 2024-09-26T16:05:18Z

/hold cancel

openshift-ci · 2024-09-26T16:05:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aleskandro, dinhxuanvu, Prashanth684, tkashem

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [tkashem]

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

openshift-bot · 2024-09-26T21:39:38Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-openshift-apiserver-operator
This PR has been included in build ose-cluster-openshift-apiserver-operator-container-v4.18.0-202409262111.p0.gfe18366.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 Aug 16, 2024

openshift-ci bot requested review from adambkaplan and dgrisonnet August 16, 2024 17:02

Prashanth684 force-pushed the importmode-imagestream branch from 80311e9 to 36a5f27 Compare August 16, 2024 17:27

Prashanth684 mentioned this pull request Aug 19, 2024

MULTIARCH-4971: Sync ImageStreamImportMode setting in the image config status openshift/cluster-image-registry-operator#1090

Merged

Prashanth684 force-pushed the importmode-imagestream branch from 36a5f27 to 4bf8d3e Compare August 19, 2024 16:16

Prashanth684 mentioned this pull request Aug 20, 2024

MULTIARCH-4558: Set default for imagestreamImportMode based on observed config changes in the image config openshift/openshift-apiserver#443

Merged

dinhxuanvu suggested changes Aug 21, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Prashanth684 force-pushed the importmode-imagestream branch from 946f24b to ec69e2b Compare August 21, 2024 22:59

dinhxuanvu reviewed Aug 22, 2024

View reviewed changes

openshift-ci bot assigned dinhxuanvu Aug 22, 2024

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

Prashanth684 requested review from p0lyn0mial and tkashem August 22, 2024 16:20

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

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 22, 2024

Prashanth684 force-pushed the importmode-imagestream branch from 0f0e3a7 to 3bfa20f Compare September 10, 2024 20:01

tkashem reviewed Sep 19, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 19, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2024

Prashanth684 force-pushed the importmode-imagestream branch from 3bfa20f to b379f0d Compare September 20, 2024 00:17

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2024

Prashanth684 force-pushed the importmode-imagestream branch 2 times, most recently from 0a2c4aa to b283df7 Compare September 24, 2024 17:09

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

openshift-ci bot assigned aleskandro Sep 26, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2024

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

openshift-ci bot assigned tkashem Sep 26, 2024

openshift-merge-bot bot merged commit fe18366 into openshift:master Sep 26, 2024

MULTIARCH-4557: Sync import mode image config status field in the observed config #582

MULTIARCH-4557: Sync import mode image config status field in the observed config #582

Uh oh!

Conversation

Prashanth684 commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 16, 2024 • 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 Aug 20, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dinhxuanvu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Prashanth684 commented Aug 22, 2024

Uh oh!

dinhxuanvu left a comment

Choose a reason for hiding this comment

Uh oh!

dinhxuanvu commented Aug 22, 2024

Uh oh!

Prashanth684 commented Aug 26, 2024

Uh oh!

tkashem left a comment

Choose a reason for hiding this comment

Uh oh!

tkashem Sep 19, 2024

Choose a reason for hiding this comment

Uh oh!

Prashanth684 Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

Prashanth684 Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tkashem Sep 19, 2024

Choose a reason for hiding this comment

Uh oh!

Prashanth684 Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

Prashanth684 Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

tkashem Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openshift-ci bot commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Prashanth684 commented Sep 25, 2024

Uh oh!

aleskandro commented Sep 26, 2024

Uh oh!

tkashem commented Sep 26, 2024

Uh oh!

Prashanth684 commented Sep 26, 2024

Uh oh!

openshift-ci bot commented Sep 26, 2024

Uh oh!

openshift-bot commented Sep 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Prashanth684 commented Aug 16, 2024 •

edited

Loading

openshift-ci-robot commented Aug 16, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 20, 2024 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Sep 24, 2024 •

edited

Loading