Added Knative CRDs and Knative CR creation logic#117
Added Knative CRDs and Knative CR creation logic#117openshift-merge-bot[bot] merged 12 commits intoredhat-developer:mainfrom
Conversation
|
Can you add to the README.md the version of the CRDs and where to download them from so it will simplify the maintenance of the chart. |
|
also, chart version bump is needed. |
There was a problem hiding this comment.
This PR will add Knative Eventing and Serving CRDs to the Orchestrator Infra helm chart.
Probably dumb question, but I might be missing something.. Shouldn't these CRDs be actually installed after the InstallPlans of the Serverless and Serverless Logic Operators are approved.
At least, that's what I noticed with the current orchestrator-infra chart from the main branch (I was able to see new Knative* CRDs, unless the ones this PR adds are different?):
# Before installing the orchestrator-infra chart
$ oc get crd | grep -i knative
$ oc get knativeeventings -A
error: the server doesn't have a resource type "knativeeventings"
$ oc get knativeservings -A
error: the server doesn't have a resource type "knativeservings"
# Install the orchestrator-infra chart and manually approve the InstallPlans
# ...
# After a few seconds/minutes
$ oc get crd | grep -i knative
knativeeventings.operator.knative.dev 2025-03-24T17:18:45Z
knativekafkas.operator.serverless.openshift.io 2025-03-24T17:18:46Z
knativeservings.operator.knative.dev 2025-03-24T17:18:44Z
$ oc get knativeservings -A
No resources found
$ oc get knativeeventings -A
No resources found
@rm3l you are correct that the infra chart will deliver all of the required CRDs, however, the issue is with the timing. When the chart installs the subscription for the openshift-serverless operator, it will not immediately create the CRDs. There will be some time before the CRDs are created. During that time, the chart will attempt to apply the knativeservice and knativeeventing CRs and will fail since the CRDs will not be available. |
Thanks for clarifying, @masayag! Could the CRDs here potentially conflict with the ones that are coming from the bundles from the InstallPlans? Asking because, based on the README, it is not clear to me which versions of the CRDs the maintainers would have to keep an eye out on, based on the Subscriptions channels (alpha for Serverless Logic and stable for Serverless).. Thinking out loud, but since the cluster admin would anyway need to manually approve the InstallPlans in order to deploy the Operators, wouldn't it make sense to document that they need to create the CRs as well (or do so by running an additional |
Hey @rm3l, Moti and I discussed this and we believe the best approach is to have the chart shipped with having the CRD versions matching between the ones the chart installs, and the ones the subscription's CSV is trying to install. The chart will be in alignment, and if the subscription version will need to change, the README will document how to verify the correct CRD version and download it for the chart to be updated. There will be no runtime validation for the versions, rather each chart release will have to verify this (and a pre-release / CI test can be added). |
Sounds good. Thanks.
Could you please capture this in a JIRA task, so it is not forgotten? Thanks. |
rm3l
left a comment
rm3l
left a comment
There was a problem hiding this comment.
@ElaiShalevRH Could you rebase your PR branch and fix the Git conflicts?
|
@rm3l And sure, let me rebase and push with the versioning disclaimer (*) I still need to add the actual steps for retrieving the CRD version from the subscription, so that the maintainer / future CI task has a reference for that |
elai-shalev
force-pushed
the
import-crds
branch
from
7d28c0a to
a146e09
Compare
|
@rm3l |
There was a problem hiding this comment.
Hey, I've added the correct CRDs and the guide to update them. The chart should be shipped with these CRDs in accordance to the subscription.
Once this change is merged we can return to the backstage chart PR and remove the creation of the Knative resources from there
Sounds good.
btw, the CI here is failing on upgrade, which works fine for me on an OpenShift cluster. Do you see why?
As I understand it, this is how Helm handles CRDs defined in the crds directory. They are supposed to be installed once with helm install, but helm upgrade will not install them. I guess that's why we are seeing this issue in CI.
Maybe you approved the InstallPlans in your OpenShift cluster before, and the CRDs got installed? I was able to reproduce the CI issue in my local CRC cluster by simulating what ct does:
- running
helm installfrom themainbranch - and then running
helm upgrade <release-name>from your PR branch
$ helm upgrade my-orchestrator-infra-1 charts/orchestrator-infra
Error: UPGRADE FAILED: [resource mapping not found for name: "knative-eventing" namespace: "knative-eventing" from "": no matches for kind "KnativeEventing" in version "operator.knative.dev/v1beta1"
ensure CRDs are installed first, resource mapping not found for name: "knative-serving" namespace: "knative-serving" from "": no matches for kind "KnativeServing" in version "operator.knative.dev/v1beta1"
ensure CRDs are installed first]We are purposely testing in-place upgrades to catch potential issues that we had in the past with the RHDH chart. And I think it makes sense too for the orchestrator-infra chart.
As a potential workaround, maybe we can temporarily change the CI job to explicitly kubectl apply the CRDs here, before running ct install.
And revert this once this PR is merged, so that the base chart would be the one with the CRDs. Thoughts?
I believe you are correct. After the InstallPlan was approved the new CRDs took place and then were included for the upgrade.
I think adding an apply command for the CRDs would work for the update, yes. But at what stage do you intend to move the CRDs to the backstage chart? |
I didn't mean to move the CRDs to the backstage chart. They should remain part of the orchestrator-infra chart, IMO, no?
This way, I think the upgrade checks would pass (and would reflect what the orchestrator-infra user would likely do). |
|
@rm3l Hey, any idea why only the Test Latest Release failing? |
🤔 Not sure why it passed on I wonder if the way the resources are being declared in the |
|
@rm3l |
|
openshift-merge-bot
Bot
merged commit 4526502
into
redhat-developer:main
As discussed in [1], now that 0.0.3 (with the CRDs) is the baseline for the upgrade checks, this is no longer needed. [1] redhat-developer#117 (comment)
…rt (#122) * ci: Revert manually applying the CRDs from the orchestrator-infra chart As discussed in [1], now that 0.0.3 (with the CRDs) is the baseline for the upgrade checks, this is no longer needed. [1] #117 (comment) * [TO REMOVE] Test the changes by bumping the orchestrator-chart version * Revert "[TO REMOVE] Test the changes by bumping the orchestrator-chart version" This reverts commit 2a284d9.
3 participants
Description of the change
This PR will add Knative Eventing and Serving CRDs to the Orchestrator Infra helm chart. This will allow an admin to install Knative resources in the knative-eventing and knative-serving namespaces, as part of pre-requisite steps necessary for installing RHDH with the Orchestrator flavor.
These changes are related to the issues from this PR.
This PR also adds logic for creating the Knative namespaces on a cluster, and creating the relevant Knative CRs.
Knative CR creation logic
Existing or Associated Issue(s)
Additional Information
Checklist
Chart.yamlaccording to semver.values.yamland added to the README.md. The pre-commit utility can be used to generate the necessary content. Usepre-commit run -ato apply changes.pre-commithook.ct lintcommand.