OSAC-23: Extract storage lifecycle into OSAC Storage Controller - #299
openshift-merge-bot[bot] merged 15 commits into
Conversation
|
@zszabo-rh: This pull request references OSAC-23 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 epic 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. |
|
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:
WalkthroughExtracts storage provisioning from ChangesStorage Controller Extraction and Tenant API Expansion
Sequence DiagramsequenceDiagram
participant Operator as Operator (main)
participant TenantReconciler as TenantReconciler
participant StorageReconciler as StorageReconciler
participant BackendProvider as AAP BackendProvider
participant ClusterStorageProvider as AAP ClusterStorageProvider
participant TenantStatus as Tenant CR Status
Operator->>TenantReconciler: reconcile Tenant
TenantReconciler->>TenantStatus: ensure finalizer, check namespace
TenantReconciler->>TenantStatus: set NamespaceReady=True, phase=Ready
Operator->>StorageReconciler: reconcile Tenant (phase=Ready)
StorageReconciler->>StorageReconciler: ensure storage finalizer
StorageReconciler->>StorageReconciler: hubSecretExists?
alt hub Secret missing
StorageReconciler->>BackendProvider: TriggerProvision
BackendProvider-->>StorageReconciler: jobID
StorageReconciler->>TenantStatus: append StorageBackendJobs, StorageBackendReady=False
else hub Secret found
StorageReconciler->>TenantStatus: set StorageBackendReady=True
StorageReconciler->>StorageReconciler: getTenantStorageClasses
StorageReconciler->>ClusterStorageProvider: TriggerProvision
ClusterStorageProvider-->>StorageReconciler: jobID
StorageReconciler->>TenantStatus: append ClusterStorageJobs, ClusterStorageReady=True
end
StorageReconciler->>TenantStatus: patch if changed
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/tenant_controller.go (1)
154-170:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftKeep the finalizer until target namespace and UDN cleanup completes.
The delete path removes
tenantFinalizerimmediately, but this controller still owns tenant namespace/UDN lifecycle. That can orphan target-cluster isolation resources after the Tenant CR disappears. Delete or confirm removal of the target Namespace and UserDefinedNetwork before dropping the finalizer.As per coding guidelines,
Tenant — namespace and OVN-Kubernetes UserDefinedNetwork for isolation.🤖 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 `@internal/controller/tenant_controller.go` around lines 154 - 170, The handleDelete method in TenantReconciler removes the tenantFinalizer prematurely before cleaning up the associated target namespace and UserDefinedNetwork resources that the controller owns, which can orphan isolation resources. Modify the handleDelete method to add cleanup logic that deletes or confirms removal of the target Namespace and UserDefinedNetwork resources before calling controllerutil.RemoveFinalizer on the tenantFinalizer. Only return success after both the namespace and UDN cleanup operations have completed successfully.Source: Coding guidelines
🤖 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 `@config/rbac/role.yaml`:
- Around line 20-27: The current ClusterRole is granting cluster-wide read and
watch access to secrets, violating the principle of least privilege. Remove the
apiGroups/resources/verbs block that grants get, list, and watch access to
secrets from this cluster-wide manager role. Create a separate namespaced Role
(not ClusterRole) in the storage config namespace that contains only the secrets
access rules with the same get, list, and watch verbs. Then create a
corresponding RoleBinding in that storage namespace that binds this Role to the
appropriate service account, ensuring the storage controller only has scoped
access to secrets within its own namespace rather than across the entire
cluster.
In `@internal/controller/storage_controller.go`:
- Around line 634-642: The needsProvisionJob function returns true when
latestJob.State.IsSuccessful() is true, which causes redundant provision jobs to
be triggered if the expected resource hasn't appeared yet. Modify the
needsProvisionJob function to implement a backoff mechanism by checking a
timestamp-based condition on successful jobs, or add additional state tracking
(such as a LastProvisionTime or AwaitingResource indicator) to prevent immediate
re-triggering of provision jobs after a successful job completion. This will
avoid multiple redundant AAP jobs when there's a delay in external resource
creation.
- Around line 704-706: The mapClusterOrderToTenant method in the
StorageReconciler always returns nil, which prevents ClusterOrder changes from
triggering Tenant reconciliation. Either add a TODO comment above the method
explaining why the mapping is not yet implemented (if this is placeholder code
for future use), or remove the ClusterOrder watch registration entirely if it is
no longer needed. Determine the intent of this watcher and take the appropriate
action to either document the incomplete implementation or clean up the unused
watch.
In `@internal/controller/tenant_controller_test.go`:
- Around line 67-72: The AfterEach function currently only deletes the Tenant
resource but does not clean up the Namespace that was created in BeforeEach. Add
explicit cleanup for the Namespace in the AfterEach block by retrieving the
namespace object (similar to how the tenant is retrieved) and deleting it if it
exists, ensuring tests are fully self-contained and prevent ordering issues from
test interdependencies.
- Around line 104-108: The test at lines 104-108 currently only asserts that
StorageClasses and Jobs are nil after getting the tenant, but per the PR
changes, TenantStatus now includes StorageBackends and ClusterStorage arrays.
Add two additional Expect assertions (following the same pattern as the existing
StorageClasses and Jobs assertions) to verify that tenant.Status.StorageBackends
and tenant.Status.ClusterStorage are also nil or empty, ensuring complete
coverage of all storage-related status fields remaining untouched.
In `@internal/controller/tenant_controller.go`:
- Around line 46-48: The TenantReconciler's handleUpdate method currently only
retrieves the target namespace and immediately marks the Tenant as Ready without
actually reconciling the namespace and UDN (User Defined Network) resources
required for tenant isolation. This causes new tenants to never have their
namespace/UDN created, and allows status updates from the StorageReconciler to
override the aggregate readiness state. Modify the reconciliation logic within
handleUpdate (around lines 127-150) to actively reconcile both the target
Namespace and the associated OVN-Kubernetes UserDefinedNetwork, and ensure the
Tenant's Ready status is derived from all required readiness conditions
(including StorageBackendReady and ClusterStorageReady) rather than just
checking namespace existence. This prevents downstream ComputeInstances from
proceeding without storage being fully ready.
---
Outside diff comments:
In `@internal/controller/tenant_controller.go`:
- Around line 154-170: The handleDelete method in TenantReconciler removes the
tenantFinalizer prematurely before cleaning up the associated target namespace
and UserDefinedNetwork resources that the controller owns, which can orphan
isolation resources. Modify the handleDelete method to add cleanup logic that
deletes or confirms removal of the target Namespace and UserDefinedNetwork
resources before calling controllerutil.RemoveFinalizer on the tenantFinalizer.
Only return success after both the namespace and UDN cleanup operations have
completed successfully.
🪄 Autofix (Beta)
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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ed74f7b0-d449-464e-afbd-4009c83acde4
📒 Files selected for processing (23)
api/v1alpha1/job_types.goapi/v1alpha1/tenant_types.goapi/v1alpha1/zz_generated.deepcopy.gocmd/main.goconfig/crd/bases/osac.openshift.io_clusterorders.yamlconfig/crd/bases/osac.openshift.io_computeinstances.yamlconfig/crd/bases/osac.openshift.io_publicipattachments.yamlconfig/crd/bases/osac.openshift.io_publicippools.yamlconfig/crd/bases/osac.openshift.io_publicips.yamlconfig/crd/bases/osac.openshift.io_securitygroups.yamlconfig/crd/bases/osac.openshift.io_subnets.yamlconfig/crd/bases/osac.openshift.io_tenants.yamlconfig/crd/bases/osac.openshift.io_virtualnetworks.yamlconfig/rbac/role.yamlinternal/controller/computeinstance_controller.gointernal/controller/computeinstance_controller_test.gointernal/controller/storage_controller.gointernal/controller/storage_tier_resolution.gointernal/controller/tenant_controller.gointernal/controller/tenant_controller_test.gointernal/controller/tenant_names.gopkg/aap/client.gopkg/provisioning/aap_provider.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
|
||
| // --- Stage 2: Class provisioning --- | ||
|
|
||
| func (r *StorageReconciler) handleClassProvisioning(ctx context.Context, instance *v1alpha1.Tenant) (ctrl.Result, error) { |
There was a problem hiding this comment.
Nit: handleClassProvisioning / handleClassDeprovisioning / pollClassProvisionJob use "Class" terminology, but everything else in this PR uses "ClusterStorage" (condition ClusterStorageReady, job type JobTypeClusterStorageProvision, provider field ClusterStorageProvider, env vars OSAC_STORAGE_CLUSTER_AAP_*).
Looks like a leftover from before the design alignment commit renamed StorageClassReady to ClusterStorageReady. Consider renaming to handleClusterStorageProvisioning / handleClusterStorageDeprovisioning / pollClusterStorageProvisionJob for consistency.
There was a problem hiding this comment.
You are absolutely right about this, fixing!
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
Can we scope this with a namespaced Role + RoleBinding in osac-system instead of adding Secrets to the cluster-wide ClusterRole? That way the operator can only read Secrets in the namespace it actually needs, without changing the pod or service account.
Something like a Role in osac-system granting secrets get/list/watch, plus a RoleBinding pointing to the existing controller-manager ServiceAccount. The rest of the ClusterRole stays as-is.
WDYT?
There was a problem hiding this comment.
Good call, adding a namespaced Role + RoleBinding in osac-system for Secrets access and removing it from the ClusterRole.
|
@zszabo-rh The storage lifecycle logic moved from the tenant controller to |
|
@zszabo-rh I didn't realize that we would need new job types for storage. This adds 4 storage-specific values ( The root cause is that two independent lifecycles (backend and cluster storage) share the Tenant CR, so they need different job types to distinguish their operations. A couple of alternatives worth discussing:
Given that v0.1 needs to support CaaS, what's the right decomposition here? WDYT? |
| return false | ||
| } | ||
| return latestJob.State.IsSuccessful() | ||
| } |
There was a problem hiding this comment.
The local needsProvisionJob re-triggers an AAP job when the previous one succeeded. But the failure case is already handled before this function is called (lines 287-291), so the default provisioning.NeedsProvisionJob from job_helpers.go would work here:
- No job: triggers first job (same behavior)
- Running: falls through to poll (same behavior)
- Failed: never reaches this function, handled at line 287 (same behavior)
- Succeeded: default returns false, falls through to
pollBackendProvisionJob, which requeues after 5s. On the next reconcile,hubSecretExists()checks if the Secret appeared. No redundant AAP jobs.
Is there a reason the local function was needed instead of the shared one? WDYT?
There was a problem hiding this comment.
Yes, seems I've badly overlooked this one.. Switching to provisioning.NeedsProvisionJob!
|
|
||
| func storageClassTenantPredicate() predicate.Predicate { | ||
| return predicate.NewPredicateFuncs(func(obj client.Object) bool { | ||
| _, exists := obj.GetLabels()[osacTenantAnnotation] |
There was a problem hiding this comment.
Nit: osacTenantAnnotation is used as a label key everywhere (client.MatchingLabels, obj.GetLabels()), not as an annotation. Should this be renamed to osacTenantLabel?
There was a problem hiding this comment.
This one is coming from upstream, but I agree, renaming to osacTenantLabel.
You're right, the tests should have been moved with the code. Adding unit tests for the StorageReconciler in this PR, following the test plan from the design spec. Will push shortly. |
|
@akshaynadkarni about the new job types: I've analyzed alternatives and want to discuss two options with you. Sending details via DM, short version:
Both would eliminate the enum pollution. |
| aapInsecureSkipVerify := helpers.GetEnvWithDefault(envAAPInsecureSkipVerify, false) | ||
|
|
||
| backendProvisionTemplate := helpers.GetEnvWithDefault( | ||
| envStorageBackendProvisionTemplate, "osac-create-tenant-storage-backend") |
There was a problem hiding this comment.
what does the default template do? can there be a default which will bring us to a functional state for storage?
There was a problem hiding this comment.
No, the default doesn't bring you to a functional state on its own, it's just a convention that avoids extra env vars when using the standard AAP deployment (same pattern as all other controllers).
The full prerequisite chain is: controller enabled → AAP configured → templates deployed via config-as-code → STORAGE_TIERS env var set on instance group → VAST appliance reachable with valid credentials.
5b19dca to
f9100bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@api/v1alpha1/job_types.go`:
- Around line 24-37: The JobType enum is shared across multiple CRDs but now
contains storage-specific values (JobTypeStorageBackendProvision,
JobTypeStorageBackendDeprovision, JobTypeClusterStorageProvision,
JobTypeClusterStorageDeprovision) that should not be accepted by all resources
using JobStatus. To preserve per-resource validation boundaries, create a
separate storage-specific job type contract instead of mixing storage operations
with the generic JobType. This means defining a new type (e.g., StorageJobType)
with its own enum values and kubebuilder validation constraint, then use this
specialized type only in storage-related CRDs while keeping JobType for generic
provisioning/deprovisioning operations across all resources.
In `@config/crd/bases/osac.openshift.io_securitygroups.yaml`:
- Around line 275-278: The storage-backend-provision,
storage-backend-deprovision, cluster-storage-provision, and
cluster-storage-deprovision job type values should not be part of the
SecurityGroup CRD schema since they are tenant-storage lifecycle operations, not
security group operations. Remove these four enum values from the jobs[].type
field definition in the SecurityGroup CRD to properly scope job types to only
security group-related operations and strengthen schema-level contract
validation.
In `@config/rbac/storage_secrets_role_binding.yaml`:
- Around line 13-16: Remove the explicit namespace field from the ServiceAccount
subject in the RoleBinding resource where the ServiceAccount subject has kind
ServiceAccount and name controller-manager. Additionally, remove the explicit
namespace field from the ServiceAccount resource definition itself in
config/rbac/service_account.yaml. This allows Kustomize to apply the namespace
transformation uniformly across all RBAC resources based on the configuration in
config/default/kustomization.yaml.
In `@internal/controller/storage_controller_test.go`:
- Around line 83-95: The createLabeledStorageClass helper function creates
StorageClass objects but never cleans them up, causing accumulation across tests
and potential flakiness since StorageClass is cluster-scoped. Add cleanup logic
to the createLabeledStorageClass function to delete the StorageClass after
creation, either by using a defer statement with k8sClient.Delete to ensure the
StorageClass is removed after the test completes, or by returning a cleanup
function that the test caller can use. This ensures that StorageClass objects do
not persist and interfere with subsequent tests.
- Around line 396-400: The test block with the Eventually call around the
Reconcile method invocation only verifies that Reconcile executes without error,
but does not actually assert that the Tenant resource is deleted or that the
storageFinalizer is removed. Add an additional assertion within the Eventually
block after the Reconcile call to verify that the Tenant is either deleted
(using Get with NotFound expectation) or that its metadata no longer contains
the storageFinalizer. This ensures the test comprehensively validates the
deletion and finalizer cleanup behavior.
In `@internal/controller/storage_controller.go`:
- Around line 506-565: The delete flow currently allows the finalizer removal to
proceed after a successful backend deprovision job without verifying that the
hub Secret has actually been deleted, which can leave credentials orphaned.
Modify the logic so that even after the backend deprovision job completes
successfully (when polling in pollDeprovisionJob returns or when handling the
DeprovisionTriggered action), the delete flow does not proceed until
hubSecretReady confirms the hub Secret is actually gone. Ensure the condition at
line 520 (the check for !hubSecretReady) remains a blocking requirement
throughout the delete process, preventing finalizer removal until the Secret
deletion is confirmed.
- Around line 207-229: When cluster storage resolution fails (when
len(result.resolved) == 0), the code sets TenantConditionClusterStorageReady to
False but leaves stale data in instance.Status.StorageClasses. In the unresolved
path (the if len(result.resolved) == 0 block), after setting the status
condition to false, explicitly clear instance.Status.StorageClasses by setting
it to an empty slice or nil before the early return statement, so that the
status accurately reflects the unresolved state without stale storage class
data.
In `@internal/controller/tenant_controller.go`:
- Around line 46-48: The TenantReconciler docstring (lines 46-48) is outdated
and no longer reflects the current implementation. The comment claims the
reconciler manages namespace creation and UDN reconciliation, but after the
refactor it only tracks namespace readiness and lifecycle state. Update the
docstring for TenantReconciler to remove references to namespace creation and
UDN reconciliation management, and instead accurately describe that it tracks
namespace readiness and lifecycle state, while keeping the note about Storage
provisioning being handled by the OSAC Storage Controller.
🪄 Autofix (Beta)
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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0156e81c-f79c-48d1-8925-e08684ff42d7
📒 Files selected for processing (37)
api/v1alpha1/job_types.goapi/v1alpha1/tenant_types.goapi/v1alpha1/zz_generated.deepcopy.gocharts/operator-crds/templates/osac.openshift.io_clusterorders.yamlcharts/operator-crds/templates/osac.openshift.io_computeinstances.yamlcharts/operator-crds/templates/osac.openshift.io_publicipattachments.yamlcharts/operator-crds/templates/osac.openshift.io_publicippools.yamlcharts/operator-crds/templates/osac.openshift.io_publicips.yamlcharts/operator-crds/templates/osac.openshift.io_securitygroups.yamlcharts/operator-crds/templates/osac.openshift.io_subnets.yamlcharts/operator-crds/templates/osac.openshift.io_tenants.yamlcharts/operator-crds/templates/osac.openshift.io_virtualnetworks.yamlcmd/main.goconfig/crd/bases/osac.openshift.io_clusterorders.yamlconfig/crd/bases/osac.openshift.io_computeinstances.yamlconfig/crd/bases/osac.openshift.io_publicipattachments.yamlconfig/crd/bases/osac.openshift.io_publicippools.yamlconfig/crd/bases/osac.openshift.io_publicips.yamlconfig/crd/bases/osac.openshift.io_securitygroups.yamlconfig/crd/bases/osac.openshift.io_subnets.yamlconfig/crd/bases/osac.openshift.io_tenants.yamlconfig/crd/bases/osac.openshift.io_virtualnetworks.yamlconfig/rbac/kustomization.yamlconfig/rbac/storage_secrets_role.yamlconfig/rbac/storage_secrets_role_binding.yamlinternal/controller/computeinstance_controller.gointernal/controller/computeinstance_controller_test.gointernal/controller/computeinstance_integration_test.gointernal/controller/computeinstance_resources.gointernal/controller/storage_controller.gointernal/controller/storage_controller_test.gointernal/controller/storage_tier_resolution.gointernal/controller/tenant_controller.gointernal/controller/tenant_controller_test.gointernal/controller/tenant_names.gopkg/aap/client.gopkg/provisioning/aap_provider.go
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 `@internal/controller/storage_controller_test.go`:
- Around line 95-97: The DeferCleanup function on lines 95-97 is silently
dropping the error from k8sClient.Delete by assigning it to underscore, which
can leak cluster-scoped StorageClass test state. Instead of ignoring the error,
check it using client.IgnoreNotFound to ensure that only NotFound errors are
acceptable and other deletion errors are properly handled or asserted. Apply the
same fix to the similar code on lines 403-406 where Get errors are incorrectly
being treated as passing deletion/finalizer checks. Replace underscore error
assignments with proper error checks that use client.IgnoreNotFound to
distinguish between expected NotFound errors and unexpected errors that should
fail the test.
🪄 Autofix (Beta)
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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e0b1fbf7-9aa5-4754-822e-a67145536313
📒 Files selected for processing (3)
internal/controller/storage_controller.gointernal/controller/storage_controller_test.gointernal/controller/tenant_controller.go
0526f9e to
a448ff6
Compare
I wouldn't wait for that for 0.1 but sub-nets having storage job types is a smell and we should change it |
Circulated the document CRD_JobTrackingAlternatives and got agreement on Option C. |
a448ff6 to
6535639
Compare
- Fix gofmt formatting in main.go, storage_tier_resolution.go, tenant_controller_test.go - Break long lines in setupStorageController to stay under 120 chars - Remove unused eventReasonStorageClassNotReady const - Fix namespace collision in tenant test (IgnoreAlreadyExists) - Sync Helm CRD templates from config/crd/bases - Remove extra blank line in tenant_types.go Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
- Use `make helm-crds` to properly sync CRDs with Helm wrappers - Handle AlreadyExists for Tenant CR in BeforeEach (same envtest race as namespace) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
Each It test case now uses a unique tenant/namespace name to avoid envtest cleanup races. The shared BeforeEach/AfterEach pattern caused finalizer-related interference between test cases. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
- Rename handleClassProvisioning→handleClusterStorageProvisioning and related functions for naming consistency with ClusterStorageReady - Replace local needsProvisionJob with shared provisioning.NeedsProvisionJob to avoid redundant AAP job triggers on success - Add TODO(OSAC-1123) for ClusterOrder-to-Tenant watch placeholder - Rename osacTenantAnnotation→osacTenantKey with updated comment reflecting dual usage as label and annotation - Scope Secrets RBAC to osac-system namespace via Role+RoleBinding instead of cluster-wide ClusterRole entry Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
Covers the design spec test plan: - Stage 1: tenant not ready (skip), no hub Secret (trigger provision), Secret exists (skip to Stage 2), no provider (NoProvider condition), job failure (failed job recorded) - Stage 2: Stage 1 complete + no SCs (trigger provision), SCs discovered (ClusterStorageReady=True) - Tier resolution: Default fallback, tenant-specific priority - Finalizer: added on first reconcile, deletion runs without class provider - Management state: Unmanaged skips reconciliation Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
- Clear status.storageClasses when no SCs resolved (prevents stale data when ClusterStorageReady=False) - Fix tenant controller docstring (tracks readiness, doesn't create) - Add DeferCleanup for StorageClasses in tests (prevent cross-test leak) - Strengthen deletion test to assert finalizer removal Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
…all CRDs Rename status.jobs to status.provisioningJobs on all 9 CRDs to establish a uniform naming pattern. Add lifecycle-specific job arrays on Tenant (storageBackendJobs, clusterStorageJobs) and ClusterOrder (clusterStorageJobs) so storage controllers use standard provision/ deprovision job types scoped by array instead of polluting the shared JobType enum. Changes: - Remove 4 storage-specific JobType enum values - Rename Jobs -> ProvisioningJobs in all CRD status structs - Split storage controller job tracking into per-lifecycle arrays - Refactor pollDeprovisionJob to accept jobs parameter - Add 4 new unit tests for job array isolation and failure paths - Regenerate CRDs and Helm chart Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
Make the provisioning lifecycle helpers lifecycle-agnostic so the storage controller and ComputeInstance can use them directly instead of reimplementing ~400 lines of manual job management. Changes: - Add JobsExtractor type for CheckAPIServerForNonTerminalProvisionJob to support lifecycle-specific job array extraction from fresh resources - Add provisionJobs parameter to TriggerDeprovision interface so the provider receives the correct provision jobs for pre-deprovision checks instead of re-extracting via GetJobsFromResource - Replace storage controller's 6 manual provisioning/deprovisioning methods with RunProvisioningLifecycle and RunDeprovisioningLifecycle - Replace ComputeInstance's handleDeprovisioning (~100 lines) with RunDeprovisioningLifecycle call - Drop fake failed jobs on trigger errors (align with standard behavior) - Keep wait-for-external-trigger on failure (storage-specific pre-check) Net: -346 lines (482 removed, 136 added) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Zoltan Szabo <zszabo@redhat.com>
Format long JobsExtractor inline functions with gofmt and remove the unused deprovisioningJobTriggeredMessage constant. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
b385bed to
8e66343
Compare
Format long JobsExtractor inline functions in publicippool, securitygroup, and virtualnetwork controllers. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@cmd/main.go`:
- Around line 434-443: The StorageReconciler is being initialized with
potentially nil backendProvider and clusterStorageProvider values when aapURL or
aapToken are empty, but there is no logging to indicate this watch-only mode.
Add an informational log message before the NewStorageReconciler call to check
if either backendProvider or clusterStorageProvider are nil, and if so, log a
message indicating that the storage controller is running in watch-only mode
without AAP integration capability. This ensures operators are aware when the
controller lacks provisioning capabilities.
In `@internal/controller/computeinstance_controller.go`:
- Around line 467-474: The code calls RunDeprovisioningLifecycle with
r.ProvisioningProvider without first checking if the provider is nil, which can
cause the delete reconcile to fail. Add a nil check for r.ProvisioningProvider
before the RunDeprovisioningLifecycle call, similar to the existing pattern used
for checking the management-state annotation, and return early if the provider
is not set to safely skip the deprovision operation when the provider is unset.
In `@internal/controller/storage_tier_resolution.go`:
- Around line 49-54: The joinStorageClassNames function returns storage class
names in API list order, which can vary across reconciles and cause the
duplicate-tier messages at lines 123 and 143 to flip ordering and create
unnecessary status churn. Sort the names slice alphabetically before returning
it from joinStorageClassNames to ensure consistent ordering of storage class
names regardless of API list order. This prevents status updates caused by
message composition changes across reconciles.
In `@pkg/provisioning/provision_lifecycle_test.go`:
- Around line 49-50: The TriggerDeprovision method in mockProvider accepts the
provisionJobs parameter but does not pass it to triggerDeprovisionFunc, making
it impossible to test deprovision job-history plumbing. Update the
triggerDeprovisionFunc function type signature to accept the provisionJobs
parameter and then pass it when calling triggerDeprovisionFunc in the
TriggerDeprovision method implementation so that test cases can verify the job
history is properly handled during deprovisioning.
In `@pkg/provisioning/provision_lifecycle.go`:
- Around line 79-84: The function `CheckAPIServerForNonTerminalProvisionJob`
calls the `extract` function parameter unconditionally on line 84, but if
`extract` is nil, this will panic the controller. Add a nil check for the
`extract` parameter before calling `extract(fresh)` and return false if the
extractor is not provided to safely skip the check instead of panicking.
🪄 Autofix (Beta)
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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7cbcbc1d-b2d3-4a47-a5fa-ac37f9677cac
📒 Files selected for processing (73)
.gitignoreapi/v1alpha1/clusterorder_types.goapi/v1alpha1/computeinstance_types.goapi/v1alpha1/publicip_types.goapi/v1alpha1/publicipattachment_types.goapi/v1alpha1/publicipattachment_types_test.goapi/v1alpha1/publicippool_types.goapi/v1alpha1/securitygroup_types.goapi/v1alpha1/subnet_types.goapi/v1alpha1/tenant_types.goapi/v1alpha1/virtualnetwork_types.goapi/v1alpha1/zz_generated.deepcopy.gocharts/operator-crds/templates/osac.openshift.io_clusterorders.yamlcharts/operator-crds/templates/osac.openshift.io_computeinstances.yamlcharts/operator-crds/templates/osac.openshift.io_publicipattachments.yamlcharts/operator-crds/templates/osac.openshift.io_publicippools.yamlcharts/operator-crds/templates/osac.openshift.io_publicips.yamlcharts/operator-crds/templates/osac.openshift.io_securitygroups.yamlcharts/operator-crds/templates/osac.openshift.io_subnets.yamlcharts/operator-crds/templates/osac.openshift.io_tenants.yamlcharts/operator-crds/templates/osac.openshift.io_virtualnetworks.yamlcharts/operator/templates/deployment.yamlcharts/operator/templates/storage-secrets-role.yamlcharts/operator/templates/storage-secrets-rolebinding.yamlcmd/main.goconfig/crd/bases/osac.openshift.io_clusterorders.yamlconfig/crd/bases/osac.openshift.io_computeinstances.yamlconfig/crd/bases/osac.openshift.io_publicipattachments.yamlconfig/crd/bases/osac.openshift.io_publicippools.yamlconfig/crd/bases/osac.openshift.io_publicips.yamlconfig/crd/bases/osac.openshift.io_securitygroups.yamlconfig/crd/bases/osac.openshift.io_subnets.yamlconfig/crd/bases/osac.openshift.io_tenants.yamlconfig/crd/bases/osac.openshift.io_virtualnetworks.yamlconfig/rbac/kustomization.yamlconfig/rbac/storage_secrets_role.yamlconfig/rbac/storage_secrets_role_binding.yamlinternal/controller/clusterorder_controller.gointernal/controller/clusterorder_controller_test.gointernal/controller/clusterorder_integration_test.gointernal/controller/computeinstance_controller.gointernal/controller/computeinstance_controller_test.gointernal/controller/computeinstance_integration_test.gointernal/controller/computeinstance_provisioning_test.gointernal/controller/computeinstance_resources.gointernal/controller/constants_common.gointernal/controller/publicip_controller.gointernal/controller/publicip_controller_test.gointernal/controller/publicipattachment_controller.gointernal/controller/publicipattachment_controller_test.gointernal/controller/publicippool_controller.gointernal/controller/publicippool_controller_test.gointernal/controller/securitygroup_controller.gointernal/controller/securitygroup_controller_test.gointernal/controller/storage_controller.gointernal/controller/storage_controller_test.gointernal/controller/storage_tier_resolution.gointernal/controller/subnet_controller.gointernal/controller/subnet_controller_test.gointernal/controller/suite_test.gointernal/controller/tenant_controller.gointernal/controller/tenant_controller_test.gointernal/controller/tenant_names.gointernal/controller/virtualnetwork_controller.gointernal/controller/virtualnetwork_controller_test.gopkg/aap/client.gopkg/provisioning/aap_provider.gopkg/provisioning/aap_provider_test.gopkg/provisioning/factory.gopkg/provisioning/factory_test.gopkg/provisioning/provider.gopkg/provisioning/provision_lifecycle.gopkg/provisioning/provision_lifecycle_test.go
💤 Files with no reviewable changes (1)
- internal/controller/constants_common.go
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
|
The From the operator logs: This repeats every 30 seconds. Because the operator cannot persist job tracking state, it launches a new AAP provision job on every reconcile cycle, creating an infinite loop. Manual E2E validation on edge22: I ran the full All provisioning controllers work correctly with the renamed fields: VirtualNetwork, Subnet, SecurityGroup, ComputeInstance, PublicIPPool, and PublicIPAttachment all pass. I'd like to Question cc: @zszabo-rh |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, zszabo-rh 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 |
|
/override ci/prow/e2e-vmaas |
|
@omer-vishlitzky: Overrode contexts on behalf of omer-vishlitzky: ci/prow/e2e-vmaas 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 kubernetes-sigs/prow repository. |
|
@zszabo-rh: The following test failed, say
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. |
5 participants
Summary
Extracts storage provisioning and deprovisioning logic from the Tenant controller into a dedicated OSAC Storage Controller, as specified in the design document (merged) and PRD (merged).
Key changes:
StorageReconcilerwatches Tenant CRs as a secondary controller with its own finalizer (osac.openshift.io/storage)StorageBackendReady(Stage 1: hub Secret) andClusterStorageReady(Stage 2: StorageClasses on target cluster)BackendProviderandClusterStorageProviderwith separate template env varsStorageBackendStatusandClusterStorageStatusfor future multi-backend/multi-cluster supportOSAC_ENABLE_STORAGE_CONTROLLER/--enable-storage-controllerEnv vars:
Dependencies:
OSAC_ENABLE_STORAGE_CONTROLLER=false) — it won't attempt to launch any templates until explicitly enabled. Merging AAP first would be risky: the config-as-code sync would rename the existing templates, breaking the current tenant controller on any deployment that has it enabled.Test plan
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
New Features
Breaking Changes
status.jobstostatus.provisioningJobs.Bug Fixes / Improvements