OCPVE-719: feat: add support for olm capability by eggfoobar · Pull Request #795 · openshift/console-operator

eggfoobar · 2023-09-23T05:07:42Z

OLM is an optional component, adding conditional to remove informer on OLM resource

/hold

Need to find more context on the use of OLM resource in console and need to hold for API to be merged in openshift/api#1589

openshift-ci-robot · 2023-09-23T05:07:45Z

@eggfoobar: This pull request references OCPVE-711 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.15.0" version, but no target version was set.

Details

In response to this:

OLM is an optional component, adding conditional to remove informer on OLM resource

/hold

Need to find more context on the use of OLM resource in console and need to hold for API to be merged in openshift/api#1589

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.

openshift-ci-robot · 2023-09-29T14:50:00Z

@eggfoobar: This pull request references OCPVE-719 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.15.0" version, but no target version was set.

Details

In response to this:

OLM is an optional component, adding conditional to remove informer on OLM resource

/hold

Need to find more context on the use of OLM resource in console and need to hold for API to be merged in openshift/api#1589

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.

wking · 2023-09-29T16:46:13Z

 }

+func isCapabilityEnabled(client configclientv1.ConfigV1Interface, expectedCap configv1.ClusterVersionCapability) bool {
+	cv, err := client.ClusterVersions().Get(context.TODO(), "version", metav1.GetOptions{})


one-shot ClusterVersion fetch attempt here on operator initiation without a limiting Context has some risks. A lower-stakes approach would be to not enable the OLM portions until we can confirm they're enabled, so you could have behavior like:

Console operator is coming up.

Console operator attempts to retrieve ClusterVersion with a 5m deadline, but this fails.

Console operator assumes that OLM is not enabled, and goes on to operate its other aspects.

Follow-up attempt to check in on ClusterVersion succeeds, and it turns out that OLM is enabled.

Console operator starts operating on the OLM-gated stuff too.

You'll want something like that anyway, because admins may decide to enable capabilities post-install. Only post-install disabling is not supported.

@wking but looks like the current implementation is registering the informer and fetching OLM config only if the the capability is enabled. Is the only issue the context ?

I think @wking would just like this to be a bit more robust for future use. So in the event that a user re-enables OLM after the fact, so the pod will need to be restarted for the console to correctly see the resources.

I'm not entirely sure what to do about the informer since it's attached at startup, I updated the code a bit since we just needed to know if a resource exists for that informer. I also updated the error checking so that we don't always skip based off of initial state. If I understand this right, the informer won't be attached, but the lister will be queried after the fact, so if the user re-enables OLM, then the CSV query will go through.

Right but in case the capability will get enabled post-installation we are not registering the informer. Please check my comment below.

jhadvig · 2023-10-05T10:10:53Z

+	defer cancel()
+
+	// We check if the resource exists in the cluster at all before attaching an informer
+	if _, err := dynamicClient.Resource(olmGroupVersionResource).List(ctx, metav1.ListOptions{}); !isNoMatchError(err) {


@eggfoobar what if the OLM capability is enabled post installation? Can the capability be enable at any time? or is there any restriction ?
In case the capability will be disabled at start and then enabled, I dont think the informer will be registered, since we are registering the informers in the NewConsoleOperator constructor.
Wondering if it wont be better to restart the operator's pod on this change?

Yeah, that might be a good idea, I added some WIP logic to get this running in my e2e tests that just checks if the resource exists but in the future it would be preferable to have an informer on the clusterversions and if we have the resource enabled, we trigger a reboot then. There might be a more elegant way of doing that, but that's what came to mind, what do you think?

So this is something I had in mind, to basically kill the pod end force new rollout so the informer is registered 👍

jhadvig

@eggfoobar thanks for addressing this 👍 I like how this got implemented

/lgtm
/approve

jhadvig · 2023-10-12T14:09:59Z

QE Approver:
/assign @yanpzhan
Docs Approver:
/assign @opayne1
PX Approver:
/assign @RickJWagner

opayne1 · 2023-10-12T17:45:24Z

/label docs-approved

RickJWagner · 2023-10-12T18:40:27Z

/label px-approved

jhadvig · 2023-10-12T19:51:54Z

/hold cancel

eggfoobar · 2023-10-13T02:47:02Z

/hold

Apologies to add a hold on this so late, noticing some errors in the no-cap job that I would like to look over before we merge this in. Will unhold tomorrow as soon as I can.

OLM is an optional component, adding conditional to remove informer on OLM resource Signed-off-by: ehila <ehila@redhat.com> fix: add olm check logic in sync loop for isCopiedCSVsDisabled Signed-off-by: ehila <ehila@redhat.com> refactor: small refinement, more logging and comments Signed-off-by: ehila <ehila@redhat.com>

eggfoobar · 2023-10-13T06:42:07Z

/unhold

@jhadvig I was able to verify the error I was seeing wasn't happening again. I went a head and added some more comments and logs to make sure it's clear what's happening. Please take a look when you have a moment, thanks!

eggfoobar · 2023-10-13T11:46:22Z

/retest-required

jhadvig · 2023-10-13T12:12:04Z

/lgtm

openshift-ci · 2023-10-13T12:12:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eggfoobar, jhadvig

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~~ [jhadvig]

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

yanpzhan · 2023-10-17T13:06:39Z

I built an image against the pr using cluster-bot, have done these tests:

Launched a cluster using the image successfully, and updated it to 4.15 nightly payload successfully.
Launched another 4.14 nightly cluster, updated it to the image I created successfully.
Launched a cluster using the image with installation options: "baselineCapabilitySet: None" and "additionalEnabledCapabilities: ["Console"]", the cluster was installed successfully. Checked on web console, there was not operators in OperatorHub page.

@eggfoobar @jhadvig except above test If any other tests are needed for this pr?

eggfoobar · 2023-10-17T13:43:27Z

@yanpzhan That looks good to me, just to make sure the current changes were in. Can you tell me which 4.15 nightly release you used, and if you still have the cluster up, can you check oc get clusterversions version -oyaml and make sure the knownCapabilities include OperatorLifecycleManager?

yanpzhan · 2023-10-17T14:16:21Z

@eggfoobar The target 4.15 nightly payload is 4.15.0-0.nightly-2023-10-09-101435, on the cluster, I checked oc get clusterversions version -oyaml, it doesn't contain "OperatorLifecycleManager" in "knownCapabilities". I wonder if this build doesn't contains changes about OLM Capability.

yanpzhan · 2023-10-17T14:16:23Z

@eggfoobar The target 4.15 nightly payload is 4.15.0-0.nightly-2023-10-09-101435, on the cluster, I checked oc get clusterversions version -oyaml, it doesn't contain "OperatorLifecycleManager" in "knownCapabilities". I wonder if this build doesn't contains changes about OLM Capability.

eggfoobar · 2023-10-17T14:27:07Z

Ah yup, @yanpzhan would you be able to verify again with this nightly, 4.15.0-0.nightly-2023-10-17-065657 ?That should have the updated capabilities

yanpzhan · 2023-10-17T15:21:04Z

I launched a cluster with the pr and image 4.15.0-0.nightly-2023-10-17-065657 successfully, and checked clusterversions 'version', it contains "OperatorLifecycleManager" in "knownCapabilities" now.

eggfoobar · 2023-10-17T15:55:10Z

Perfect, thanks @yanpzhan !

In terms of what we need to verify with this PR, two things:

Console should function normally with out OLM being enabled ✅ (you've already verified this part if no errors are happening)
When you re-enable OLM via oc edit clusterversions version to enable OperatorLifecycleManager, after CVO installs OLM, we expect that console will restart in about 5 minutes and continue operating as normal.

yanpzhan · 2023-10-18T03:23:53Z

@eggfoobar I've checked again on new cluster. The 2 points you mentioned passed, no errors. Console works after re-enabled OLM by set 'OperatorLifecycleManager' in Clusterversions 'version'.
Before :

oc get clusterversions.config.openshift.io version -ojson | jq .status.capabilities

{
"enabledCapabilities": [
"Console"
],
"knownCapabilities": [
"Build",
"CSISnapshot",
"Console",
"DeploymentConfig",
"ImageRegistry",
"Insights",
"MachineAPI",
"NodeTuning",
"OperatorLifecycleManager",
"Storage",
"baremetal",
"marketplace",
"openshift-samples"
]
}
After:

oc get clusterversions.config.openshift.io version -ojson | jq .status.capabilities

{
"enabledCapabilities": [
"Console",
"OperatorLifecycleManager"
],
"knownCapabilities": [
"Build",
"CSISnapshot",
"Console",
"DeploymentConfig",
"ImageRegistry",
"Insights",
"MachineAPI",
"NodeTuning",
"OperatorLifecycleManager",
"Storage",
"baremetal",
"marketplace",
"openshift-samples"
]
}
Could you tell me what resources about OLM I could check to compare them before/after enable OLM?

eggfoobar · 2023-10-18T03:32:29Z

Perfect, once OLM has been enabled, the specific resource that Console is looking for is olmconfigs so if you see that then things should be good, oc get olmconfigs, as long as that call doesn't fail then Console will work. That resource does not exist when OLM is not enabled.

yanpzhan · 2023-10-18T08:45:24Z

Thanks for your guide. There is no issue for now.
/label qe-approved

openshift-ci-robot · 2023-10-18T08:45:47Z

@eggfoobar: This pull request references OCPVE-719 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.15.0" version, but it targets "openshift-4.15" instead.

Details

In response to this:

OLM is an optional component, adding conditional to remove informer on OLM resource

/hold

Need to find more context on the use of OLM resource in console and need to hold for API to be merged in openshift/api#1589

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.

eggfoobar · 2023-10-18T12:38:23Z

/retest-required

eggfoobar · 2023-10-18T13:42:36Z

/retest-required

openshift-ci-robot · 2023-10-18T13:54:15Z

/retest-required

Remaining retests: 0 against base HEAD d02bd9b and 2 for PR HEAD bbcf813 in total

eggfoobar · 2023-10-18T20:48:11Z

/retest-required

openshift-ci · 2023-10-18T23:05:06Z

@eggfoobar: all tests passed!

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/test-infra repository. I understand the commands that are listed here.

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

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 23, 2023

openshift-ci Bot requested review from jhadvig and spadgett September 23, 2023 05:08

eggfoobar mentioned this pull request Sep 23, 2023

OCPVE-672: feat: add the OLM as a capability openshift/api#1589

Merged

eggfoobar changed the title ~~OCPVE-711: feat: add support for olm capability~~ OCPVE-719: feat: add support for olm capability Sep 29, 2023

wking reviewed Sep 29, 2023

View reviewed changes

eggfoobar force-pushed the add-olm-capability-support branch 2 times, most recently from db606d2 to 4b3da5e Compare October 5, 2023 02:08

jhadvig reviewed Oct 5, 2023

View reviewed changes

eggfoobar force-pushed the add-olm-capability-support branch from 34f1536 to e68c0a6 Compare October 5, 2023 20:53

jhadvig approved these changes Oct 12, 2023

View reviewed changes

openshift-ci Bot assigned jhadvig Oct 12, 2023

openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 12, 2023

openshift-ci Bot added the docs-approved Signifies that Docs has signed off on this PR label Oct 12, 2023

openshift-ci Bot added the px-approved Signifies that Product Support has signed off on this PR label Oct 12, 2023

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2023

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2023

eggfoobar force-pushed the add-olm-capability-support branch from e68c0a6 to bbcf813 Compare October 13, 2023 06:39

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2023

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2023

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2023

openshift-ci Bot added the qe-approved Signifies that QE has signed off on this PR label Oct 18, 2023

openshift-ci Bot merged commit 301b4bf into openshift:master Oct 18, 2023

Conversation

eggfoobar commented Sep 23, 2023

Uh oh!

openshift-ci-robot commented Sep 23, 2023 • 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 Sep 29, 2023 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

jhadvig Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

eggfoobar Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

jhadvig Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

jhadvig Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

eggfoobar Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

jhadvig Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

jhadvig left a comment

Choose a reason for hiding this comment

Uh oh!

jhadvig commented Oct 12, 2023

Uh oh!

opayne1 commented Oct 12, 2023

Uh oh!

RickJWagner commented Oct 12, 2023

Uh oh!

jhadvig commented Oct 12, 2023

Uh oh!

eggfoobar commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eggfoobar commented Oct 13, 2023

Uh oh!

eggfoobar commented Oct 13, 2023

Uh oh!

jhadvig commented Oct 13, 2023

Uh oh!

openshift-ci Bot commented Oct 13, 2023

Uh oh!

yanpzhan commented Oct 17, 2023

Uh oh!

eggfoobar commented Oct 17, 2023

Uh oh!

yanpzhan commented Oct 17, 2023

Uh oh!

yanpzhan commented Oct 17, 2023

Uh oh!

eggfoobar commented Oct 17, 2023

Uh oh!

yanpzhan commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eggfoobar commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanpzhan commented Oct 18, 2023

oc get clusterversions.config.openshift.io version -ojson | jq .status.capabilities

oc get clusterversions.config.openshift.io version -ojson | jq .status.capabilities

Uh oh!

eggfoobar commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanpzhan commented Oct 18, 2023

Uh oh!

openshift-ci-robot commented Oct 18, 2023 • 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 Sep 23, 2023 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Sep 29, 2023 •

edited by openshift-ci Bot

Loading

eggfoobar commented Oct 13, 2023 •

edited

Loading

yanpzhan commented Oct 17, 2023 •

edited

Loading

eggfoobar commented Oct 17, 2023 •

edited

Loading

eggfoobar commented Oct 18, 2023 •

edited

Loading

openshift-ci-robot commented Oct 18, 2023 •

edited by openshift-ci Bot

Loading