docs: AllNamespaces install mode design proposal - #2371
Conversation
Adds a phased plan to migrate OADP from OwnNamespace-only to supporting AllNamespaces install mode via a separate OLM channel. Covers namespace decoupling, empty watch namespace handling, two-bundle build infrastructure, e2e test updates, CI integration, and customer migration documentation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe design proposal specifies OADP AllNamespaces installation through a second OLM channel while preserving OwnNamespace. It covers bundle behavior, validation, CI, migration, rollback, compatibility, security, and future namespace handling. ChangesAllNamespaces installation design
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath 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 |
openshift-ci
Bot
added
the
approved
Phase 3 now presents both approaches with trade-offs and decision criteria rather than pre-deciding on channels. Phase 4 and Phase 6 document how e2e tests and migration steps differ per option. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
docs/design/allnamespaces-install-mode_design.md (1)
71-80: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDefine one owner for
OPERATOR_NAMESPACE.Phase 1 adds this variable to the shared manager manifest. Phase 3 says the AllNamespaces overlay adds it again. Specify whether the overlay replaces or reuses the base entry, and validate that each rendered Deployment contains exactly one
OPERATOR_NAMESPACEentry.Also applies to: 139-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/allnamespaces-install-mode_design.md` around lines 71 - 80, Clarify the ownership of the OPERATOR_NAMESPACE environment variable between the shared manager manifest and the AllNamespaces overlay, specifying whether the overlay reuses or replaces the base entry. Update the design steps to ensure each rendered Deployment contains exactly one OPERATOR_NAMESPACE entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/allnamespaces-install-mode_design.md`:
- Around line 223-228: Replace the placeholder channel in the “Step 2: Switch
subscription channel” command with a concrete release-specific channel matching
the documented minimum migration version, or define and document a template
variable outside the executable command block before using it.
- Around line 317-324: Update the phase dependency table so final Phase 6
migration and rollback documentation depends on validated E2E behavior from
Phase 4, rather than only Phase 3. Keep any explicitly draft documentation
parallelizable if needed, while ensuring finalized instructions are not treated
as complete before Phase 4 validation.
- Around line 69-80: Update the Changes table to include
internal/controller/cloudstorage_controller.go, identifying its CloudStorage STS
lookup and WaitForSecret flow as using OPERATOR_NAMESPACE instead of
WATCH_NAMESPACE. Define that the namespace source is the operator namespace,
consistent with the Phase 1 credential-creation changes.
- Around line 128-135: Add the `text` language identifier to the fenced catalog
tree code block around the catalog example, leaving the tree content unchanged.
- Around line 223-279: Rework the migration and rollback sequences around the
OperatorGroup transition so every selected CSV is compatible with the currently
configured OperatorGroup and has a valid OLM update path from the installed CSV.
Use a transition CSV supporting both OwnNamespace and AllNamespaces, or move to
the global OperatorGroup before selecting an AllNamespaces-only channel, and
include verification that each CSV reaches Succeeded rather than
UnsupportedOperatorGroup.
- Around line 147-149: The catalog generation changes must preserve the existing
OwnNamespace package as the published default channel. Use
operators.operatorframework.io.bundle.channel.default.v1 rather than
defaultChannelName, restrict DEFAULT_CHANNEL=dev-allnamespaces to local
deployment, or explicitly set the package default during catalog generation. Add
a catalog test covering the default channel and both OwnNamespace and
AllNamespaces channels.
- Around line 91-102: Update getWatchNamespace to distinguish an explicitly set
empty WATCH_NAMESPACE from an unset variable: accept the former for
AllNamespaces mode, but return an error for the latter. Preserve the existing
informational logging for valid empty values and ensure manager configuration
does not treat an unset variable as cluster-wide scope.
- Around line 123-149: Update the Option A catalog design to define an explicit
upgrade edge from oadp-operator.v99.0.0 to oadp-operator.v99.0.0-allns using the
selected catalog format’s graph metadata, and revise the olm.skipRange so the
installed 99.0.0 version is included. Replace any description of switching back
as an OLM rollback with a supported recovery procedure, and document and test
that procedure.
---
Nitpick comments:
In `@docs/design/allnamespaces-install-mode_design.md`:
- Around line 71-80: Clarify the ownership of the OPERATOR_NAMESPACE environment
variable between the shared manager manifest and the AllNamespaces overlay,
specifying whether the overlay reuses or replaces the base entry. Update the
design steps to ensure each rendered Deployment contains exactly one
OPERATOR_NAMESPACE entry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 18299249-55c5-4dcf-a1a9-d6c80e7d557b
📒 Files selected for processing (1)
docs/design/allnamespaces-install-mode_design.md
| #### Changes | ||
|
|
||
| | File | Change | Lines | | ||
| |---|---|---| | ||
| | `config/manager/manager.yaml` | Add `OPERATOR_NAMESPACE` env var via downward API (`metadata.namespace`) | near 63 | | ||
| | `cmd/main.go` | Add `getOperatorNamespace()` helper, modeled on `getWatchNamespace()` | near 348 | | ||
| | `cmd/main.go` | `addPodSecurityPrivilegedLabels()` uses `operatorNamespace` instead of `watchNamespace` | 140 | | ||
| | `cmd/main.go` | `CLIDownloadSetup` / `VMDPDownloadSetup` `Namespace` and `OperatorNamespace` use `operatorNamespace` | 312-329 | | ||
| | `pkg/credentials/stsflow/stsflow.go` | Read `OPERATOR_NAMESPACE` instead of `WATCH_NAMESPACE` | 115 | | ||
| | `internal/controller/nonadmin_controller.go` | Propagate `OPERATOR_NAMESPACE` (resolves existing TODO at line 176) | 176 | | ||
| | `internal/controller/kubevirt_datamover_controller.go` | Propagate `OPERATOR_NAMESPACE` | 152 | | ||
| | `internal/controller/vmfilerestore_controller.go` | Propagate `OPERATOR_NAMESPACE` | 184 | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Include the CloudStorage STS lookup in Phase 1.
Phase 1 moves credential creation to OPERATOR_NAMESPACE, but internal/controller/cloudstorage_controller.go still reads WATCH_NAMESPACE before calling WaitForSecret. In AllNamespaces mode, that value is empty, so the reconciler cannot find the CCO-created secret in the operator namespace. Add this consumer to the change table and define its namespace source.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/allnamespaces-install-mode_design.md` around lines 69 - 80,
Update the Changes table to include
internal/controller/cloudstorage_controller.go, identifying its CloudStorage STS
lookup and WaitForSecret flow as using OPERATOR_NAMESPACE instead of
WATCH_NAMESPACE. Define that the namespace source is the operator namespace,
consistent with the Phase 1 credential-creation changes.
| ### Phase 2: Handle Empty WATCH_NAMESPACE in the Manager | ||
|
|
||
| Make the controller-runtime manager work correctly when `WATCH_NAMESPACE` is empty, which signals AllNamespaces mode. | ||
|
|
||
| #### Changes | ||
|
|
||
| | File | Change | Lines | | ||
| |---|---|---| | ||
| | `cmd/main.go` | Conditional cache config: skip `DefaultNamespaces` when `watchNamespace` is empty (cache watches all namespaces) | 204-208 | | ||
| | `cmd/main.go` | `getWatchNamespace()`: empty or unset is now valid; log info instead of error | 127-131, 348-360 | | ||
| | `cmd/main.go` | Remove the `watchNamespace == ""` skip for CLI/VMDP setup (these use `operatorNamespace` from Phase 1) | 305-306 | | ||
| | `cmd/main.go` | `addPodSecurityPrivilegedLabels`: already fixed in Phase 1 to use `operatorNamespace`; verify empty-string guard is removed | 362-368 | |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- design references to WATCH_NAMESPACE and getWatchNamespace ---'
rg -n -C 5 'WATCH_NAMESPACE|getWatchNamespace|LookupEnv|AllNamespaces|DefaultNamespaces|operatorNamespace' docs/design/allnamespaces-install-mode_design.md cmd internal config deploy 2>/dev/null || true
echo '--- candidate source files ---'
git ls-files | rg '(^|/)(main\.go|.*controller.*\.go|.*allnamespaces.*|.*install.*)' | head -200
echo '--- main.go structure and relevant definitions ---'
main_file="$(fd -t f '^main\.go$' . | head -1 || true)"
if [ -n "$main_file" ]; then
ast-grep outline "$main_file"
rg -n -C 12 'WATCH_NAMESPACE|getWatchNamespace|LookupEnv|DefaultNamespaces|watchNamespace|operatorNamespace' "$main_file"
fi
echo '--- all direct WATCH_NAMESPACE consumers ---'
rg -n -C 8 'WATCH_NAMESPACE|LookupEnv\(' . -g '*.go' -g '*.yaml' -g '*.yml' -g '*.md' 2>/dev/null || trueRepository: openshift/oadp-operator
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- relevant dependency versions ---'
rg -n 'sigs.k8s.io/controller-runtime|controller-runtime' go.mod go.sum vendor 2>/dev/null | head -40 || true
echo '--- main.go startup path ---'
sed -n '115,215p' cmd/main.go
sed -n '300,350p' cmd/main.go
echo '--- all Go WATCH_NAMESPACE references only ---'
rg -n -C 4 'WATCH_NAMESPACE|LookupEnv\(' --glob '*.go' . | head -240
echo '--- design wording around Phase 1 and Phase 2 ---'
sed -n '64,125p' docs/design/allnamespaces-install-mode_design.md
echo '--- tests for namespace helpers and startup behavior ---'
rg -n -C 5 'getWatchNamespace|addPodSecurityPrivilegedLabels|DefaultNamespaces|watchNamespace' --glob '*_test.go' cmd internal pkg . | head -240Repository: openshift/oadp-operator
Length of output: 15220
🌐 Web query:
controller-runtime cache.Options DefaultNamespaces empty string key all namespaces
💡 Result:
In controller-runtime, using an empty string ("") as a key within the cache.Options configuration—specifically in ByObject.Namespaces or DefaultNamespaces—is a valid approach for defining cache settings that apply to namespaces without an explicit configuration [1]. This functionality was introduced to support scenarios where an empty string is used as a placeholder, such as when a namespace value is derived from a CLI flag that might be empty [1]. When you use an empty string as a key in these maps, it acts as a specific configuration target rather than a wildcard for all namespaces [1]. To clarify common points of confusion: 1. All Namespaces: If your intention is to cache all namespaces, you should use the constant cache.AllNamespaces (often represented as an empty string value in Kubernetes' metav1.NamespaceAll) as the key in DefaultNamespaces [2][3][4]. However, note that because controller-runtime defaults to caching all namespaces, adding cache.AllNamespaces to DefaultNamespaces is often redundant unless you are attaching specific label or field selectors to that configuration [5][3]. 2. Namespace Precedence: The cache uses a specific precedence order for defaulting configurations [3][4]: - First: ByObject settings - Second: DefaultNamespaces[namespace] settings - Third: Default* settings Using an empty string key does not trigger "all namespaces" behavior; instead, it allows you to define a catch-all configuration for namespaces that are otherwise unconfigured, or to handle empty-string inputs programmatically [1]. If you encounter issues where empty-string keys do not behave as "all namespaces," ensure you are using the cache.AllNamespaces constant to explicitly target that scope [5][4].
Citations:
- 1: ✨ Cache: Allow defining options that apply to all namespaces that themselves have no explicit config kubernetes-sigs/controller-runtime#2528
- 2: https://github.com/kubernetes-sigs/controller-runtime/blob/main/designs/cache_options.md
- 3: https://github.com/kubernetes-sigs/controller-runtime/blob/4cf9db06e3b06be9c57ced5d8b705339b816765d/pkg/cache/cache.go
- 4: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.24.0/pkg/cache/cache.go
- 5: DefaultNamespaces Cache Option doesn't allow All Namespaces for List kubernetes-sigs/controller-runtime#2628
🌐 Web query:
sigs.k8s.io/controller-runtime cache multiNamespaceCache empty namespace DefaultNamespaces source
💡 Result:
In the context of the sigs.k8s.io/controller-runtime library, the handling of empty strings as namespaces, especially regarding the legacy MultiNamespacedCache and the modern DefaultNamespaces configuration, is as follows: MultiNamespacedCache (Legacy) The MultiNamespacedCacheBuilder was historically used to restrict a manager's cache to a specific list of namespaces [1][2]. In early versions, providing an empty string ("") as a namespace in this list was often problematic or misunderstood [3][2]. It was intended to either cause errors or produce unexpected behavior (such as duplication of objects) because it did not correctly map to "all namespaces" or "cluster-scoped resources" in the way some users expected [3][4][2]. The library was later updated to improve support for cluster-scoped resources by treating them separately from namespaced ones, often involving an internal mapping for the empty namespace to a global cache to avoid "unknown namespace" errors [5][4]. Users are generally advised to avoid passing an empty string to the legacy MultiNamespacedCache builder, as it is not the intended way to configure cache scope [2]. DefaultNamespaces (Modern Configuration) In the modern cache configuration (cache.Options), DefaultNamespaces is a map used to define cache settings for specific namespaces [6][7]. 1. Behavior: If you specify DefaultNamespaces, the cache will only watch the namespaces explicitly included in that map [8][9]. 2. Empty String Key: Recent updates have allowed the use of the empty string ("") as a key within the DefaultNamespaces map [10]. This allows users to define a "default" cache configuration that applies to all namespaces that do not have an explicit, specific configuration assigned to them [10]. 3. AllNamespaces Const: To watch all namespaces, you should typically not use the MultiNamespacedCache builder [2]. Instead, the default behavior of the controller-runtime Manager is to watch all namespaces if no restrictive cache options are provided [2][8]. You can also use the cache.AllNamespaces constant as a key within cache configurations to explicitly include all namespaces [6][7]. In summary, while older versions of the library struggled with empty string namespaces in the legacy MultiNamespacedCache, modern controller-runtime versions (via cache.Options) support the use of an empty string key in DefaultNamespaces to serve as a fallback configuration for namespaces lacking specific rules [10]. To watch all namespaces, standard manager behavior or the AllNamespaces constant should be used instead of manual namespace list building [2][6].
Citations:
- 1: https://github.com/kubernetes-sigs/controller-runtime/blob/f236f0345ad2933912ebf34bfcf0f93620769654/pkg/cache/multi_namespace_cache.go
- 2: MultiNamespacedCacheBuilder support namespace list with empty string kubernetes-sigs/controller-runtime#1006
- 3: MultiNamespacedCacheBuilder duplicates objects if provided empty string "" along with namespaces kubernetes-sigs/controller-runtime#1869
- 4: 🐛 Modify multinamespaced cache to support cluster scoped resources kubernetes-sigs/controller-runtime#1418
- 5: Unknown namespace for the cache error while Getting cluster scoped for MultiNamespacedCache kubernetes-sigs/controller-runtime#1378
- 6: https://github.com/kubernetes-sigs/controller-runtime/blob/main/designs/cache_options.md
- 7: https://github.com/kubernetes-sigs/controller-runtime/blob/4cf9db06e3b06be9c57ced5d8b705339b816765d/pkg/cache/cache.go
- 8: https://sdk.operatorframework.io/docs/building-operators/golang/operator-scope/
- 9: https://master.sdk.operatorframework.io/docs/building-operators/golang/operator-scope/
- 10: ✨ Cache: Allow defining options that apply to all namespaces that themselves have no explicit config kubernetes-sigs/controller-runtime#2528
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import os
import re
from pathlib import Path
source = Path("cmd/main.go").read_text()
body = re.search(
r'func getWatchNamespace\(\) \(string, error\) \{(?P<body>.*?)\n\}',
source,
re.S,
)
if not body:
raise SystemExit("getWatchNamespace not found")
snippet = body.group("body")
has_lookup = "os.LookupEnv" in snippet
returns_error_on_missing = bool(re.search(r'if\s+!found\s*\{.*?return\s+"",\s*fmt\.Errorf', snippet, re.S))
returns_present_value = "return ns, nil" in snippet
def lookup(env, name="WATCH_NAMESPACE"):
return (env[name], None) if name in env else ("", "missing")
cases = [
("unset", {}),
("set to empty", {"WATCH_NAMESPACE": ""}),
("set to namespace", {"WATCH_NAMESPACE": "openshift-adp"}),
]
print("source_uses_os.LookupEnv:", has_lookup)
print("source_errors_when_absent:", returns_error_on_missing)
print("source_returns_present_value:", returns_present_value)
for label, env in cases:
value, error = lookup(env)
print(f"{label}: value={value!r}, error={error!r}")
if not (has_lookup and returns_error_on_missing and returns_present_value):
raise SystemExit("current helper does not preserve the LookupEnv distinction")
PY
echo '--- controller-runtime v0.19.3 cache source ---'
curl -fsSL \
https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/v0.19.3/pkg/cache/cache.go \
| rg -n -C 5 'DefaultNamespaces|AllNamespaces|default.*namespace|multiNamespace' | head -180Repository: openshift/oadp-operator
Length of output: 7330
Keep an unset WATCH_NAMESPACE invalid.
An empty value explicitly selects AllNamespaces mode. An unset variable must return an error. Otherwise, both states become "", and the manager selects the cluster-wide cache scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/allnamespaces-install-mode_design.md` around lines 91 - 102,
Update getWatchNamespace to distinguish an explicitly set empty WATCH_NAMESPACE
from an unset variable: accept the former for AllNamespaces mode, but return an
error for the latter. Preserve the existing informational logging for valid
empty values and ensure manager configuration does not treat an unset variable
as cluster-wide scope.
| | **Makefile** | Add `INSTALL_MODE ?= OwnNamespace`. New targets: `bundle-allnamespaces`, `bundle-build-allnamespaces`, `catalog-build-allnamespaces` | | ||
| | **Catalog build** | Parameterize `Dockerfile.catalog` and `catalog-build` target to produce a catalog with two channels: existing channel (OwnNamespace bundle) and new `-allnamespaces` channel (AllNamespaces bundle) | | ||
| | **CSV naming** | AllNamespaces CSV uses a distinct version suffix: `oadp-operator.v99.0.0-allns` vs `oadp-operator.v99.0.0` | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- design document ---'
sed -n '1,220p' docs/design/allnamespaces-install-mode_design.md
printf '%s\n' '--- catalog and channel references ---'
rg -n --hidden -S 'defaultChannelName|channelName|dev|stable|allnamespaces|all-namespaces|bundle-allnamespaces|catalog-build' \
. ':!**/.git/**' | head -n 240Repository: openshift/oadp-operator
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- bundle metadata and Dockerfiles ---'
for f in bundle/metadata/annotations.yaml build/Dockerfile.bundle bundle.Dockerfile build/Dockerfile.catalog; do
if [ -f "$f" ]; then
echo "### $f"
cat -n "$f"
fi
done
printf '%s\n' '--- Makefile build variables and catalog targets ---'
sed -n '1,45p;285,345p' Makefile
printf '%s\n' '--- design references to default/channel/package ---'
rg -n -i 'default(channel|Channel)|channel|package|catalog' docs/design/allnamespaces-install-mode_design.md
printf '%s\n' '--- package metadata in repository ---'
rg -n -i 'packageName|package name|bundle\.package|bundle\.channels|bundle\.channel\.default|defaultChannelName|PackageManifest' \
--glob '*.yaml' --glob '*.yml' --glob '*.md' --glob 'Makefile' . | head -n 160Repository: openshift/oadp-operator
Length of output: 18270
🌐 Web query:
Operator Lifecycle Manager package defaultChannelName bundle annotations default channel specification
💡 Result:
In the Operator Lifecycle Manager (OLM), the default channel for an operator package is specified differently depending on whether you are using traditional bundle-based catalogs or file-based catalogs (FBC). For bundle-based catalogs, the default channel is specified using the annotation operators.operatorframework.io.bundle.channel.default.v1 in the bundle's annotations.yaml file [1][2]. This annotation identifies the default channel to which an operator should be subscribed when installed from a registry [1][3]. It is optional if the default channel has already been set by previous bundles and remains unchanged for the current bundle [3][4]. If not specified, OLM may determine the default channel based on the lexical (alphabetical) ordering of the available channels [5]. For file-based catalogs (FBC), the default channel is explicitly defined in the olm.package object within the catalog's metadata [6][7]. The olm.package schema includes a defaultChannel field, which must contain the name of the designated default channel for that package [6][8]. The default channel is critical for OLM's operation, as it is used when a Subscription does not explicitly specify a channel [5][9]. Additionally, during dependency resolution, OLM prefers the default channel over others; if the default channel cannot satisfy a dependency, OLM considers the remaining channels in lexicographical order [5][10]. If a package contains only a single channel, that channel is implicitly treated as the default [11][9].
Citations:
- 1: https://olm.operatorframework.io/docs/tasks/creating-operator-bundle/
- 2: https://docs.okd.io/4.18/operators/understanding/olm-packaging-format.html
- 3: https://github.com/operator-framework/operator-registry/blob/master/docs/design/operator-bundle.md
- 4: operators.operatorframework.io.bundle.channel.default.v1 annotation ought to be OPTIONAL operator-framework/operator-sdk#5569
- 5: https://olm.operatorframework.io/docs/best-practices/channel-naming/
- 6: https://olm.operatorframework.io/docs/reference/file-based-catalogs/
- 7: https://olm.operatorframework.io/docs/reference/catalog-templates/
- 8: https://docs.okd.io/latest/operators/understanding/olm-packaging-format.html
- 9: https://docs.okd.io/4.15/rest_api/operatorhub_apis/packagemanifest-packages-operators-coreos-com-v1.html
- 10: https://olm.operatorframework.io/docs/concepts/olm-architecture/dependency-resolution/
- 11: https://pkg.go.dev/github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/apis/operators
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- deployment and migration sections ---'
sed -n '220,315p' docs/design/allnamespaces-install-mode_design.md
printf '%s\n' '--- all default-channel assignments in design ---'
rg -n -C 3 'DEFAULT_CHANNEL|default channel|defaultChannel|bundle\.channel\.default' \
docs/design/allnamespaces-install-mode_design.md Makefile bundle/metadata/annotations.yaml build/Dockerfile.bundle bundle.Dockerfile
printf '%s\n' '--- generated-bundle commands ---'
rg -n -C 4 'operator-sdk.*bundle|BUNDLE_METADATA_OPTS|DEFAULT_CHANNEL|CHANNELS' Makefile \
docs/developer docs/design --glob '*.md' | head -n 220Repository: openshift/oadp-operator
Length of output: 13430
Preserve the OwnNamespace package default channel.
For Option A, keep the published oadp-operator package default on dev or the existing release channel. Use operators.operatorframework.io.bundle.channel.default.v1, not defaultChannelName. Scope DEFAULT_CHANNEL=dev-allnamespaces to local deployment only, or set the package default explicitly during catalog generation. Add a catalog test that checks the default channel and both channels.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/allnamespaces-install-mode_design.md` around lines 147 - 149, The
catalog generation changes must preserve the existing OwnNamespace package as
the published default channel. Use
operators.operatorframework.io.bundle.channel.default.v1 rather than
defaultChannelName, restrict DEFAULT_CHANNEL=dev-allnamespaces to local
deployment, or explicitly set the package default during catalog generation. Add
a catalog test covering the default channel and both OwnNamespace and
AllNamespaces channels.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/design/allnamespaces-install-mode_design.md (1)
231-235: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd migration coverage for Option B.
The test plan adds migration coverage only for Option A. If Option B is selected, test subscription and CSV deletion, resource retention, OperatorGroup replacement, CSV success, and DPA reconciliation after reinstall. The current Option B procedure remains unvalidated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/allnamespaces-install-mode_design.md` around lines 231 - 235, Extend the migration test plan to cover Option B in addition to Option A. Validate deleting the Subscription and CSV, confirming resources are retained, replacing the OperatorGroup, reinstalling until the CSV succeeds, and verifying DPA reconciliation afterward; keep the existing Option A migration coverage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/allnamespaces-install-mode_design.md`:
- Around line 175-183: Update the Option B catalog tree code fence in the design
document to specify the text language identifier, changing the opening fence to
```text while preserving the tree content and closing fence.
- Around line 122-124: Revise the design section around producing two OLM
bundles to clarify that two CSVs are a deliberate design choice, not an OLM
requirement. State that a single CSV may declare both OwnNamespace and
AllNamespaces as supported, with the active OperatorGroup selecting the scope;
retain two CSVs only when explicit topology and independent delivery justify
them.
---
Nitpick comments:
In `@docs/design/allnamespaces-install-mode_design.md`:
- Around line 231-235: Extend the migration test plan to cover Option B in
addition to Option A. Validate deleting the Subscription and CSV, confirming
resources are retained, replacing the OperatorGroup, reinstalling until the CSV
succeeds, and verifying DPA reconciliation afterward; keep the existing Option A
migration coverage unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 50ffc689-2705-4d78-a7df-a9fefa118ab3
📒 Files selected for processing (1)
docs/design/allnamespaces-install-mode_design.md
|
Perhaps call it OADP 2.0 since there is significant operational mode update. |
WATCH_NAMESPACE sourced from metadata.namespace (not olm.targetNamespaces) means the operator keeps watching only its own namespace regardless of install mode. No Go code changes needed. Moves OPERATOR_NAMESPACE decoupling and cluster-wide watching to Future Enhancements. Reduces plan from 6 phases to 5. Phase 1 is now just the CSV change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/allnamespaces-install-mode_design.md`:
- Around line 83-87: Keep
config/manifests/bases/oadp-operator.clusterserviceversion.yaml unchanged as the
OwnNamespace source, and move the AllNamespaces installModes and WATCH_NAMESPACE
changes into a separate overlay or derived bundle consumed only for the
AllNamespaces variant. Update the bundle generation flow so make bundle does not
modify the existing OwnNamespace CSV.
- Around line 443-444: Update the security discussion around the operator’s
runtime behavior to separate WATCH_NAMESPACE and OperatorGroup scope from CSV
RBAC scope. Remove the claim that OwnNamespace contains or limits the velero
ServiceAccount wildcard permissions, and assess the CSV clusterPermissions and
ClusterRoleBinding independently for both installation modes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 34d13fe8-09ce-45e0-bede-350bbbab2c8d
📒 Files selected for processing (1)
docs/design/allnamespaces-install-mode_design.md
Phase 1 (CSV change) cannot be merged alone — it would modify the existing OwnNamespace CSV. The installModes flip and WATCH_NAMESPACE source change must live inside the AllNamespaces kustomize overlay, which is part of the build infrastructure work. Merges old Phases 1+2 into a single Phase 1. Renumbers to 4 phases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/allnamespaces-install-mode_design.md`:
- Around line 197-201: Update ValidateDataProtectionCR to enforce a single
DataProtectionApplication across all namespaces, while explicitly defining the
behavior for DPAs outside openshift-adp. Do not use ClusterWideClient for
singleton enforcement; add E2E coverage for cross-namespace duplicates and
migration rollback.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: afa04e92-51da-4760-b526-f41fc0d2a1aa
📒 Files selected for processing (1)
docs/design/allnamespaces-install-mode_design.md
| | `Makefile` `deploy-olm` (line 634-642) | Parameterize OperatorGroup creation: when `INSTALL_MODE=AllNamespaces`, create OperatorGroup with empty `spec` (no `targetNamespaces`). Default (`OwnNamespace`) keeps current behavior. | | ||
| | `Makefile` | New target: `deploy-olm-allnamespaces`. For Option A (channels): sets `INSTALL_MODE=AllNamespaces` and `DEFAULT_CHANNEL=dev-allnamespaces`. For Option B (packages): sets `INSTALL_MODE=AllNamespaces` and overrides subscription package name. | | ||
| | `tests/e2e/upgrade_suite_test.go` (lines 31-50) | Parameterize OperatorGroup creation to support both modes based on a test flag | | ||
| | E2E test scenarios | Add: install AllNamespaces, create DPA in operator namespace, verify Velero deploys. Verify `WATCH_NAMESPACE` resolves to operator namespace. Verify singleton enforcement. Verify sub-controller namespace config. | | ||
| | Migration test (Option A only) | Test switching channel on an existing Subscription and swapping the OperatorGroup to verify the documented migration path works end-to-end. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C 5 'DPA|singleton|one per namespace|global' \
docs/design/allnamespaces-install-mode_design.md internal tests/e2eRepository: openshift/oadp-operator
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- design sections ---'
sed -n '1,110p' docs/design/allnamespaces-install-mode_design.md
sed -n '450,490p' docs/design/allnamespaces-install-mode_design.md
printf '%s\n' '--- DPA-related implementation symbols ---'
rg -n -i 'singleton|one.*DPA|multiple.*DPA|only.*DPA|dpa.*already|already.*dpa|ClusterWideClient|List\(.*DataProtection|DataProtectionApplicationList|DataProtectionApplication' \
internal cmd api config tests/e2e \
-g '*.go' -g '*.yaml' -g '*.md' | head -n 240
printf '%s\n' '--- DPA reconciler outline ---'
ast-grep outline internal/controller/dataprotectionapplication_controller.go --match 'DataProtectionApplicationReconciler' --view expandedRepository: openshift/oadp-operator
Length of output: 34363
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact design claims ---'
rg -n -C 3 -i 'singleton|one DPA|per namespace|global DPA|outside|openshift-adp|duplicate|rollback' \
docs/design/allnamespaces-install-mode_design.md
printf '%s\n' '--- controller validation and DPA listing ---'
rg -n -C 8 -i 'singleton|multiple|already exists|ListOptions|DataProtectionApplicationList|ClusterWideClient|dpa.*namespace|namespace.*dpa' \
internal/controller cmd api tests \
-g '*.go' | head -n 320
printf '%s\n' '--- DPA controller setup and watches ---'
rg -n -C 6 'SetupWithManager|For\(.*DataProtectionApplication|Owns\(' \
internal/controller -g '*.go'Repository: openshift/oadp-operator
Length of output: 34481
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- DPA reconciler implementation ---'
sed -n '40,165p' internal/controller/dataprotectionapplication_controller.go
printf '%s\n' '--- cross-namespace validator implementation ---'
rg -n -C 12 'ClusterWideClient|Validate.*DPA|DataProtectionApplication' internal/controller/validator.go internal/controller -g '*.go' | head -n 260
printf '%s\n' '--- DPA list operations outside the namespaced client ---'
rg -n -C 8 '\.List\(.*DataProtectionApplication|DataProtectionApplicationList|ListAll|ClusterWideClient' internal cmd -g '*.go'Repository: openshift/oadp-operator
Length of output: 37273
Enforce the global DPA singleton.
ValidateDataProtectionCR currently rejects multiple DPAs only within the same namespace. ClusterWideClient is used for Non-Admin validation, not DPA singleton enforcement. Define the behavior for DPAs outside openshift-adp and add cross-namespace duplicate and migration-rollback E2E cases.
🧰 Tools
🪛 LanguageTool
[style] ~200-~200: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...amespace. Verify singleton enforcement. Verify sub-controller namespace config. | | Mi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/allnamespaces-install-mode_design.md` around lines 197 - 201,
Update ValidateDataProtectionCR to enforce a single DataProtectionApplication
across all namespaces, while explicitly defining the behavior for DPAs outside
openshift-adp. Do not use ClusterWideClient for singleton enforcement; add E2E
coverage for cross-namespace duplicates and migration rollback.
Removes the two-packages option. Documents why both channels must coexist for at least one release: without it, existing OwnNamespace customers hit an OLM deadlock where they can't upgrade (new CSV doesn't support their OperatorGroup) and can't change the OperatorGroup (current CSV doesn't support AllNamespaces either). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/allnamespaces-install-mode_design.md (1)
198-204: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the actual OperatorGroup name.
Step 1 discovers the existing OperatorGroup, but the delete and recreate commands assume
oadp-operator-group. Capturemetadata.nameand reuse it, or derive it in the commands. Otherwise, the migration fails when the existing OperatorGroup has another name.Also applies to: 217-235
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/allnamespaces-install-mode_design.md` around lines 198 - 204, The migration instructions must reuse the actual OperatorGroup name discovered in Step 1 instead of assuming oadp-operator-group. Update the delete and recreate commands in the affected steps to capture or derive metadata.name from the existing OperatorGroup, then use that value consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/design/allnamespaces-install-mode_design.md`:
- Around line 198-204: The migration instructions must reuse the actual
OperatorGroup name discovered in Step 1 instead of assuming oadp-operator-group.
Update the delete and recreate commands in the affected steps to capture or
derive metadata.name from the existing OperatorGroup, then use that value
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e5c7f788-d0ef-4b6c-aab8-22cddea761ed
📒 Files selected for processing (1)
docs/design/allnamespaces-install-mode_design.md
Phase 2 now adds deploy-olm-allnamespaces target (using operator-sdk run bundle --install-mode AllNamespaces) and a Prow job that runs the existing e2e suite against it. This gives immediate signal before writing any AllNamespaces-specific test code. Phase 3 becomes AllNamespaces-specific test scenarios (migration, upgrade parameterization). Adds Phase 5 for OwnNamespace deprecation planning. Updates current state table to reflect operator-sdk run bundle as the deploy mechanism. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
I'm not sure if it is that significant? But in terms of having customers move to this new InstallMode which is a breaking change....maybe calling it 2.0 isn't a bad idea. Not sure who I should talk to about this. Will add it to the design doc. |
- Add cross-channel update graph requirement to Phase 1: AllNamespaces CSV must include olm.skipRange covering OwnNamespace versions so OLM can resolve the channel switch - Fix security section: ClusterRoleBindings are cluster-scoped regardless of OperatorGroup mode, OwnNamespace does not contain velero SA perms - Add language identifier to catalog tree code fence (markdownlint MD040) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cluster testing confirmed that both OwnNamespace and AllNamespaces install modes can be enabled in the same CSV. The two-channel approach is no longer needed. Three CSV metadata changes replace the entire multi-channel plan: 1. Flip AllNamespaces to supported: true 2. Source WATCH_NAMESPACE from metadata.namespace (not olm.targetNamespaces) 3. Add permissions entries for non-admin-controller and velero SAs Reduces plan from 5 phases to 3. Links to HackMD test log from OpenShift 4.22 cluster validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Key improvements based on design review and OLM edge case research: - Added OLM behavioral differences section: CSV copies on large clusters, CRD ownership constraints, dual-installation prevention - Added OLMv1 considerations: AllNamespaces is the strategic direction, OLMv1 GA required it, OwnNamespace came later as compat feature - Added fresh AllNamespaces install path via OperatorHub (Console auto-creates namespace + OperatorGroup) - Clarified that velero SA permissions are placeholder rules for OLM, not actual leader election - Added OLM ensureSingletonRBAC behavior: permissions promoted to ClusterRoles in AllNamespaces mode - Noted operator-sdk substitution is hardcoded and cannot be disabled - Added alternative: handle empty WATCH_NAMESPACE in Go (rejected — requires code changes) - Added security note for new namespace-scoped permissions entries - Added minimum OpenShift version note - Updated test log link to full dual-mode validation - Removed Current State table (implementation detail, not design) - Simplified migration to essential steps only Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds dedicated test phase between CI setup and migration docs: - AllNamespaces fresh install validation - OwnNamespace → AllNamespaces migration test - AllNamespaces → OwnNamespace rollback test - Upgrade from OwnNamespace-only to dual-mode CSV - Upgrade test parameterization for global OperatorGroup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
not quite.. |
|
@Joeavaikath: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
Summary
AllNamespacesinstall mode on the OADP operatorOwnNamespaceandAllNamespacesenabled in the same CSVThree CSV Changes
installModes: enableAllNamespaces: truealongsideOwnNamespace: trueWATCH_NAMESPACEsource: change fromolm.targetNamespacestometadata.namespace(backward-compatible — both resolve to the same value under OwnNamespace)permissionsentries: add namespace-scoped Roles fornon-admin-controllerandveleroSAs (OLM requires these in AllNamespaces mode)Key Design Decisions
metadata.namespaceoverolm.targetNamespaces: resolves to the pod's namespace in both modes. Zero behavioral change for existing installs.permissionsfor all SAs inclusterPermissions: discovered during cluster testing. The velero SA permissions are placeholders for OLM SA creation, not actual leader election.Cluster Test Results
openshift-adpTest plan
Posted via Claude Code