[Fix] Improve DBInstance status and condition patching - #238
Conversation
📝 WalkthroughWalkthroughThe PR adds shared condition and status patching, updates DBInstance image and condition contracts, refactors reconciliation and deletion handling, revises Harvester VM lifecycle and cloud-init behavior, and adds manager configuration plus controller and provider tests. ChangesDBInstance API and status contracts
Condition and status patch infrastructure
Harvester lifecycle and cloud-init integration
Controller orchestration and deployment wiring
Validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Yohansenanayake
changed the title
Yohansenanayake
added
the
Type/Improvement
…c for condition management
…ing and condition management
2d6e979 to
2d55fcc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
database/internal/harvester/typed_client.go (2)
149-151: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate the existing VM owner before accepting name conflicts.
CreatePostgresVMreturns successfully onAlreadyExists, then laterTeardownAlldeletes the stored VM name unconditionally. Fetch the existing VM and only return success when it has the expected controller owner forp.Owner; otherwise return a collision error instead of treating another controller’s DBInstance VM as owned.🤖 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/harvester/typed_client.go` around lines 149 - 151, Update CreatePostgresVM around the VirtualMachines(...).Create call to handle AlreadyExists by fetching the existing VM, validating that its controller owner matches the expected owner for p.Owner, and only then treating the conflict as success. Return a collision error for an existing VM owned by another controller, while preserving normal creation errors and ensuring TeardownAll is not given an unowned VM name.
283-305: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore cleanup for VM disk resources.
TeardownAllrecordsDataVolumeNameand writes both OS and data disks via VM template PVCs, but it does not delete them. Add teardown tasks for the OS PVC (pg-{id}-os) and data PVC (DataVolumeName) before deleting the VM, and cover these deletions in the teardown tests.🤖 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/harvester/typed_client.go` around lines 283 - 305, Update TeardownAll’s deleteTask list to remove the VM template PVCs before the virtual machine: add tasks for the OS PVC named pg-{id}-os and the data PVC identified by refs.DataVolumeName, using the Kubernetes CoreV1 PVC delete API. Preserve the existing deletion ordering and extend teardown tests to verify both PVC deletions.
🤖 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/api/v1alpha1/dbinstance_types.go`:
- Around line 157-169: Add the CRD CEL immutability validation rule self ==
oldSelf to the OSImage schema, matching the existing networkRef validation. Keep
OSImage included in AppliedSpec and immutablerDrift() so controller drift
detection remains aligned with the API-level guard.
In `@database/internal/credentials/cloudinit.go`:
- Around line 199-207: Update the cloud-init PostgreSQL installation flow around
PG_VER so Spec.EngineVersion is not silently ignored: explicitly install and
configure the requested engine version, or atomically remove/reject that API
field while preserving reconciliation state consistency. Ensure the recorded
applied version matches the server actually installed, rather than always
deriving the distribution default from pg_lsclusters.
In `@database/internal/patch/patch.go`:
- Around line 92-112: Update Helper.patchStatus to build the
client.MergeFrom(before) patch helper, inspect its Data(after) result, and
return nil when the focused status patch is empty; only call
helper.client.Status().Patch for non-empty data. Preserve the existing
shouldPatch, calculateStatusPatch, and error-handling flow.
---
Outside diff comments:
In `@database/internal/harvester/typed_client.go`:
- Around line 149-151: Update CreatePostgresVM around the
VirtualMachines(...).Create call to handle AlreadyExists by fetching the
existing VM, validating that its controller owner matches the expected owner for
p.Owner, and only then treating the conflict as success. Return a collision
error for an existing VM owned by another controller, while preserving normal
creation errors and ensuring TeardownAll is not given an unowned VM name.
- Around line 283-305: Update TeardownAll’s deleteTask list to remove the VM
template PVCs before the virtual machine: add tasks for the OS PVC named
pg-{id}-os and the data PVC identified by refs.DataVolumeName, using the
Kubernetes CoreV1 PVC delete API. Preserve the existing deletion ordering and
extend teardown tests to verify both PVC deletions.
🪄 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 Plus
Run ID: 7e6e46b1-d4d6-4f2c-a515-1fcfea2bfd5b
📒 Files selected for processing (37)
database/.gitignoredatabase/api/v1alpha1/dbinstance_conditions.godatabase/api/v1alpha1/dbinstance_types.godatabase/api/v1alpha1/zz_generated.deepcopy.godatabase/cmd/main.godatabase/config/crd/bases/dbaas.opencloud.wso2.com_dbinstances.yamldatabase/config/manager/manager.yamldatabase/config/rbac/role.yamldatabase/images/packer/build.shdatabase/images/packer/http/meta-datadatabase/images/packer/http/user-datadatabase/images/packer/images.yamldatabase/images/packer/scripts/provision.shdatabase/images/packer/ubuntu-postgres.pkr.hcldatabase/internal/conditions/getter.godatabase/internal/conditions/patch.godatabase/internal/conditions/patch_test.godatabase/internal/conditions/setter.godatabase/internal/controller/controller_test_helpers_test.godatabase/internal/controller/dbinstance_controller.godatabase/internal/controller/reconcile_delete_test.godatabase/internal/controller/reconcile_instance.godatabase/internal/controller/reconcile_instance_monitoring_test.godatabase/internal/controller/reconcile_instance_test.godatabase/internal/controller/reconcile_status_defer_test.godatabase/internal/controller/status.godatabase/internal/controller/status_test.godatabase/internal/credentials/cloudinit.godatabase/internal/harvester/interface.godatabase/internal/harvester/typed_client.godatabase/internal/harvester/typed_client_test.godatabase/internal/patch/options.godatabase/internal/patch/patch.godatabase/internal/patch/patch_test.godatabase/internal/patch/serial.godatabase/internal/patch/utils.godatabase/test/repave-e2e.sh
💤 Files with no reviewable changes (13)
- database/images/packer/scripts/provision.sh
- database/images/packer/http/user-data
- database/images/packer/http/meta-data
- database/images/packer/build.sh
- database/.gitignore
- database/images/packer/ubuntu-postgres.pkr.hcl
- database/images/packer/images.yaml
- database/test/repave-e2e.sh
- database/config/manager/manager.yaml
- database/config/rbac/role.yaml
- database/api/v1alpha1/zz_generated.deepcopy.go
- database/cmd/main.go
- database/internal/harvester/interface.go
| // OSImage is the Harvester VirtualMachineImage to clone for the VM's | ||
| // OS disk. Either "<ns>/<name>" or just "<name>" (resolved in the | ||
| // "default" namespace), or the image's spec.displayName. | ||
| // Immutable after first reconcile. | ||
| // +optional | ||
| OSImage string `json:"osImage,omitempty"` | ||
|
|
||
| // NetworkRef is a Harvester NAD reference (namespace/name) for the VLAN | ||
| // network the database VM attaches to. This is the VM's only network | ||
| // interface: client traffic and the Prometheus metrics scrape go through it. | ||
| // The NAD must already exist on the cluster (the controller does not create | ||
| // networks). No internet egress required — packages are pre-installed in the | ||
| // baked image. | ||
| // interface: client traffic, package install during cloud-init, and the | ||
| // Prometheus metrics scrape all go through it. The NAD must already exist | ||
| // on the cluster (the controller does not create networks) and the VLAN | ||
| // must have internet egress. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for controller-side enforcement of OSImage immutability, and whether the CRD yaml has been regenerated.
rg -n 'OSImage' --type=go -C3 database/internal
rg -n 'osImage' database/config/crd/bases/*.yamlRepository: wso2/open-cloud-datacenter
Length of output: 11576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CRB osImage entries =="
sed -n '190,220p' database/config/crd/bases/dbaas.opencloud.wso2.com_dbinstances.yaml
sed -n '328,405p' database/config/crd/bases/dbaas.opencloud.wso2.com_dbinstances.yaml
echo "== Ensure immutable drift preflight =="
sed -n '1,220p' database/internal/ensure/preflight.go
sed -n '75,95p' database/internal/ensure/defaults.go
echo "== Type declarations =="
sed -n '120,175p' database/api/v1alpha1/dbinstance_types.go
sed -n '320,345p' database/api/v1alpha1/dbinstance_types.go
echo "== Search for immutable reason =="
rg -n 'ReasonImmutableFieldChanged|immutableField|ImmutableField' --type=go database/internal database/api -C2Repository: wso2/open-cloud-datacenter
Length of output: 16606
Keep OSImage immutability in sync between CRD CEL and controller drift checks.
OSImage is included in AppliedSpec and immutablerDrift() now reports post-create drift, but it still lacks a CRD-level x-kubernetes-validations[0].rule: self == oldSelf guard like networkRef. Missing that CEL rule means immutable edits can pass API validation and then be handled as a terminal preflight error instead.
🤖 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/api/v1alpha1/dbinstance_types.go` around lines 157 - 169, Add the
CRD CEL immutability validation rule self == oldSelf to the OSImage schema,
matching the existing networkRef validation. Keep OSImage included in
AppliedSpec and immutablerDrift() so controller drift detection remains aligned
with the API-level guard.
| # 1. Install PostgreSQL + helpers. Done here, not via cloud-init's | ||
| # "packages:" directive, so it works on minimal cloud images | ||
| # that don't load the package module. | ||
| export DEBIAN_FRONTEND=noninteractive | ||
| apt-get update -y | ||
| apt-get install -y postgresql postgresql-contrib jq qemu-guest-agent prometheus-postgres-exporter | ||
| systemctl enable --now qemu-guest-agent | ||
|
|
||
| PG_VER=$(pg_lsclusters -h | awk '{print $1}' | head -1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not silently ignore Spec.EngineVersion.
This now installs the distribution-default postgresql package and derives its version from the resulting cluster. Meanwhile, reconciliation still records inst.Spec.EngineVersion as applied. A request for a specific engine version can therefore report success with a different server version. Restore explicit version selection, map the version to the selected image, or reject/remove the API field atomically.
🤖 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/credentials/cloudinit.go` around lines 199 - 207, Update
the cloud-init PostgreSQL installation flow around PG_VER so Spec.EngineVersion
is not silently ignored: explicitly install and configure the requested engine
version, or atomically remove/reject that API field while preserving
reconciliation state consistency. Ensure the recorded applied version matches
the server actually installed, rather than always deriving the distribution
default from pg_lsclusters.
| helper.changes, err = helper.calculateChanges(obj) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return kerrors.NewAggregate([]error{ | ||
| helper.patchStatusConditions(ctx, obj, patchOptions.OwnedConditions), | ||
| helper.patchStatus(ctx, obj), | ||
| }) | ||
| } | ||
|
|
||
| func (helper *Helper) patchStatus(ctx context.Context, obj client.Object) error { | ||
| if !helper.shouldPatch("status") { | ||
| return nil | ||
| } | ||
| before, after, err := helper.calculateStatusPatch(obj) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return helper.client.Status().Patch(ctx, after, client.MergeFrom(before)) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate patch.go =="
fd -a 'patch\.go$' . | sed 's#^\./##'
echo "== outline =="
ast-grep outline database/internal/patch/patch.go || true
echo "== relevant lines =="
cat -n database/internal/patch/patch.go | sed -n '80,210p'
echo "== searches for shouldPatch/calculateStatusPatch/focusedStatusCopy =="
rg -n "shouldPatch|calculateStatusPatch|focusedStatusCopy|isConditionsSetter|Patch\\(" database/internal/patch/patch.goRepository: wso2/open-cloud-datacenter
Length of output: 6628
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find controller-runtime dependencies =="
for f in go.mod go.sum; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n "sigs.k8s.io/controller-runtime|controller-runtime|jsonpatch" "$f" || true
fi
done
echo "== any local vendored/controller-runtime files =="
fd 'json.*\.go$|merge.*\.go$|patch.*\.go$' -t f . | rg 'vendor|mod|pkg/sig|controller|json' || trueRepository: wso2/open-cloud-datacenter
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect go.mod version =="
cat go.mod | sed -n '1,160p'
echo "== fetch controller-runtime MergeFrom implementation from module cache or proxy if available =="
python3 - <<'PY'
import json, os, pathlib, re, subprocess, tempfile, urllib.request
mod = pathlib.Path('go.mod').read_text()
match = re.search(r'sigs\.k8s\.io/controller-runtime\s+v?([^\s#]+)', mod)
if not match:
print("controller-runtime dependency not found in go.mod")
raise SystemExit(0)
version = match.group(1)
print("controller-runtime version:", version)
url = f"https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/v{version.strip('v')}/pkg/client/patch.go"
try:
data = urllib.request.urlopen(url, timeout=10).read().decode()
except Exception as e:
print("could not fetch raw file:", e)
raise SystemExit(0)
lines = data.splitlines()
for name, regex in [("type MergePatch", r"^type MergePatch struct"), ("func (f MergePatch) Data", r"^func \(f MergePatch\) Data"), ("func MergeFrom", r"^func MergeFrom")]:
print(f"--- {name} ---")
for i, line in enumerate(lines, 1):
if re.search(regex, line):
start = max(1, i-3); end = min(len(lines), i+25)
for j in range(start, end+1):
print(f"{j:4d}\t{lines[j-1]}")
break
PYRepository: wso2/open-cloud-datacenter
Length of output: 232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== top-level files =="
git ls-files | sed -n '1,120p'
echo "== locate files mentioning CalculateChanges/calculateChanges/focusedStatusCopy/helper.changes =="
rg -n "CalculateChanges|calculateChanges|focusedStatusCopy|helper\.changes|shouldPatch\\(\"status\"\\)|calculatePatchData" . --glob '!vendor/**' --glob '!dist/**' --glob '!build/**'
echo "== relevant helper implementation lines if available =="
for f in $(git ls-files | rg '(^|/)internal/patch/patch\.go$|(^|/)patch/patch\.go$'); do
echo "--- $f ---"
cat -n "$f" | sed -n '1,220p'
doneRepository: wso2/open-cloud-datacenter
Length of output: 13923
🌐 Web query:
controller-runtime MergeFrom Data empty merge patch jsonpatch
💡 Result:
In controller-runtime, client.MergeFrom calculates a JSON merge patch by comparing two versions of an object [1][2]. When the calculated patch is empty (i.e., no fields have changed), the resulting patch data is an empty JSON object {} [3]. Key considerations regarding empty merge patches include: API Server Interaction: Historically, calling Patch with an empty patch still results in a request to the Kubernetes API server [3]. While some developers implement wrappers to check if a patch is empty before calling the client to avoid unnecessary network traffic, controller-runtime itself typically performs the request unless optimized by the controller logic [3]. Zero Values: Because MergeFrom relies on a JSON-based diff of the serialized objects, it may not detect changes if a field's value is the zero value for its type, depending on how struct fields are tagged (e.g., omitempty) [4]. If a field is omitted from the JSON representation, the patch generator cannot distinguish between a field that was intentionally set to a zero value and one that was left unset [4]. Strategic Merge Patch vs. Merge Patch: MergeFrom generates a standard JSON merge patch [1][2]. If you require behavior specific to Strategic Merge Patch (which supports list merging via patchMergeKey), you should use client.StrategicMergeFrom instead [1][2][5]. Note that Strategic Merge Patch is not supported for Custom Resource Definitions (CRDs) [5]. Recommendation: To avoid unnecessary API calls, check if the object has been mutated before invoking the patch. If you must calculate the patch to check its content, you can compute it once, verify if it is empty, and only proceed with the patch request if it contains actual changes [3].
Citations:
- 1: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/patch.go
- 2: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/client/patch.go
- 3: ✨ Avoid contacting kube apiserver is patch is empty kubernetes-sigs/controller-runtime#2417
- 4:
mergeFromPatchdoes not consider the zero value as part of the patch kubernetes-sigs/controller-runtime#703 - 5: ✨ Add client.StrategicMergeFrom kubernetes-sigs/controller-runtime#1406
Skip patchStatus() when the focused status patch is empty.
shouldPatch("status") is set from the full status diff, so condition-only changes make it true, but calculateStatusPatch() strips .status.conditions for objects using the condition getter/setter path. That can result in an empty focused status patch being sent through Status().Patch(...) when conditions were already persisted separately. Skip this call when client.MergeFrom(before).Data(after) returns {}.
🤖 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/patch/patch.go` around lines 92 - 112, Update
Helper.patchStatus to build the client.MergeFrom(before) patch helper, inspect
its Data(after) result, and return nil when the focused status patch is empty;
only call helper.client.Status().Patch for non-empty data. Preserve the existing
shouldPatch, calculateStatusPatch, and error-handling flow.
2 participants
Summary
Replace whole-status optimistic-lock retrying with a Flux-derived status and condition patching design, implemented locally without introducing a Flux dependency.
This prepares status management for multiple controllers, preserves conditions owned by other controllers, and ensures final in-memory status changes are persisted through deferred patching.
Closes #239.
Changes
internal/conditionsutilities:internal/patchutilities:GetConditionsandSetConditionstoDBInstance.NotFounderrors after successful deletion.patchStatusIfChangedconflict-retry implementation.Testing
Verified with:
go test ./... go vet ./... git diff --checkAdded and updated tests covering:
All tests and validation completed successfully.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation