[Feature] Redesign DBInstance Controller Around Bounded Level-driven Reconciliation#230
Open
Yohansenanayake wants to merge 61 commits into
Open
[Feature] Redesign DBInstance Controller Around Bounded Level-driven Reconciliation#230Yohansenanayake wants to merge 61 commits into
Yohansenanayake wants to merge 61 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
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 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 |
…tableDrift function
…literals in switch cases and assignments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@database/internal/controller/dbinstance_controller.go`:
- Around line 224-226: Update the deletion path around finalizeStatus and
patchStatusIfChanged to capture and return the patch error instead of discarding
it. Preserve the successful return when the status update succeeds, and
propagate failures so controller retry behavior requeues reconciliation.
In `@database/README.md`:
- Around line 20-26: Update the “What’s NOT in this version” table in the README
to remove or revise the entry claiming status.conditions is not written, so it
consistently documents the condition-based status model described in the
Reconciler section.
🪄 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: 49e947f2-ad1c-429f-9ebe-03974aaddcb8
📒 Files selected for processing (17)
database/README.mddatabase/api/v1alpha1/condition_reason_enforcement_test.godatabase/config/crd/bases/dbaas.opencloud.wso2.com_dbinstances.yamldatabase/internal/controller/dbinstance_controller.godatabase/internal/controller/ensure_health.godatabase/internal/controller/ensure_health_test.godatabase/internal/controller/reconcile_delete_test.godatabase/internal/controller/status_ready.godatabase/internal/controller/stop_start_test.godatabase/internal/credentials/material.godatabase/internal/credentials/resolver.godatabase/internal/credentials/resolver_test.godatabase/internal/harvester/typed_client.godatabase/internal/harvester/typed_client_test.godatabase/internal/resource/builder_test.godatabase/internal/resource/connection_secret.godatabase/internal/resource/metrics_endpoints.go
🚧 Files skipped from review as they are similar to previous changes (13)
- database/internal/resource/metrics_endpoints.go
- database/internal/controller/status_ready.go
- database/internal/resource/connection_secret.go
- database/api/v1alpha1/condition_reason_enforcement_test.go
- database/internal/controller/reconcile_delete_test.go
- database/internal/credentials/resolver_test.go
- database/internal/credentials/resolver.go
- database/internal/controller/ensure_health.go
- database/internal/controller/stop_start_test.go
- database/internal/controller/ensure_health_test.go
- database/internal/harvester/typed_client_test.go
- database/internal/harvester/typed_client.go
- database/config/crd/bases/dbaas.opencloud.wso2.com_dbinstances.yaml
Yohansenanayake
changed the title
…documentation directories
… corresponding test
…method fix(docs): update comment to reflect single NIC configuration in BuildNetworkData
…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
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
1 participant
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