MGMT-24108: make setup.sh idempotent for pre-installed operators - #77
Conversation
|
@omer-vishlitzky: This pull request references MGMT-24108 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 task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/setup.sh (1)
185-189: Consider whether CA issuer application should also be gated.This block is not guarded by an existence check, so every
setup.shrun re-appliesprerequisites/ca-issuer.yamlto the shared cluster. The PR rationale ("avoid overwriting existing installations... break webhooks") partially applies here too: if another tenant has customizedclusterissuer/default-ca, this will clobber it. The risk is lower than for operator installs (aClusterIssueris a single declarative resource, so re-applying the same manifest is a no-op), but if the intent is "don't touch anything that already exists on the shared cluster," symmetry would suggest gating this as well — e.g.,oc get clusterissuer default-ca &>/dev/null.Leaving this as an optional consideration; up to you whether shared-cluster safety should extend to cluster-scoped issuer resources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup.sh` around lines 185 - 189, The script currently always reapplies the ClusterIssuer manifest via retry_until calling 'oc apply -f prerequisites/ca-issuer.yaml' affecting 'clusterissuer/default-ca'; change this to first check for existence (e.g., run 'oc get clusterissuer default-ca' for presence) and only run the apply+retry_until if the ClusterIssuer is missing, and still call wait_for_resource 'clusterissuer/default-ca condition=Ready' after creation; update the block around retry_until and wait_for_resource to gate the apply by that existence check to avoid clobbering an existing cluster-scoped resource.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/setup.sh`:
- Around line 185-189: The script currently always reapplies the ClusterIssuer
manifest via retry_until calling 'oc apply -f prerequisites/ca-issuer.yaml'
affecting 'clusterissuer/default-ca'; change this to first check for existence
(e.g., run 'oc get clusterissuer default-ca' for presence) and only run the
apply+retry_until if the ClusterIssuer is missing, and still call
wait_for_resource 'clusterissuer/default-ca condition=Ready' after creation;
update the block around retry_until and wait_for_resource to gate the apply by
that existence check to avoid clobbering an existing cluster-scoped resource.
| echo "Timed out waiting for cert-manager CRD to exist" | ||
| exit 1 | ||
| } | ||
| if oc get deployment cert-manager -n cert-manager &>/dev/null; then |
There was a problem hiding this comment.
In OpenShift, if cert-manager has been installed using OLM, this will most probably be in the openshift-operators namespace.
| echo "Failed to apply trust-manager prerequisites" | ||
| exit 1 | ||
| } | ||
| if oc get deployment trust-manager -n cert-manager &>/dev/null; then |
There was a problem hiding this comment.
It is common practice to install trust-manager in the same namespace than cert-manager, so in OpenShift this will most probably be in the openshift-operators namespace.
6a383a2 to
fa7a7af
Compare
Skip operator installation if its deployment already exists on the cluster. For cert-manager, trust-manager, keycloak, and AAP, check both the expected namespace and openshift-operators since different installation methods deploy to different namespaces.
fa7a7af to
57e97b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/setup.sh (1)
213-220: Skipping namespace cleanup may leave stale resources on a broken prior install.If a previous
setup.shrun partially failed and leftdeployment/keycloak-servicein thekeycloaknamespace (but e.g., the kustomization resources or PVCs are stale/misconfigured), the skip path bypasseswait_for_namespace_cleanupandoc apply -k prerequisites/keycloak/entirely, and line 220 will just wait 10 minutes before failing. The same shape applies to AAP at 223–232. Per the PR goal "a broken pre-existing install will be detected", that detection happens but recovery requires manual teardown.Consider either (a) detecting readiness (not just existence) before deciding to skip, or (b) documenting that a broken pre-install must be manually torn down (via
teardown.sh) before re-running. A readiness-based check would be more user-friendly on shared clusters:♻️ Readiness-aware skip (illustrative)
-if oc get deployment keycloak-service -n keycloak &>/dev/null; then +if [[ "$(oc get deployment keycloak-service -n keycloak -o jsonpath='{.status.conditions[?(@.type=="Available")].status}' 2>/dev/null)" == "True" ]]; then echo "Keycloak is already installed, skipping..." else wait_for_namespace_cleanup keycloak oc apply -k prerequisites/keycloak/ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup.sh` around lines 213 - 220, Current logic only checks for existence of deployment/keycloak-service and skips cleanup/apply; change the branch in scripts/setup.sh to check readiness instead of mere existence: call oc to inspect deployment/keycloak-service status (e.g., Available condition and availableReplicas vs desired replicas) and only skip wait_for_namespace_cleanup and oc apply -k prerequisites/keycloak/ when the deployment is truly Ready; otherwise invoke wait_for_namespace_cleanup keycloak and oc apply -k prerequisites/keycloak/ as in the else path. Apply the same readiness-aware change for the AAP block (the corresponding deployment/service checks) so a partially broken pre-install triggers cleanup+reapply rather than silently waiting for wait_for_resource.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/setup.sh`:
- Around line 199-211: The CSV lookup and waits should be conditional so an
already-installed operator (or one installed outside OLM) doesn't cause an empty
AUTHORINO_CSV to be used; modify the logic around AUTHORINO_CSV and the
wait_for_resource calls so you only compute AUTHORINO_CSV and call
wait_for_resource clusterserviceversion/${AUTHORINO_CSV} when AUTHORINO_CSV is
non-empty (e.g. check oc get csv | grep authorino first), and always perform a
fallback wait_for_resource deployment/authorino-operator condition=Available (or
the existing oc get deployment authorino-operator presence check) so the script
proceeds cleanly when the operator exists but has no CSV; apply the same
conditional guarding pattern for the AAP CSV/wait logic as well.
- Around line 181-190: The apply step installs prerequisites/trust-manager.yaml
without honoring CERT_MANAGER_NS, causing the resource to be created in the
hardcoded "cert-manager" namespace while wait_for_resource looks in
${CERT_MANAGER_NS}; update the install path so the namespace is overridden at
apply time (e.g., render the manifest through kustomize with a namespace
transformer, or run a sed/yq patch to set metadata.namespace=${CERT_MANAGER_NS}
before calling oc apply), or explicitly detect a mismatched CERT_MANAGER_NS and
fail fast; ensure the code paths around retry_until (the oc apply call) and
wait_for_resource still reference ${CERT_MANAGER_NS} so deployment/trust-manager
is created and checked in the same namespace.
---
Nitpick comments:
In `@scripts/setup.sh`:
- Around line 213-220: Current logic only checks for existence of
deployment/keycloak-service and skips cleanup/apply; change the branch in
scripts/setup.sh to check readiness instead of mere existence: call oc to
inspect deployment/keycloak-service status (e.g., Available condition and
availableReplicas vs desired replicas) and only skip wait_for_namespace_cleanup
and oc apply -k prerequisites/keycloak/ when the deployment is truly Ready;
otherwise invoke wait_for_namespace_cleanup keycloak and oc apply -k
prerequisites/keycloak/ as in the else path. Apply the same readiness-aware
change for the AAP block (the corresponding deployment/service checks) so a
partially broken pre-install triggers cleanup+reapply rather than silently
waiting for wait_for_resource.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, omer-vishlitzky 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 |
|
/retest |
3 participants
https://redhat.atlassian.net/browse/MGMT-24108
Skip operator installation if its deployment already exists on the cluster. On shared clusters like hypershift1, running setup.sh unconditionally re-installs operators (cert-manager, trust-manager, authorino, keycloak, AAP), which overwrites existing installations and breaks webhooks.
For each operator, check if its deployment exists before applying. Still validate readiness afterward regardless, so a broken pre-existing install will be caught.
Summary by CodeRabbit