Orchestrator installation - #1325
Conversation
Reviewer's GuideEnable script-driven setup for Orchestrator infrastructure by introducing a dedicated helper script and updating the build targets, remove outdated inline YAML and obsolete configs, and centralize installation instructions into a new orchestrator.md while simplifying dynamic-plugins.md references. Flow diagram for plugin-infra.sh script executionflowchart TD
Start([Start]) --> CheckAction{Action: apply or delete?}
CheckAction -- apply --> Serverless[Install OpenShift Serverless Operator]
Serverless --> WaitCRDs[Wait for CRDs]
WaitCRDs --> Knative[Install Knative Serving & Eventing]
Knative --> ServerlessLogic[Install OpenShift Serverless Logic Operator]
ServerlessLogic --> End([End])
CheckAction -- delete --> ServerlessLogicDel[Delete OpenShift Serverless Logic Operator]
ServerlessLogicDel --> KnativeDel[Delete Knative Serving & Eventing]
KnativeDel --> ServerlessDel[Delete OpenShift Serverless Operator]
ServerlessDel --> End
CheckAction -- other --> Error[Show error and exit]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @gazarenkov - I've reviewed your changes - here's some feedback:
- Double-check that removing the ClusterRole for knativeeventings and knativeservings in rbac-sonataflow.yaml won't prevent the operator from managing Knative resources.
- Ensure the relative link in dynamic-plugins.md ('orchestrator.md#plugin-dependencies') actually resolves to docs/orchestrator.md in the generated documentation.
- Add error handling around the kubectl wait and apply/delete commands in plugin-infra.sh to fail fast and surface any installation errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Double-check that removing the ClusterRole for knativeeventings and knativeservings in rbac-sonataflow.yaml won't prevent the operator from managing Knative resources.
- Ensure the relative link in dynamic-plugins.md ('orchestrator.md#plugin-dependencies') actually resolves to docs/orchestrator.md in the generated documentation.
- Add error handling around the kubectl wait and apply/delete commands in plugin-infra.sh to fail fast and surface any installation errors.
## Individual Comments
### Comment 1
<location> `config/profile/rhdh/plugin-infra/plugin-infra.sh:102` </location>
<code_context>
+
+# execution
+
+if [ "$action" == "apply" ]; then
+ serverless
+ echo "Waiting for CRDs to be established..."
</code_context>
<issue_to_address>
The script does not handle errors from kubectl commands.
If a kubectl command fails, the script continues, which may cause inconsistent infrastructure. Add error handling after each kubectl call or use 'set -e' to exit on errors.
</issue_to_address>
### Comment 2
<location> `config/profile/rhdh/plugin-infra/plugin-infra.sh:9` </location>
<code_context>
+action="${1:-apply}" # Default action is 'apply'
+
+serverless() {
+ kubectl $action -f - <<EOF
+apiVersion: v1
+kind: Namespace
</code_context>
<issue_to_address>
Directly passing user-supplied action to kubectl may allow unsupported operations.
Validate $action to ensure only 'apply' or 'delete' are allowed before passing it to kubectl.
</issue_to_address>
### Comment 3
<location> `config/profile/rhdh/plugin-infra/plugin-infra.sh:113` </location>
<code_context>
+ knative
+ serverless
+else
+ echo "Action '$action' is not supported. Use 'apply' (default) or 'delete'."
+fi
</code_context>
<issue_to_address>
Script does not exit with non-zero status on unsupported action.
Add 'exit 1' after the error message to ensure the script signals failure when an unsupported action is provided.
</issue_to_address>
### Comment 4
<location> `docs/orchestrator.md:44` </location>
<code_context>
+ ```bash
+ curl -sSLO https://raw.githubusercontent.com/redhat-developer/rhdh-operator/refs/heads/release-1.7/config/profile/rhdh/plugin-infra/plugin-infra.sh
+ ```
+You can specify the RHDH version in the URL (`/release-X.Y/`, e.g., `1.7` in this example) or main .
+2. Run the script:
+ ```bash
</code_context>
<issue_to_address>
Incomplete sentence: 'or main .' should be clarified.
Change 'or main .' to 'or use main.' or similar for better clarity.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
You can specify the RHDH version in the URL (`/release-X.Y/`, e.g., `1.7` in this example) or main .
=======
You can specify the RHDH version in the URL (`/release-X.Y/`, e.g., `1.7` in this example) or use `main`.
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `docs/orchestrator.md:28` </location>
<code_context>
+
+##### Steps
+1. [Prepare your infrastructure for OpenShift Serverless installation.](https://docs.redhat.com/en/documentation/red_hat_openshift_serverless/1.36/html/installing_openshift_serverless/preparing-serverless-install)
+2. [Install the OpenShift Serverless Operator.](https://docs.redhat.com/en/documentation/red_hat_openshift_serverless/1.33/html/installing_openshift_serverless/preparing-serverless-install)
+3. [Install Knative Serving.](https://docs.redhat.com/en/documentation/red_hat_openshift_serverless/1.36/html/installing_openshift_serverless/installing-knative-serving)
+4. [Install Knative Eventing.](https://docs.redhat.com/en/documentation/red_hat_openshift_serverless/1.36/html/installing_openshift_serverless/installing-knative-eventing)
</code_context>
<issue_to_address>
Version inconsistency in documentation link.
Update the installation link to use version 1.36 for consistency with the rest of the document.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
2. [Install the OpenShift Serverless Operator.](https://docs.redhat.com/en/documentation/red_hat_openshift_serverless/1.33/html/installing_openshift_serverless/preparing-serverless-install)
=======
2. [Install the OpenShift Serverless Operator.](https://docs.redhat.com/en/documentation/red_hat_openshift_serverless/1.36/html/installing_openshift_serverless/preparing-serverless-install)
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: gazarenkov <gazarenkov@users.noreply.github.com>
|
|
…figure-orchestrator
# Conflicts: # bundle/backstage.io/manifests/backstage-operator.clusterserviceversion.yaml # bundle/rhdh/manifests/backstage-operator.clusterserviceversion.yaml # docs/dynamic-plugins.md
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a script-driven installation process for the Orchestrator infrastructure on OpenShift, updates related documentation, and removes legacy configuration files.
- Added new documentation detailing installation steps in docs/orchestrator.md
- Introduced a helper script (plugin-infra.sh) to automate infrastructure installation
- Updated the Makefile to use the new helper script and removed legacy YAML files
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/orchestrator.md | Added detailed installation and configuration documentation |
| docs/dynamic-plugins.md | Updated to reference the new orchestrator plugin dependencies |
| config/profile/rhdh/plugin-infra/plugin-infra.sh | New helper script for automating infrastructure installation |
| config/profile/rhdh/plugin-infra/orchestrator/infra-sonataflow.yaml | Legacy Sonataflow infra configuration removed |
| config/profile/rhdh/plugin-infra/orchestrator/infra-serverless.yaml | Legacy Serverless infra configuration removed |
| config/profile/rhdh/plugin-infra/orchestrator/infra-knative-cr.yaml | Legacy Knative custom resource configuration removed |
| Makefile | Updated plugin-infra target to invoke the new helper script |
| $(KUBECTL) wait --for=condition=Established crd --all --timeout=60s; \ | ||
| $(KUBECTL) apply -f config/profile/$(PROFILE)/plugin-infra/orchestrator/infra-knative-cr.yaml; \ | ||
| $(KUBECTL) apply -f config/profile/$(PROFILE)/plugin-infra/orchestrator/infra-sonataflow.yaml; \ | ||
| @if [ -f "config/profile/$(PROFILE)/plugin-infra/plugin-infra.sh" ]; then \ |
There was a problem hiding this comment.
Would it make sense to rename the target (and the script as well) to orchestrator-infra, so that the intent is clearer?
There was a problem hiding this comment.
I do not think so, with generic name we can use it in Makefile for other profiles as well (same name). The intent is exactly to manage plugin infra.
There was a problem hiding this comment.
What if there is a future need for a similar flavor B (in the same rhdh profile like here) that would need some other plugin infra manifests to be deployed? Are you expecting these to be in the same plugin-infra script, thus installing everything, even if the user won't be using the Orchestrator flavor?
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @gazarenkov - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `config/profile/rhdh/plugin-infra/plugin-infra.sh:110` </location>
<code_context>
+ kubectl wait --for=condition=Established crd --all --timeout=60s
+ knative
+ serverless_logic
+elif [ "$action" == "delete" ]; then
+ serverless_logic
+ knative
</code_context>
<issue_to_address>
Order of deletion may cause dependency issues.
Ensure that deleting resources in this order won't result in errors from dependencies, such as CRDs being removed before their CRs.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
elif [ "$action" == "delete" ]; then
serverless_logic
knative
serverless
=======
elif [ "$action" == "delete" ]; then
# Delete custom resources first to avoid dependency issues
serverless_logic
knative
# Delete CRDs and related infrastructure last
serverless
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rm3l The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
9f03871
into
redhat-developer:main
* plugin-infra.sh * enable delete * orchestrator install docs * orchestrator doc and plugin-infra.sh * Regenerate bundle/installer manifests Co-authored-by: gazarenkov <gazarenkov@users.noreply.github.com> * fixes * fixes * fix doc * clean Makefile --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: gazarenkov <gazarenkov@users.noreply.github.com>
Description
Which issue(s) does this PR fix or relate to
https://issues.redhat.com/browse/RHIDP-7960
PR acceptance criteria
How to test changes / Special notes to the reviewer
To test infra installation:
Summary by Sourcery
Provide a script-driven approach for installing and removing the Orchestrator plugin’s required OpenShift Serverless, Knative, and logic operators, update the project Makefile to use this script, and add dedicated installation documentation.
New Features:
Enhancements: