[Feature] Redesign DBInstance Controller Around Bounded Level-driven Reconciliation - #230
Open
Yohansenanayake wants to merge 77 commits into
Open
[Feature] Redesign DBInstance Controller Around Bounded Level-driven Reconciliation#230Yohansenanayake wants to merge 77 commits into
Yohansenanayake wants to merge 77 commits into
Conversation
…and interface files
…econciliation steps
…ance - Added ensureReady function to set the DBInstance status to available and update observed generation. - Created ensure_steps.go to manage the ordered steps during DBInstance provisioning. - Implemented runEnsureSteps to execute provisioning steps and handle outcomes. - Developed ensureVM function to ensure the VirtualMachine resource exists and create it if absent. - Added tests for ensureReady, ensureVM, and provisioning steps to validate functionality. - Removed obsolete probe_gate_test.go as its functionality is now covered by new tests. - Introduced stubHarvester for testing, simulating interactions with the Harvester client.
- Added `ensureStorageResize` method to handle cold resizing of VMs based on shape drift. - Introduced `vmShapeDrift` struct to compare VM's declared shape against desired specifications. - Created tests for various scenarios in `ensure_resize_test.go`, including: - No drift satisfied - Class drift requiring VM stop - Waiting for teardown during resize - Applying class and storage changes when VM is down - Handling unsupported storage shrink attempts - Updated `ensure_steps.go` to include the new resize step in the provisioning process. - Refactored existing tests to accommodate changes in the lifecycle management of VMs. - Enhanced `stubHarvester` to track resize calls for testing purposes. - Modified `typed_client.go` and `typed_client_test.go` to reflect changes in volume claim template handling.
- Update `ensurePowerState` to suspend power management during crash-loop conditions, ensuring VMs are not restarted or stopped unnecessarily. - Modify tests to validate that power management behaves correctly under crash-loop conditions, ensuring no VM calls are made while in a halted state. - Enhance `ensureReady` to reflect degraded states accurately, ensuring that the phase remains consistent with health checks. - Introduce new tests to verify that the system correctly handles degraded states and does not trigger unnecessary VM operations. - Implement cleanup steps in the provisioning process to remove cloud-init secrets after VM creation. - Adjust the stub harvester to log operations for better traceability during tests.
- Introduced a new resource package to handle declarative child objects (Service, Endpoints, ServiceMonitor) for monitoring. - Updated DBInstance reconciler to create and manage monitoring resources with controller ownership. - Added owner reference handling for VM creation to ensure proper garbage collection. - Removed legacy monitoring deployment methods from the typed client. - Enhanced tests to verify the creation and ownership of monitoring resources. - Ensured that metrics Service and Endpoints are updated correctly on VM IP changes.
Move credential and TLS material generation out of the Harvester client and into a new internal/credentials package (Resolver.Resolve), decomposing the old monolithic per-instance Secret into three: - pg-<name>-credentials (tenant ns): admin_user/admin_password only - dbi-<uid>-internal (operator ns): repl_password/exporter_password - dbi-<uid>-tls (operator ns, type kubernetes.io/tls): CA + server cert/key Each is get-or-create-once and never regenerated on reentry, preserving the invariant that a booted VM's password/CA can't drift from what the operator holds. status.CACertPEM is removed; the CA is now served via a new pg-<name>-connect Secret (internal/resource.ConnectionSecret) alongside host/port/dbname/jdbcUrl/sslmode, reconciled once status.Endpoint is known. Cloud-init rendering (internal/credentials/cloudinit.go, moved from internal/harvester) now reads from the resolved Material instead of generating its own secrets inline, and is applied via a new internal/resource.CloudInitSecret builder before VM creation. harvester.CreatePostgresVM is slimmed to (VMCreateParams) (vmName, err) with a CloudInitSecretName field — it no longer generates any credential material itself, in both the typed and legacy dynamic clients. Because the two operator-namespace Secrets are cross-namespace from their DBInstance and can't carry an owner reference, reconcileDelete now removes them explicitly: by the recorded status.resources ref first, then a dbaas.opencloud.wso2.com/dbinstance-uid label sweep as a backstop for a ref lost to a status reset. A new --operator-namespace flag (default POD_NAMESPACE via downward API, fallback "dbaas-system") controls where these live.
…ace(PR9) Close a real enforcement gap first: status.appliedSpec never tracked spec.staticNetwork/spec.vmPassword (true since the original commit, not a regression), so immutableDrift() silently allowed editing them post-create. Both are now snapshotted and compared (equality.Semantic.DeepEqual for the struct field). Add +kubebuilder:validation:XValidation "self== oldSelf" transition rules on networkRef/engineVersion/staticNetwork/vmPassword as API-server-level defense-in-depth. Deliberately skip the other five documented-immutable fields (osImage/dbName/masterUsername/port/storageType): immutableDrift() compares them post-defaulting, so a raw CEL rule on the unset spec value would be stricter than today's behavior (e.g. rejecting an explicit port: 5432 after leaving it unset). No enum markers either: dbInstanceClass is validated against a live Go map already, and storageType's real value on Harvester is unconfirmed — enum-constraining either now would either duplicate maintenance or risk locking out the correct value. Remove the legacy dynamic-client Harvester implementation (client.go, probe.go, cmd/harvester_dynamic.go) and the dead DialVMListener method it was built around, both already self-flagged "not used anymore" in the code. TypedClient is now the only implementation, constructed directly in cmd/main.go with no build tag. While touching ClientInterface, drop two more methods that never belonged on a Harvester-specific contract: CreateDataVolume did no I/O at all (pure string formatting, now the plain DataVolumeName function next to buildPostgresVM), and DeleteSecret deleted a plain corev1.Secret via client-go with nothing Harvester-specific about it (now a direct r.Delete call in ensureBootstrapCleanup, matching the pattern deleteOperatorSecrets already used). interface.go now holds only the actual contract: the interface plus the DTOs its methods reference.
…ret, update related tests
… related comments and tests
…atusIfChanged and clean up unused code
…rove comments for clarity
…te method names for consistency
…for clarity, introduce new status for incompatible parameters, and ensure DatabaseReady condition is accurately set during power state and resize operations.
…ng readiness checks and recovery logic
- Introduced a new `finalizeStatus` method to centralize status updates for DBInstance, ensuring consistent handling of Accepted, Ready, and InterventionRequired conditions. - Replaced legacy condition checks with a more robust mechanism that uses ConditionReason for better clarity and maintainability. - Updated tests to reflect changes in condition handling, ensuring that the status phase accurately represents the current state of the DBInstance. - Removed the `markProvisioningFailed` function, consolidating its logic into the new condition management approach. - Enhanced error handling in various steps to ensure that the DBInstance reflects the correct status based on the latest conditions. - Added new tests to validate the behavior of the updated status management logic, ensuring comprehensive coverage of edge cases.
…geChangeRejected and update related logic and tests for clarity
… for resize operations and enhance test coverage for resize lifecycle
…ng for database recovery and update test cases for clarity
…ance comments for clarity on resize lifecycle aggregation
- Add ensureConnectionSecret function to publish tenant connection secrets after database health is established. - Modify ensureCredentials to handle credential changes and requeue if necessary. - Update tests to cover new connection secret logic and ensure proper handling of credentials. - Refactor credential resolver to return a result indicating if the state has changed. - Ensure connection secrets are created only when the database endpoint is known. - Adjust provisioning steps to include connection secret management.
…in DBInstance reconciler
…oud-init and connection secrets
…nstanceStatus and related logic
…init secret management
|
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:
📝 WalkthroughWalkthroughThe DBInstance controller is redesigned around ordered ensure steps and observed state. The change adds condition-based status and phase derivation, durable credential/TLS Secrets, connection and monitoring resources, VM health and resize handling, cloud-init redaction, deletion cleanup, watches, schema validation, and extensive tests. ChangesDBInstance controller redesign
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
…alsResolvesAndRecordsRefsWithoutEndpoint
…alse liveness monitor alerts
…ent shell injection test(credentials): add test for cloud-init to ensure shell metacharacters are neutralized fix(controller): add check to prevent VM restart when VMI is gone
…icNetwork after creation in DBInstance
…ApplyBuildersIdempotentWithOwnerRefs
…prove error reporting
… and recovery logic
…on; improve cloud-init YAML formatting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
database/internal/controller/status.go (1)
56-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve and remove the leftover "redudant?" comment.
This line ships with an open question as an inline comment (
//redudant ? can we remove this ?, also misspelled). The assignment is likely intentional — it re-syncsinst.Statusto the actual persisted object even when only semantically equal todesiredStatus, guarding against subtle field differences (e.g., server-setLastTransitionTime) thatequality.Semantic.DeepEqualignores but exact equality wouldn't. Please resolve the question and replace the comment with a clear rationale rather than shipping a rhetorical note.📝 Proposed fix
- inst.Status = *latest.Status.DeepCopy() //redudant ? can we remove this ? + // Sync the caller's status with the persisted value; latest.Status may + // differ from desiredStatus in fields equality.Semantic.DeepEqual + // treats as equal (e.g. LastTransitionTime set by an earlier patch). + inst.Status = *latest.Status.DeepCopy()🤖 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 `@database/internal/controller/status.go` around lines 56 - 60, Resolve the inline comment on the inst.Status assignment in the status reconciliation flow: retain the assignment because it re-syncs inst.Status with the persisted latest.Status when semantic equality hides server-populated field differences, and replace the rhetorical misspelled note with a concise rationale describing that purpose.
🤖 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 `@database/README.md`:
- Around line 38-53: The Monitoring row in the “What it provisions” section
contains a dangling “see below” reference; remove that cross-reference while
preserving the exporter-install-pending statement, since no corresponding
limitation entry remains in the “Not yet implemented” table.
---
Nitpick comments:
In `@database/internal/controller/status.go`:
- Around line 56-60: Resolve the inline comment on the inst.Status assignment in
the status reconciliation flow: retain the assignment because it re-syncs
inst.Status with the persisted latest.Status when semantic equality hides
server-populated field differences, and replace the rhetorical misspelled note
with a concise rationale describing that purpose.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9aa3d38e-9927-4fbb-83e1-5cc4b4fe6eb0
📒 Files selected for processing (22)
database/README.mddatabase/api/v1alpha1/dbinstance_types.godatabase/internal/controller/dbinstance_controller.godatabase/internal/controller/ensure_connection_secret.godatabase/internal/controller/ensure_finalizer.godatabase/internal/controller/ensure_health.godatabase/internal/controller/ensure_health_test.godatabase/internal/controller/ensure_monitoring_test.godatabase/internal/controller/ensure_power.godatabase/internal/controller/ensure_power_test.godatabase/internal/controller/ensure_preflight_test.godatabase/internal/controller/ensure_resize_test.godatabase/internal/controller/ensure_steps.godatabase/internal/controller/ensure_vm_test.godatabase/internal/controller/failed_recovery_test.godatabase/internal/controller/outcomes.godatabase/internal/controller/reconcile_delete_test.godatabase/internal/controller/status.godatabase/internal/controller/stub_harvester_test.godatabase/internal/harvester/interface.godatabase/internal/harvester/typed_client.godatabase/internal/harvester/typed_client_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- database/internal/controller/ensure_finalizer.go
- database/internal/controller/outcomes.go
- database/internal/controller/ensure_steps.go
- database/internal/controller/ensure_connection_secret.go
- database/internal/controller/ensure_vm_test.go
- database/internal/controller/stub_harvester_test.go
- database/internal/harvester/interface.go
- database/internal/controller/ensure_preflight_test.go
- database/internal/controller/ensure_health_test.go
- database/internal/controller/ensure_power.go
- database/internal/controller/ensure_resize_test.go
- database/internal/controller/failed_recovery_test.go
- database/internal/controller/ensure_monitoring_test.go
- database/api/v1alpha1/dbinstance_types.go
- database/internal/harvester/typed_client.go
- database/internal/controller/ensure_health.go
…il VM IP is known
…on and syncReadyCondition for clarity and accuracy
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 `@database/internal/controller/watches.go`:
- Around line 75-90: Update vmiInterfaceIPsEqual to distinguish missing
interface names from entries whose IP is the empty string by checking newIPs key
existence alongside value equality. Preserve the existing length check and
return false whenever a name is absent or its IP differs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c779c69-a4bc-438b-b1b2-503e3e0b63ba
📒 Files selected for processing (8)
database/api/v1alpha1/dbinstance_types.godatabase/internal/controller/status_conditions.godatabase/internal/controller/status_ready.godatabase/internal/controller/watches.godatabase/internal/credentials/cloudinit.godatabase/internal/credentials/cloudinit_test.godatabase/internal/resource/builder_test.godatabase/internal/resource/metrics_endpoints.go
🚧 Files skipped from review as they are similar to previous changes (6)
- database/internal/controller/status_ready.go
- database/internal/resource/metrics_endpoints.go
- database/internal/controller/status_conditions.go
- database/internal/resource/builder_test.go
- database/internal/credentials/cloudinit.go
- database/api/v1alpha1/dbinstance_types.go
…hecks; enhance error classification for image resolution
…reVM; enhance stub for resize tracking
…ple files for clarity and accuracy
…ng; remove unused code and enhance status patching logic
…and improve logging format
…inalizer handling in Reconcile method
- introduce the ensure.Step interface and Runner - move ensure-step implementations into internal/ensure - centralize step results and dependencies - expose shared condition helpers on DBInstance - move reusable test fixtures into internal/testutil - preserve existing reconciliation behavior and test coverage
- move step-specific tests beside their implementations - retain reconciliation and envtest coverage in the controller package - remove the legacy controller test adapter - centralize shared fixtures in package-local test helpers - clean up stale helpers, comments, and scaffold TODOs
gnudeep
approved these changes
Jul 23, 2026
gnudeep
approved these changes
Jul 23, 2026
Yohansenanayake
added
the
Severity/Blocker
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Redesigns the
DBInstancecontroller from a phase-pointer workflow into a level-driven, bounded reconciliation pipeline.The controller now derives decisions from observed Kubernetes and Harvester state rather than using status as memory of previously completed actions. Ordered, idempotent ensure steps continue only while their prerequisites are satisfied and stop after external mutations, asynchronous waits, terminal states, or transient failures.
Closes #229.
Design discussion: #207
Changes
Controller architecture
status.provisioningPhasedispatcher with an ordered ensure pipeline covering finalization, preflight, credentials, VM, resize, power, database health, connection metadata, monitoring, bootstrap cleanup, and generation completion.Satisfied,Pending,Terminal, andTransientstep outcomes.--max-concurrent-reconciles.Status and lifecycle contract
DatabaseReady), monitoring convergence (MonitoringReady), and aggregate product readiness (Ready).degraded,incompatible-parameters, andcrash-loop-halted.observedGenerationonly after the complete ensure chain is re-observed as converged.Managed resources and credentials
API and validation changes
status.provisioningPhasestatus.masterUserSecretstatus.caCertPemstatus.resources.secretNamewithstatus.resources.adminCredentialsSecretName.This is intentionally a breaking redesign. Legacy status compatibility and migration behavior are not included because there are no production
DBInstanceresources requiring migration.Testing
Verified with:
go test ./... -count=1All automated tests pass.
Checklist
Summary by CodeRabbit