fix: Operator Size Increase - #311
Conversation
📝 WalkthroughWalkthroughChangesThe PR adds staged SeaweedFS topology migration, PVC expansion, and ClickHouse Keeper reconciliation. ClickHouse updates now wait for infrastructure readiness. Redis Sentinel becomes optional and defaults to enabled. StorageClass permissions support PVC expansion. Managed infrastructure
Redis Sentinel defaulting
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant SeaweedFS
participant StorageClass
participant PVC
Reconciler->>SeaweedFS: discover volume and filer targets
Reconciler->>StorageClass: check expansion support
Reconciler->>PVC: compare and patch requested capacity
PVC-->>Reconciler: report capacity convergence
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 10
🧹 Nitpick comments (7)
internal/controller/infra/managed/objectstore/seaweedfs/topology_test.go (3)
253-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe test name does not match the path it exercises.
reconcileTopologycallsseaweedComponentsAndWorkloadsReadybefore it evaluates the S3 rollout annotation. In this casedeploymentReadyreturns false becauseGenerationis 2 andObservedGenerationis 1. The reconcile therefore stops at the component-readiness gate, which is why the expected reason isWaitingForSeaweedComponents. The name states that the test covers the rollout gate before post-verification.Rename the test, or set
ObservedGenerationto 2 and change another rollout field so the assertion reachess3GatewayRolloutReady.🤖 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/infra/managed/objectstore/seaweedfs/topology_test.go` around lines 253 - 272, Rename the test around reconcileTopology to reflect that it exercises the seaweed component-readiness gate, not the S3 rollout-generation gate; keep the existing Generation and ObservedGeneration values and expectations unchanged.
55-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn named workloads instead of a positional slice.
Lines 174, 235, and 259 index into the returned slice and type-assert (
workloads[1].(*appsv1.StatefulSet),workloads[3].(*appsv1.Deployment)). Any change to the order inreadyWorkloadsbreaks those assertions at runtime with a panic rather than a clear failure.Return a struct with
master,volume,filer, ands3fields, and derive the[]client.Objectslice from it.🤖 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/infra/managed/objectstore/seaweedfs/topology_test.go` around lines 55 - 91, Update readyWorkloads to return a struct containing named master, volume, filer, and s3 workload fields, while also deriving the client.Object slice from those fields for client setup. Replace the positional indexing and type assertions at the affected test sites with the corresponding named fields, preserving the existing workload definitions and ordering in the derived slice.
113-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the refusal and failure paths.
The suite covers the happy path and two wait states. It does not cover three branches that change operator behavior:
ReplicaReductionUnsupportedwhentargetReplicas < actualReplicas(line 111 oftopology.go).UnexpectedReplicationPolicywhen the actual replication matches neither the stable nor the target value (line 177).- The Job failure branch, which returns
PreMigrationVerificationFailedorReplicationFailedand currently has no recovery path.Add cases for these branches.
🤖 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/infra/managed/objectstore/seaweedfs/topology_test.go` around lines 113 - 273, Extend the topology reconciliation tests with cases covering refusal when target replicas are below actual replicas, rejection when the actual replication matches neither stable nor target policy, and failed migration Jobs producing PreMigrationVerificationFailed or ReplicationFailed. Use the existing reconcileTopology, seaweed, completedJob, and readyWorkloads helpers, and assert the returned condition reasons plus unchanged topology/policy where applicable.internal/controller/infra/managed/clickhouse/altinity/keeper/topology.go (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the API error text in the condition.
Line 33 discards
errand sets an empty message.reconcileMembershipJobsetserr.Error()in the same situation at line 144. Without the message, an operator cannot tell an RBAC denial from a CRD-missing error.♻️ Proposed change
found, err := common.GetResource(ctx, cl, nsName, ResourceTypeName, actual) if err != nil { - return topologyCondition(metav1.ConditionUnknown, common.ApiErrorReason, "") + return topologyCondition(metav1.ConditionUnknown, common.ApiErrorReason, err.Error()) }🤖 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/infra/managed/clickhouse/altinity/keeper/topology.go` around lines 31 - 34, Update the error path in the topology reconciliation flow around GetResource to pass err.Error() as the condition message instead of an empty string, matching reconcileMembershipJob’s existing behavior while preserving the ApiErrorReason and Unknown condition status.internal/controller/reconciler/storage_expansion_test.go (1)
42-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a PVC without a storage request.
The
pvchelper always setsRequests[corev1.ResourceStorage].reconcileSeaweedFSVolumeExpansionpanics when that map is nil, as noted oninternal/controller/reconciler/storage_expansion.goline 118. Add a case that builds a PVC with a nilRequestsmap so the fix stays covered.🤖 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/reconciler/storage_expansion_test.go` around lines 42 - 66, Extend the test cases around the pvc helper to construct a PersistentVolumeClaim with Spec.Resources.Requests set to nil, then exercise reconcileSeaweedFSVolumeExpansion with that PVC and assert it does not panic and preserves the expected behavior. Keep the existing request-populated cases unchanged.internal/controller/infra/managed/objectstore/seaweedfs/topology.go (2)
409-424: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTreat a missing S3 Deployment as pending, not as an error.
s3GatewayRolloutReadyreturnsfalsewhen the S3 Deployment does not exist.migrateTopologythen callsrolloutS3Gateway, which returns theNotFounderror unchanged.migrateTopologyconverts that into a reconcile error instead of a wait condition.Ignore
NotFoundhere and let the caller reportRestartingS3Gateway.♻️ Proposed change
deployment := &appsv1.Deployment{} if err := kubeClient.Get(ctx, types.NamespacedName{ Namespace: seaweed.Namespace, Name: fmt.Sprintf("%s-s3", seaweed.Name), }, deployment); err != nil { + if apierrors.IsNotFound(err) { + return nil + } return err }🤖 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/infra/managed/objectstore/seaweedfs/topology.go` around lines 409 - 424, Update rolloutS3Gateway to detect a Kubernetes NotFound error from kubeClient.Get and return nil, allowing the caller to treat the missing Deployment as pending and report RestartingS3Gateway; continue returning other errors unchanged.
312-336: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate scheduling and security settings to the topology Job pod.
The Job pod copies only
ImagePullSecrets. It omits node affinity, tolerations, and security context. The equivalent Keeper Job ininternal/controller/infra/managed/clickhouse/altinity/keeper/topology.gocopiesAffinity,Tolerations,SecurityContext, and the containerSecurityContext. On clusters with restrictive Pod Security Standards or dedicated nodes, this Job can fail to schedule or fail admission.Copy the same fields from the SeaweedFS spec, and add resource requests.
🤖 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/infra/managed/objectstore/seaweedfs/topology.go` around lines 312 - 336, Update the topology Job pod in the Job construction block to propagate SeaweedFS scheduling and security settings: copy Affinity, Tolerations, Pod SecurityContext, and the topology container’s SecurityContext from the corresponding SeaweedFS spec, following the Keeper topology Job pattern. Also add the container resource requests from the SeaweedFS configuration while preserving the existing image and command settings.
🤖 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/infra/managed/clickhouse/altinity/keeper/topology.go`:
- Line 182: Add bounds checks in SetKeeperReplicas, configurationJob, and
membershipJob before indexing Spec.Configuration.Clusters, Templates, or
Containers. When required slices are missing or empty, return an error or report
the appropriate Ready/Condition and stop before patching replicas or
constructing jobs. Preserve the existing flow only when all required entries
exist.
In `@internal/controller/infra/managed/clickhouse/altinity/write.go`:
- Around line 73-94: Restrict the readiness gate in the ClickHouse
reconciliation flow to topology changes only, rather than comparing the entire
actualClickHouse.Spec with desired.Spec. Update the condition around
chPodsRunningStatus and computeClickHouseReportedReadyCondition to compare the
relevant cluster-layout fields, such as shard and replica counts, while allowing
unrelated spec corrections to apply immediately.
In `@internal/controller/infra/managed/objectstore/seaweedfs/topology.go`:
- Around line 255-292: The failed topology Jobs are never recoverable because
both controllers retain deterministic, failed Jobs indefinitely. In
internal/controller/infra/managed/objectstore/seaweedfs/topology.go lines
255-292, update reconcileTopologyJob’s JobFailed branch to delete the failed Job
after an appropriate backoff or otherwise generate a new Job name so
reconciliation can retry; in
internal/controller/infra/managed/clickhouse/altinity/keeper/topology.go lines
158-165, apply the same recovery behavior in reconcileMembershipJob before
returning KeeperMembershipFailed.
- Around line 360-371: Update replicationScript to enable pipeline failure
propagation with set -o pipefail before the weed shell verification pipeline,
ensuring a non-zero weed shell status causes the script to fail even when tee
succeeds. Apply the same pipefail setting to verificationScript as requested,
without changing the existing replication checks.
- Around line 460-471: Update reconcileTopologyJob and topologyJobName so the
Seaweed name is shortened before constructing the Job name, using
common.FitDefaultInfraName with the full "-swt-<stage>-<replication>" suffix and
the 63-character limit. Preserve the existing stage and replication tokens while
ensuring the resulting topology Job name fits Kubernetes label-name constraints.
In `@internal/controller/reconciler/readiness_test.go`:
- Around line 86-107: The TestInfrastructureBlockersIncludesDegradedClickHouse
test still uses testing.T; rewrite it using the repository’s Ginkgo/Gomega
conventions and assertions while preserving its existing setup and expected
blocker value. Remove standard testing-specific assertions or parameters as
needed, then run make lint and make test before completion.
In `@internal/controller/reconciler/reconcile_v2.go`:
- Around line 305-342: Remove the early returns from the expansionBlockers and
expansionPending branches in the main reconciliation flow. Keep reporting these
conditions through the object-store status using updateReadyStatus, but scope
the expansion gate to object-store reconciliation only; allow Redis, MySQL,
Kafka, ClickHouse, and ReconcileWandbManifest to continue executing.
In `@internal/controller/reconciler/storage_expansion_test.go`:
- Around line 18-25: Convert TestReconcileSeaweedFSVolumeExpansion and its
subtests from the standard testing package to Ginkgo/Gomega, replacing t.Run,
t.Fatal, and assertion calls with Describe/It structure and Gomega matchers.
Follow the existing suite_test.go/envtest setup and the Ginkgo style used by
topology_test.go, removing direct testing-package usage while preserving the
current test coverage and behavior.
In `@internal/controller/reconciler/storage_expansion.go`:
- Around line 91-121: Initialize pvc.Spec.Resources.Requests before assigning
the desired storage quantity in the expansion path around
storageClassAllowsExpansion and the subsequent Patch call. Preserve the existing
zero-value read and expansion checks, but ensure the map is non-nil before
updating corev1.ResourceStorage so PVCs without an existing request do not
panic.
- Around line 65-78: In reconcileSeaweedFSVolumeExpansion, validate each target
before the PVC lookup: when target.desired is non-empty, skip targets with an
empty target.namespace or target.name. Ensure malformed targets never reach the
client.List call or subsequent PVC patching, while preserving processing for
complete targets.
---
Nitpick comments:
In `@internal/controller/infra/managed/clickhouse/altinity/keeper/topology.go`:
- Around line 31-34: Update the error path in the topology reconciliation flow
around GetResource to pass err.Error() as the condition message instead of an
empty string, matching reconcileMembershipJob’s existing behavior while
preserving the ApiErrorReason and Unknown condition status.
In `@internal/controller/infra/managed/objectstore/seaweedfs/topology_test.go`:
- Around line 253-272: Rename the test around reconcileTopology to reflect that
it exercises the seaweed component-readiness gate, not the S3 rollout-generation
gate; keep the existing Generation and ObservedGeneration values and
expectations unchanged.
- Around line 55-91: Update readyWorkloads to return a struct containing named
master, volume, filer, and s3 workload fields, while also deriving the
client.Object slice from those fields for client setup. Replace the positional
indexing and type assertions at the affected test sites with the corresponding
named fields, preserving the existing workload definitions and ordering in the
derived slice.
- Around line 113-273: Extend the topology reconciliation tests with cases
covering refusal when target replicas are below actual replicas, rejection when
the actual replication matches neither stable nor target policy, and failed
migration Jobs producing PreMigrationVerificationFailed or ReplicationFailed.
Use the existing reconcileTopology, seaweed, completedJob, and readyWorkloads
helpers, and assert the returned condition reasons plus unchanged
topology/policy where applicable.
In `@internal/controller/infra/managed/objectstore/seaweedfs/topology.go`:
- Around line 409-424: Update rolloutS3Gateway to detect a Kubernetes NotFound
error from kubeClient.Get and return nil, allowing the caller to treat the
missing Deployment as pending and report RestartingS3Gateway; continue returning
other errors unchanged.
- Around line 312-336: Update the topology Job pod in the Job construction block
to propagate SeaweedFS scheduling and security settings: copy Affinity,
Tolerations, Pod SecurityContext, and the topology container’s SecurityContext
from the corresponding SeaweedFS spec, following the Keeper topology Job
pattern. Also add the container resource requests from the SeaweedFS
configuration while preserving the existing image and command settings.
In `@internal/controller/reconciler/storage_expansion_test.go`:
- Around line 42-66: Extend the test cases around the pvc helper to construct a
PersistentVolumeClaim with Spec.Resources.Requests set to nil, then exercise
reconcileSeaweedFSVolumeExpansion with that PVC and assert it does not panic and
preserves the expected behavior. Keep the existing request-populated cases
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a23a869-a3c3-41ad-b817-0d8e66ff5eb1
📒 Files selected for processing (35)
api/v2/weightsandbiases_types.goapi/v2/zz_generated.deepcopy.goconfig/crd/bases/apps.wandb.com_weightsandbiases.yamlconfig/rbac/role.yamldeploy/operator/templates/wandb-operator-wandb-role.yamlinternal/controller/common/state.gointernal/controller/infra/managed/clickhouse/altinity/keeper/read.gointernal/controller/infra/managed/clickhouse/altinity/keeper/read_test.gointernal/controller/infra/managed/clickhouse/altinity/keeper/spec.gointernal/controller/infra/managed/clickhouse/altinity/keeper/spec_test.gointernal/controller/infra/managed/clickhouse/altinity/keeper/topology.gointernal/controller/infra/managed/clickhouse/altinity/keeper/topology_test.gointernal/controller/infra/managed/clickhouse/altinity/keeper/write.gointernal/controller/infra/managed/clickhouse/altinity/read.gointernal/controller/infra/managed/clickhouse/altinity/status_test.gointernal/controller/infra/managed/clickhouse/altinity/write.gointernal/controller/infra/managed/clickhouse/altinity/write_test.gointernal/controller/infra/managed/objectstore/seaweedfs/status.gointernal/controller/infra/managed/objectstore/seaweedfs/status_test.gointernal/controller/infra/managed/objectstore/seaweedfs/topology.gointernal/controller/infra/managed/objectstore/seaweedfs/topology_test.gointernal/controller/infra/managed/objectstore/seaweedfs/write.gointernal/controller/infra/managed/objectstore/seaweedfs/write_test.gointernal/controller/infra/managed/redis/opstree/spec.gointernal/controller/infra/managed/redis/opstree/spec_test.gointernal/controller/reconciler/clickhouse.gointernal/controller/reconciler/readiness_test.gointernal/controller/reconciler/reconcile_v2.gointernal/controller/reconciler/storage_expansion.gointernal/controller/reconciler/storage_expansion_test.gointernal/controller/weightsandbiases_controller.gointernal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yamlinternal/webhook/v2/weightsandbiases_defaulter_redis_test.gointernal/webhook/v2/weightsandbiases_webhook.gointernal/webhook/v2/weightsandbiases_webhook_test.go
| name string, | ||
| replica int, | ||
| ) *batchv1.Job { | ||
| podSpec := desired.Spec.Templates.PodTemplates[0].Spec |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the desired ClickHouseKeeperInstallation always populates Templates.PodTemplates and Configuration.Clusters.
rg -nP -C6 'PodTemplates|Clusters\s*:' internal/controller/infra/managed/clickhouse/altinity/keeper --type=go
# Inspect the vendored type definitions for pointer vs value fields.
rg -nP -C3 'Templates\s+\*?|PodTemplates\s+\[\]|Clusters\s+\[\]' pkg/vendored/altinity-clickhouse --type=goRepository: wandb/operator
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file size =="
wc -l internal/controller/infra/managed/clickhouse/altinity/keeper/topology.go
echo "== imports and function outline =="
ast-grep outline internal/controller/infra/managed/clickhouse/altinity/keeper/topology.go || true
echo "== relevant topology.go sections =="
sed -n '150,285p' internal/controller/infra/managed/clickhouse/altinity/keeper/topology.go
echo "== spec generation relevant section =="
sed -n '50,105p' internal/controller/infra/managed/clickhouse/altinity/keeper/spec.go
echo "== callers of membershipJob / topology functions =="
rg -n -C3 'membershipJob|configurationJob|reconfigurationEnabled|ClusterName|KeeperImage|KeeperRunAsUser' internal/controller/infra/managed/clickhouse/altinity/keeper --type=goRepository: wandb/operator
Length of output: 18583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== full topology.go =="
cat -n internal/controller/infra/managed/clickhouse/altinity/keeper/topology.go
echo "== Go slice nil/short panic probe =="
python3 - <<'PY'
data = "fmt.Println(map[string]interface{}{\"nil\": func() string { var s []string; return fmt.Sprintf(\"%+v (%v): %v\") // placeholder } })"
print("Go behavior is runtime-dependent; source indexes into nil slices as len=0 so [0] panics.")
PYRepository: wandb/operator
Length of output: 11040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== vendored CHK spec template definitions =="
rg -n -C4 'type Templates\b|type TemplatesList\b|type Cluster\b|type Configuration\b|type ChkClusterLayout\b' pkg/vendored/altinity-clickhouse/clickhouse-keeper.altinity.com/v1 --type=go
echo "== functions around GetTemplates/GetConfiguration in vendored types =="
sed -n '30,80p' pkg/vendored/altinity-clickhouse/clickhouse-keeper.altinity.com/v1/type_spec.go
sed -n '310,355p' pkg/vendored/altinity-clickhouse/clickhouse-keeper.altinity.com/v1/type_chk.go
sed -n '35,50p' pkg/vendored/altinity-clickhouse/clickhouse-keeper.altinity.com/v1/type_configuration.goRepository: wandb/operator
Length of output: 7412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== vendor type_spec GetTemplates =="
sed -n '103,130p' pkg/vendored/altinity-clickhouse/clickhouse-keeper.altinity.com/v1/type_spec.go 2>/dev/null || sed -n '103,130p' pkg/vendored/altinity-clickhouse/clickhouse-keeper.altinity.com/v1/type_spec.go
echo "== all functions that mutate CHK Spec or call WriteState/ToKeeperVendorSpec =="
rg -n -C3 'WriteState|ReadState|ToKeeperVendorSpec|DeepCopyInto|DeepCopy|Update\(|Patch\(' internal/controller/infra/managed/clickhouse/altinity/keeper --type=go
echo "== Go nil/empty slice direct evidence from source =="
go run "cmd=echo; print" 2>/dev/null || true
printf '%s\n' 'Go source for Go slice subscript checks: panics for out-of-range when len=0.'Repository: wandb/operator
Length of output: 14501
Guard the fixed slice indexes in topology helpers.
SetKeeperReplicas, configurationJob, and membershipJob index into Spec.Configuration.Clusters, Templates, and Containers without bounds checks. If a persisted CHK is missing or has empty slices, the reconcile loop panics. Return an error or report a Ready/Condition before constructing the job or patching replicas.
Also applies to: topology.go:182, topology.go:219-221, topology.go:263
🤖 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/infra/managed/clickhouse/altinity/keeper/topology.go` at
line 182, Add bounds checks in SetKeeperReplicas, configurationJob, and
membershipJob before indexing Spec.Configuration.Clusters, Templates, or
Containers. When required slices are missing or empty, return an error or report
the appropriate Ready/Condition and stop before patching replicas or
constructing jobs. Preserve the existing flow only when all required entries
exist.
| if clickHouseFound && !common.JSONEqual(actualClickHouse.Spec, desired.Spec) { | ||
| podsRunning, err := chPodsRunningStatus(ctx, client, installationNsName.Namespace, actualClickHouse) | ||
| if err != nil { | ||
| results = append(results, metav1.Condition{ | ||
| Type: ClickHouseReportedReadyType, | ||
| Status: metav1.ConditionUnknown, | ||
| Reason: common.ApiErrorReason, | ||
| }) | ||
| return results | ||
| } | ||
| currentConditions := computeClickHouseReportedReadyCondition(ctx, actualClickHouse, podsRunning) | ||
| results = append(results, currentConditions...) | ||
| if !apimeta.IsStatusConditionTrue(currentConditions, ClickHouseReportedReadyType) { | ||
| results = append(results, metav1.Condition{ | ||
| Type: ClickHouseCustomResourceType, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: common.PendingCreateReason, | ||
| Message: "waiting for the current ClickHouse topology before applying the desired topology", | ||
| }) | ||
| return results | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
This gate blocks every ClickHouse spec change, not only topology growth.
The condition is clickHouseFound && !common.JSONEqual(actualClickHouse.Spec, desired.Spec). Any spec difference now requires the current installation to be fully ready before the operator applies the change. If the running installation is unhealthy because of its own spec, for example a bad image tag or an insufficient memory limit, the operator refuses to apply the corrected spec. The installation then stays broken and no reconcile can repair it.
Narrow the gate to the change that needs staging. Compare the cluster layout, for example shard and replica counts, instead of the whole spec.
🤖 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/infra/managed/clickhouse/altinity/write.go` around lines
73 - 94, Restrict the readiness gate in the ClickHouse reconciliation flow to
topology changes only, rather than comparing the entire actualClickHouse.Spec
with desired.Spec. Update the condition around chPodsRunningStatus and
computeClickHouseReportedReadyCondition to compare the relevant cluster-layout
fields, such as shard and replica counts, while allowing unrelated spec
corrections to apply immediately.
| func reconcileTopologyJob( | ||
| ctx context.Context, | ||
| kubeClient client.Client, | ||
| seaweed *seaweedv1.Seaweed, | ||
| owner client.Object, | ||
| stage, targetReplication, script string, | ||
| ) (topologyJobState, string, error) { | ||
| name := topologyJobName(seaweed.Name, stage, targetReplication) | ||
| job := &batchv1.Job{} | ||
| err := kubeClient.Get(ctx, types.NamespacedName{Namespace: seaweed.Namespace, Name: name}, job) | ||
| if err != nil && !apierrors.IsNotFound(err) { | ||
| return topologyJobPending, "", err | ||
| } | ||
| if apierrors.IsNotFound(err) { | ||
| job = topologyJob(seaweed, name, stage, targetReplication, script) | ||
| if err := controllerutil.SetOwnerReference(owner, job, kubeClient.Scheme()); err != nil { | ||
| return topologyJobPending, "", err | ||
| } | ||
| if err := kubeClient.Create(ctx, job); err != nil { | ||
| return topologyJobPending, "", err | ||
| } | ||
| return topologyJobPending, fmt.Sprintf("SeaweedFS topology %s job %s created", stage, name), nil | ||
| } | ||
|
|
||
| for _, condition := range job.Status.Conditions { | ||
| if condition.Type == batchv1.JobFailed && condition.Status == corev1.ConditionTrue { | ||
| message := condition.Message | ||
| if message == "" { | ||
| message = fmt.Sprintf("SeaweedFS topology %s job %s failed", stage, name) | ||
| } | ||
| return topologyJobFailed, message, nil | ||
| } | ||
| if condition.Type == batchv1.JobComplete && condition.Status == corev1.ConditionTrue { | ||
| return topologyJobSucceeded, fmt.Sprintf("SeaweedFS topology %s job %s succeeded", stage, name), nil | ||
| } | ||
| } | ||
| return topologyJobPending, fmt.Sprintf("waiting for SeaweedFS topology %s job %s", stage, name), nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
A failed migration Job blocks topology changes permanently in both controllers. Both controllers create a Job with a deterministic name and BackoffLimit: 0, then read JobFailed from the persisted Job on every later reconcile. Neither controller deletes or recreates the failed Job, so a single transient failure stops the topology change until an operator deletes the Job by hand.
internal/controller/infra/managed/objectstore/seaweedfs/topology.go#L255-L292: inreconcileTopologyJob, add a recovery path for theJobFailedbranch. Delete the failed Job after a backoff interval, or include a generation suffix intopologyJobNameso a new Job can be created.internal/controller/infra/managed/clickhouse/altinity/keeper/topology.go#L158-L165: apply the same recovery path inreconcileMembershipJobbefore it returnsKeeperMembershipFailed.
📍 Affects 2 files
internal/controller/infra/managed/objectstore/seaweedfs/topology.go#L255-L292(this comment)internal/controller/infra/managed/clickhouse/altinity/keeper/topology.go#L158-L165
🤖 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/infra/managed/objectstore/seaweedfs/topology.go` around
lines 255 - 292, The failed topology Jobs are never recoverable because both
controllers retain deterministic, failed Jobs indefinitely. In
internal/controller/infra/managed/objectstore/seaweedfs/topology.go lines
255-292, update reconcileTopologyJob’s JobFailed branch to delete the failed Job
after an appropriate backoff or otherwise generate a new Job name so
reconciliation can retry; in
internal/controller/infra/managed/clickhouse/altinity/keeper/topology.go lines
158-165, apply the same recovery behavior in reconcileMembershipJob before
returning KeeperMembershipFailed.
| func replicationScript(seaweed *seaweedv1.Seaweed, targetReplication string) string { | ||
| master := fmt.Sprintf("%s-master:%d", seaweed.Name, seaweedv1.MasterHTTPPort) | ||
| filer := fmt.Sprintf("%s-filer:%d", seaweed.Name, seaweedv1.FilerHTTPPort) | ||
| return fmt.Sprintf(`set -eu | ||
| printf 'lock\nvolume.configure.replication -replication=%s\nvolume.fix.replication -apply -doDelete=false\nunlock\n' | weed shell -master=%s -filer=%s | ||
| output=/tmp/seaweed-replication-check.out | ||
| echo "volume.fix.replication -verbose -doDelete=false" | weed shell -master=%s -filer=%s | tee "$output" | ||
| if grep -Eq 'under replicated|failed to place|not well placed|mismatch in topology' "$output"; then | ||
| exit 1 | ||
| fi | ||
| `, targetReplication, master, filer, master, filer) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
weed shell failures are masked in the replication script.
Line 366 pipes weed shell into tee. The shell uses the exit status of the last command in a pipeline, so set -eu does not stop the script when weed shell fails. If weed shell exits non-zero and writes nothing, grep -Eq finds no match and the script exits 0. The operator then treats the replication stage as successful and commits the new stable replication without verified replica placement.
Set pipefail and check the command status explicitly.
🛡️ Proposed fix
- return fmt.Sprintf(`set -eu
+ return fmt.Sprintf(`set -eu
+set -o pipefail
printf 'lock\nvolume.configure.replication -replication=%s\nvolume.fix.replication -apply -doDelete=false\nunlock\n' | weed shell -master=%s -filer=%s
output=/tmp/seaweed-replication-check.out
echo "volume.fix.replication -verbose -doDelete=false" | weed shell -master=%s -filer=%s | tee "$output"Apply the same set -o pipefail line to verificationScript.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func replicationScript(seaweed *seaweedv1.Seaweed, targetReplication string) string { | |
| master := fmt.Sprintf("%s-master:%d", seaweed.Name, seaweedv1.MasterHTTPPort) | |
| filer := fmt.Sprintf("%s-filer:%d", seaweed.Name, seaweedv1.FilerHTTPPort) | |
| return fmt.Sprintf(`set -eu | |
| printf 'lock\nvolume.configure.replication -replication=%s\nvolume.fix.replication -apply -doDelete=false\nunlock\n' | weed shell -master=%s -filer=%s | |
| output=/tmp/seaweed-replication-check.out | |
| echo "volume.fix.replication -verbose -doDelete=false" | weed shell -master=%s -filer=%s | tee "$output" | |
| if grep -Eq 'under replicated|failed to place|not well placed|mismatch in topology' "$output"; then | |
| exit 1 | |
| fi | |
| `, targetReplication, master, filer, master, filer) | |
| } | |
| func replicationScript(seaweed *seaweedv1.Seaweed, targetReplication string) string { | |
| master := fmt.Sprintf("%s-master:%d", seaweed.Name, seaweedv1.MasterHTTPPort) | |
| filer := fmt.Sprintf("%s-filer:%d", seaweed.Name, seaweedv1.FilerHTTPPort) | |
| return fmt.Sprintf(`set -eu | |
| set -o pipefail | |
| printf 'lock\nvolume.configure.replication -replication=%s\nvolume.fix.replication -apply -doDelete=false\nunlock\n' | weed shell -master=%s -filer=%s | |
| output=/tmp/seaweed-replication-check.out | |
| echo "volume.fix.replication -verbose -doDelete=false" | weed shell -master=%s -filer=%s | tee "$output" | |
| if grep -Eq 'under replicated|failed to place|not well placed|mismatch in topology' "$output"; then | |
| exit 1 | |
| fi | |
| `, targetReplication, master, filer, master, filer) | |
| } |
🤖 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/infra/managed/objectstore/seaweedfs/topology.go` around
lines 360 - 371, Update replicationScript to enable pipeline failure propagation
with set -o pipefail before the weed shell verification pipeline, ensuring a
non-zero weed shell status causes the script to fail even when tee succeeds.
Apply the same pipefail setting to verificationScript as requested, without
changing the existing replication checks.
| func targetJobToken(replication string) string { | ||
| return strings.ReplaceAll(replication, "0", "z") | ||
| } | ||
|
|
||
| func topologyJobName(seaweedName, stage, replication string) string { | ||
| stageToken := map[string]string{ | ||
| topologyJobPreVerify: "p", | ||
| topologyJobReplicate: "r", | ||
| topologyJobPostVerify: "v", | ||
| }[stage] | ||
| return fmt.Sprintf("%s-swt-%s-%s", seaweedName, stageToken, targetJobToken(replication)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the shared name-fitting helper and the SeaweedFS name source.
rg -nP -A12 'func FitDefaultInfraName' internal/controller/common
rg -nP -C3 'Name\s+string' api/v2 --glob '*types.go' | rg -n -B3 -A3 'ManagedObjectStore|ManagedInfraSpec'Repository: wandb/operator
Length of output: 1120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== topology.go relevant section =="
sed -n '420,490p' internal/controller/infra/managed/objectstore/seaweedfs/topology.go | nl -ba -v420
echo
echo "== imports and common naming usages in topology =="
rg -n -C2 'FitDefaultInfraName|topologyJobName|Job name|job-name|labels.*job' internal/controller/infra/managed/objectstore/seaweedfs/topology.go internal/controller/common/naming.go
echo
echo "== relevant objectstore SeaweedFS top level files =="
git ls-files internal/controller/infra/managed/objectstore/seaweedfs | sed -n '1,120p'
echo
echo "== SeaweedFS resource names in controller code =="
rg -n '\.GetName|spec\.Name|Topology|Job|common\.FitDefaultInfraName|ObjectStore' internal/controller/infra/managed/objectstore/seaweedfs/api internal/controller/infra/managed/objectstore/seaweedfs -g '*.go' | sed -n '1,160p'Repository: wandb/operator
Length of output: 227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== topology.go relevant section =="
sed -n '420,490p' internal/controller/infra/managed/objectstore/seaweedfs/topology.go
echo
echo "== imports and common naming usages in topology =="
rg -n -C2 'FitDefaultInfraName|topologyJobName|Job name|job-name|labels.*job' internal/controller/infra/managed/objectstore/seaweedfs/topology.go internal/controller/common/naming.go || true
echo
echo "== relevant objectstore SeaweedFS top level files =="
git ls-files internal/controller/infra/managed/objectstore/seaweedfs | sed -n '1,120p'
echo
echo "== SeaweedFS resource names in controller code =="
rg -n '\.GetName|spec\.Name|topologyJobName|common\.FitDefaultInfraName|ObjectStore|Topology|Job' internal/controller/infra/managed/objectstore/seaweedfs -g '*.go' | sed -n '1,220p'Repository: wandb/operator
Length of output: 28855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== topology.go imports and Job construction =="
sed -n '1,380p' internal/controller/infra/managed/objectstore/seaweedfs/topology.go
echo
echo "== common naming constraints/usages =="
sed -n '1,90p' internal/controller/common/naming.go
rg -n 'MaxSpecNameLength|FitDefaultInfraName\(' api internal/controller -g '*.go' | sed -n '1,160p'Repository: wandb/operator
Length of output: 17560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Seaweed object CR names =="
rg -n -C3 'type Seaweed struct|SeaweedObjectStoreSpec|Spec .*Seaweed' -g '*.go' pkg api internal/controller/infra/managed/objectstore/seaweedfs | sed -n '1,220p'
echo
echo "== SeaweedFS naming constants =="
sed -n '1,50p' internal/controller/infra/managed/objectstore/seaweedfs/naming.go
sed -n '80,150p' internal/controller/infra/managed/objectstore/seaweedfs/spec.goRepository: wandb/operator
Length of output: 13063
Clamp topology Job names before creating them.
reconcileTopologyJob passes seaweed.Name directly into topologyJobName, which appends a stage token and replication suffix. Kubernetes copies Job names into labels, including job-name, so long Seaweed names can make Job creation fail. Use the same label-name budgeting helper, such as common.FitDefaultInfraName(seaweed.Name, "-swt-<stage>-<replication>", 63).
🤖 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/infra/managed/objectstore/seaweedfs/topology.go` around
lines 460 - 471, Update reconcileTopologyJob and topologyJobName so the Seaweed
name is shortened before constructing the Job name, using
common.FitDefaultInfraName with the full "-swt-<stage>-<replication>" suffix and
the 63-character limit. Preserve the existing stage and replication tokens while
ensuring the resulting topology Job name fits Kubernetes label-name constraints.
| func TestInfrastructureBlockersIncludesDegradedClickHouse(t *testing.T) { | ||
| wandb := &apiv2.WeightsAndBiases{ | ||
| Spec: apiv2.WeightsAndBiasesSpec{ | ||
| ClickHouse: map[string]apiv2.ClickHouseSpec{ | ||
| apiv2.DefaultInstanceName: {ManagedClickHouse: &apiv2.ManagedClickHouseSpec{}}, | ||
| }, | ||
| }, | ||
| Status: apiv2.WeightsAndBiasesStatus{ | ||
| ClickHouseStatus: map[string]apiv2.ClickHouseInfraStatus{ | ||
| apiv2.DefaultInstanceName: { | ||
| WBInfraStatus: apiv2.WBInfraStatus{Ready: false, State: "Degraded"}, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| blockers := infrastructureBlockers(wandb) | ||
|
|
||
| if len(blockers) != 1 || blockers[0] != "clickhouse/default" { | ||
| t.Fatalf("unexpected infrastructure blockers: %#v", blockers) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate test file and suite =="
fd -a 'readiness_test.go|suite_test.go' . || true
echo
echo "== inspect readiness_test.go around referenced lines =="
if [ -f internal/controller/reconciler/readiness_test.go ]; then
nl -ba internal/controller/reconciler/readiness_test.go | sed -n '1,140p'
fi
echo
echo "== inspect nearby Ginkgo/Ginkgo test conventions =="
rg -n "Describe\\(|Context\\(|It\\(|Gomega|Ginkgo|func Test|t\\.Fatf|t\\.Fatall" internal/controller/reconciler -S || true
echo
echo "== make files/existence =="
fd -a '^Makefile$|makefile$' . || true
if [ -f Makefile ]; then
rg -n '^(lint|test):|lint|test' Makefile -A5 -B2 || true
fiRepository: wandb/operator
Length of output: 1606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate test file and suite =="
fd -a 'readiness_test.go|suite_test.go' . || true
echo
echo "== inspect readiness_test.go around referenced lines =="
if [ -f internal/controller/reconciler/readiness_test.go ]; then
nl -ba internal/controller/reconciler/readiness_test.go | sed -n '1,140p'
fi
echo
echo "== inspect nearby Ginkgo/Gomega test conventions =="
rg -n "Describe\\(|Context\\(|It\\(|Gomega|Ginkgo|func Test|t\\.Fatf|t\\.Fatall" internal/controller/reconciler -S || true
echo
echo "== make files/existence =="
fd -a '^Makefile$|makefile$' . || true
if [ -f Makefile ]; then
rg -n '^(lint|test):|lint|test' Makefile -A5 -B2 || true
fiRepository: wandb/operator
Length of output: 1606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect readiness_test.go around referenced lines =="
awk '{printf "%6d\t%s\n", NR, $0}' internal/controller/reconciler/readiness_test.go | sed -n '1,140p'
echo
echo "== v2_suite_test.go =="
awk '{printf "%6d\t%s\n", NR, $0}' internal/controller/reconciler/v2_suite_test.go | sed -n '1,220p'
echo
echo "== Makefile existence and linter/test targets =="
awk '{printf "%6d\t%s\n", NR, $0}' Makefile | sed -n '1,220p' | grep -Ei "^(lint|test):|lint|test" -A4 -B4 || trueRepository: wandb/operator
Length of output: 10580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact infrastructureBlockers usages and imports =="
rg -n "infrastructureBlockers|DefaultInstanceName|ClickHouseInfraStatus|WBInfraStatus" internal/controller/reconciler/api -S || true
echo
echo "== read reconciler files containing infrastructure blockers =="
for f in $(rg -l "infrastructureBlockers" internal/controller/reconciler -S); do
echo "--- $f ---"
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,220p'
done
echo
echo "== read-only analyzer: test package/test types =="
python3 - <<'PY'
from pathlib import Path
p = Path("internal/controller/reconciler/readiness_test.go")
txt = p.read_text()
checks = {
"uses Ginkgo/v2": ". \"github.com/onsi/ginkgo/v2\"" in txt,
"uses Gomega": ". \"github.com/onsi/gomega\"" in txt,
"uses testing.T": "testing.T" in txt,
"uses t.Fatalf": "t.Fatalf" in txt,
"has TestInfrastructureBlockersIncludesDegradedClickHouse": "func TestInfrastructureBlockersIncludesDegradedClickHouse(t *testing.T)" in txt,
}
for k, v in checks.items():
print(f"{k}: {v}")
PYRepository: wandb/operator
Length of output: 19913
Use the required test framework before merge.
internal/controller/reconciler/readiness_test.go still uses the standard testing.T style. Rewrite this test with Ginkgo/Gomega, and run make lint and make test before marking this task complete.
🤖 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/reconciler/readiness_test.go` around lines 86 - 107, The
TestInfrastructureBlockersIncludesDegradedClickHouse test still uses testing.T;
rewrite it using the repository’s Ginkgo/Gomega conventions and assertions while
preserving its existing setup and expected blocker value. Remove standard
testing-specific assertions or parameters as needed, then run make lint and make
test before completion.
Source: Coding guidelines
| expansionBlockers, expansionPending, err := reconcileSeaweedFSVolumeExpansion(ctx, client, wandb) | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("reconcile SeaweedFS volume expansion: %w", err) | ||
| } | ||
| if len(expansionBlockers) > 0 { | ||
| statusBefore := wandb.DeepCopy().Status | ||
| message := volumeExpansionBlockersMessage(expansionBlockers) | ||
| if err := updateReadyStatus( | ||
| ctx, | ||
| client, | ||
| wandb, | ||
| statusBefore, | ||
| false, | ||
| "StorageExpansionUnsupported", | ||
| message, | ||
| ); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| recorder.Event(wandb, corev1.EventTypeWarning, "StorageExpansionUnsupported", message) | ||
| return ctrl.Result{RequeueAfter: defaultRequeueDuration}, nil | ||
| } | ||
|
|
||
| if len(expansionPending) > 0 { | ||
| statusBefore := wandb.DeepCopy().Status | ||
| message := volumeExpansionPendingMessage(expansionPending) | ||
| if err := updateReadyStatus( | ||
| ctx, | ||
| client, | ||
| wandb, | ||
| statusBefore, | ||
| false, | ||
| "StorageExpansionInProgress", | ||
| message, | ||
| ); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| return ctrl.Result{RequeueAfter: defaultRequeueDuration}, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
A blocked or pending PVC expansion halts all other reconciliation.
Both branches return before line 346. That skips the write and read phases for Redis, MySQL, Kafka, object store, and ClickHouse, and it skips ReconcileWandbManifest. Two consequences follow:
- The blocker case is permanent. An unexpandable StorageClass never becomes expandable on its own. The operator then stops reconciling the entire deployment until a user edits the spec, so unrelated changes and failures are never repaired.
- The pending case can last a long time. Offline volume expansion requires a pod restart, and a stuck
FileSystemResizePendingcondition freezes the whole reconcile loop.
Scope the gate to the object store. Report the condition on the object-store status, and let the remaining infrastructure and the manifest continue to reconcile.
🤖 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/reconciler/reconcile_v2.go` around lines 305 - 342,
Remove the early returns from the expansionBlockers and expansionPending
branches in the main reconciliation flow. Keep reporting these conditions
through the object-store status using updateReadyStatus, but scope the expansion
gate to object-store reconciliation only; allow Redis, MySQL, Kafka, ClickHouse,
and ReconcileWandbManifest to continue executing.
| func TestReconcileSeaweedFSVolumeExpansion(t *testing.T) { | ||
| scheme := runtime.NewScheme() | ||
| if err := corev1.AddToScheme(scheme); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := storagev1.AddToScheme(scheme); err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Convert this test to Ginkgo/Gomega.
This file uses the standard testing package with t.Run subtests. The other new test in this change, internal/controller/infra/managed/objectstore/seaweedfs/topology_test.go, uses Ginkgo and Gomega.
As per coding guidelines: "Use Ginkgo/Gomega for tests; test suites are configured through suite_test.go files and envtest."
🤖 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/reconciler/storage_expansion_test.go` around lines 18 -
25, Convert TestReconcileSeaweedFSVolumeExpansion and its subtests from the
standard testing package to Ginkgo/Gomega, replacing t.Run, t.Fatal, and
assertion calls with Describe/It structure and Gomega matchers. Follow the
existing suite_test.go/envtest setup and the Ginkgo style used by
topology_test.go, removing direct testing-package usage while preserving the
current test coverage and behavior.
Source: Coding guidelines
| for _, target := range seaweedFSVolumeTargets(wandb) { | ||
| if target.desired == "" { | ||
| continue | ||
| } | ||
|
|
||
| pvcs, ok := pvcsByNamespace[target.namespace] | ||
| if !ok { | ||
| pvcList := &corev1.PersistentVolumeClaimList{} | ||
| if err := c.List(ctx, pvcList, client.InNamespace(target.namespace)); err != nil { | ||
| return nil, nil, fmt.Errorf("list PVCs in namespace %q: %w", target.namespace, err) | ||
| } | ||
| pvcs = pvcList.Items | ||
| pvcsByNamespace[target.namespace] = pvcs | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether ManagedObjectStore Namespace and Name are defaulted or required.
rg -nP -C4 'Namespace\s+string|Name\s+string' api/v2 --glob '*types.go' | rg -n -B6 -A6 'ManagedInfraSpec|ManagedObjectStoreSpec'
rg -nP -C6 'ManagedObjectStore' internal/webhook/v2 --type=goRepository: wandb/operator
Length of output: 34209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== storage_expansion.go outline =="
ast-grep outline internal/controller/reconciler/storage_expansion.go --view compact || true
echo
echo "== storage_expansion.go relevant section =="
cat -n internal/controller/reconciler/storage_expansion.go | sed -n '1,180p'
echo
echo "== ObjectStore validation/error references =="
rg -n -C5 'ManagedObjectStore.*Name|Name.*ManagedObjectStore|Namespace.*ManagedObjectStore|defaultSpec|DefaultSpecName|cannot be deployed|objectStore' internal controller api --glob '*.go' | sed -n '1,260p'Repository: wandb/operator
Length of output: 33480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DefaultSpecName definition =="
rg -n -C4 'func .*DefaultSpecName|DefaultSpecName\(' --glob '*.go'
echo
echo "== reconcileSeaweedFSVolumeExpansion callers =="
rg -n -C4 'reconcileSeaweedFSVolumeExpansion|reconcile.*VolumeExpansion|VolumeExpansion' internal --glob '*.go'
echo
echo "== Reconcile path around webhook/defaulting =="
rg -n -C8 'Default\(|reconcile|return.*reconcil|SetupWithManager' internal/controller internal/webhook --glob '*.go' | sed -n '1,240p'
echo
echo "== Kubernetes controller-runtime client InNamespace implementation in vendored dependencies =="
for f in \
$(find . -path '*controller-runtime*pkg/client/list.go' -o -path '*controller-runtime*pkg/client/list.go' 2>/dev/null | head -10) \
$(find . -path '*/pkg/client/list.go' -o -path '*client/list.go' -mod=readonly 2>/dev/null | sed -n '1,10p')
do
[ -z "$f" ] && continue
echo "--- $f ---"
rg -n -C6 'func .*InNamespace|InNamespace|opts.*Namespace' "$f" | sed -n '1,120p'
done
echo
echo "== module/vendor presence =="
(
fd -a 'controller-runtime|client|go.sum|go.mod' . | sed -n '1,120p'
grep -n 'sigs.k8s.io/controller-runtime' go.mod 2>/dev/null || true
)Repository: wandb/operator
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate DefaultSpecName and related symbols =="
rg -n -C5 'DefaultSpecName|seaweedfs\.' --glob '*.go' .
echo
echo "== Locate storage_expansion references =="
rg -n -C6 'reconcileSeaweedFSVolumeExpansion|StorageExpansion|InNamespace\(""\)' --glob '*.go' .
echo
echo "== Webhook default usage =="
rg -n -C6 'weightsandbiases_defaulter|Default\(ctx|applyObjectStoreDefaults|reconcile.*VolumeExpansion' internal controller --glob '*.go' | sed -n '1,240p'
echo
echo "== Local deps/config hints =="
ls -la | sed -n '1,60p'
if [ -f go.mod ]; then
grep -n 'controller-runtime|k8s.io/api' go.mod | sed -n '1,60p' || true
fi
if [ -f go.sum ]; then
grep -n 'controller-runtime|k8s.io/api' go.sum | sed -n '1,40p' || true
fiRepository: wandb/operator
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Controller Reconcile section =="
cat -n internal/controller/weightsandbiases_controller.go | sed -n '180,360p'
echo
echo "== ObjectStore Write section =="
cat -n internal/controller/reconciler/objectstore.go | sed -n '150,220p'
echo
echo "== SeaweedFS naming section =="
cat -n pkg/vendored/seaweedfs-operator/seaweed.seaweedfs.com/v1/naming.go | sed -n '1,80p'
echo
echo "== Common FitDefaultInfraName section =="
rg -n -C4 'func FitDefaultInfraName|FitDefaultInfraName|InstanceBaseName' internal/controller/common pkg -g '*.go'
for f in $(rg -l 'FitDefaultInfraName|InstanceBaseName' internal/controller/common pkg -g '*.go'); do
echo "--- $f ---"
cat -n "$f" | sed -n '1,160p'
done
echo
echo "== Reconcile webhook/defaulting references only =="
rg -n -C3 'Default\(|Validation|Validating|Mutating|ApplyInfraSizing|reconcileSeaweedFSVolumeExpansion' internal/controller internal/webhook --glob '*.go' | sed -n '1,180p'
echo
echo "== storage expansion tests =="
cat -n internal/controller/reconciler/storage_expansion_test.go | sed -n '1,180p'Repository: wandb/operator
Length of output: 5285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file stats =="
for f in internal/controller/weightsandbiases_controller.go internal/controller/reconciler/objectstore.go internal/webhook/v2/weightsandbiases_webhook.go; do
[ -f "$f" ] && wc -l "$f"
done
echo
echo "== Relevant reconcile line ranges =="
if [ -f internal/controller/weightsandbiases_controller.go ]; then
wc -l internal/controller/weightsandbiases_controller.go
cat -n internal/controller/weightsandbiases_controller.go | sed -n '220,360p'
fi
echo
echo "== SeaweedFS naming candidates =="
fd -a 'naming.go' . | xargs -r -n1 sh -c 'echo "--- $0 ---"; wc -l "$0"; cat -n "$0" | sed -n "1,120p"'
echo
echo "== storage expansion source tail =="
cat -n internal/controller/reconciler/storage_expansion.go | sed -n '165,210p'
echo
echo "== all storage expansion calls =="
rg -n 'reconcileSeaweedFSVolumeExpansion|volumeExpansion|StorageExpansion' internal/controller internal/webhook -g '*.go'Repository: wandb/operator
Length of output: 27870
🏁 Script executed:
#!/bin/bash
set -u
echo "== reconcile_v2 section around storage expansion =="
cat -n internal/controller/reconciler/reconcile_v2.go | sed -n '260,350p'
echo
echo "== webhook default call sites =="
rg -n -C3 'WeightsAndBiasesDefault|Default\(|ApplyInfraSizing' internal/controller internal/webhook --glob '*.go' | sed -n '1,220p'
echo
echo "== package declarations for DefaultSpecName imports =="
rg -n -C2 'seaweedfs\.DefaultSpecName|moco\.DefaultSpecName|opstree\.DefaultSpecName|bufstream\.DefaultSpecName' internal/webhook/v2/weightsandbiases_webhook.goRepository: wandb/operator
Length of output: 25242
🌐 Web query:
controller-runtime client.inNamespaceList List all namespaces empty namespace InNamespace behavior
💡 Result:
In controller-runtime, the InNamespace(namespace string) function is used to restrict List and DeleteAllOf operations to a specific namespace [1][2]. The behavior regarding empty namespace strings and scope is as follows: 1. Scope Validation: The controller-runtime client enforces scoping rules. If you attempt to use InNamespace with a cluster-scoped (non-namespaced) resource, the client will return an error rather than silently ignoring it or performing an unintended operation [3]. 2. Empty Namespace String: - When calling List without the InNamespace option, the client performs a cluster-wide operation (if the resource is namespaced) or a normal cluster-scoped list (if the resource is cluster-scoped) [3]. - If you explicitly provide an empty string to InNamespace(string("")), it is functionally equivalent to not setting the namespace filter at all in many contexts, but passing an explicit namespace string is the standard way to constrain operations. - Historically, some confusion existed where InNamespace with an empty string might behave inconsistently with cluster-scoped resources, but modern versions of controller-runtime explicitly reject namespace filters for cluster-scoped resources to prevent accidental cluster-wide actions [3]. 3. Best Practice: - To list resources across all namespaces, do not include the InNamespace option in your ListOptions [3]. - To list resources in a specific namespace, provide the non-empty string to InNamespace [1][2]. - Ensure that the object type you are listing is actually namespaced before applying the InNamespace filter to avoid validation errors [3][4].
Citations:
- 1: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/options.go
- 2: https://master.sdk.operatorframework.io/docs/building-operators/golang/references/client/
- 3: 🐛 Reject namespace filters for cluster-scoped resources kubernetes-sigs/controller-runtime#3556
- 4: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/client/namespaced_client.go
🌐 Web query:
controller-runtime list.go inNamespaceList InNamespace namespace ==
💡 Result:
In controller-runtime, the InNamespace function is a ListOption used to restrict List and DeleteAllOf operations to a specific Kubernetes namespace [1]. When passed to these operations, it sets the namespace field in the underlying client options [1]. Recent versions of controller-runtime have improved the safety of this option by validating the scope of the target resource [2]. If you use InNamespace with a cluster-scoped (non-namespaced) resource, the operation will now return an error rather than silently failing or performing an unintended cluster-wide operation [2]. The implementation generally involves checking if the object is namespace-scoped using the RESTMapper [3]. If it is namespace-scoped, the provided namespace is applied to the operation options [3]. If it is cluster-scoped and a namespace is provided, the operation is rejected to prevent accidental scope escalation [2]. For example, when using a namespaced client wrapper, the implementation logic typically looks like this [3]: isNamespaceScoped, err:= n.IsObjectNamespaced(obj) if err!= nil { return fmt.Errorf("error finding the scope of the object: %w", err) } if isNamespaceScoped && n.namespace!= "" { opts = append(opts, InNamespace(n.namespace)) } return n.client.List(ctx, obj, opts...)
Citations:
- 1: https://sdk.operatorframework.io/docs/building-operators/golang/references/client/
- 2: 🐛 Reject namespace filters for cluster-scoped resources kubernetes-sigs/controller-runtime#3556
- 3: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/client/namespaced_client.go
🌐 Web query:
sigs.k8s.io/controller-runtime pkg client options.go list.go inNamespaceList InNamespace
💡 Result:
In the sigs.k8s.io/controller-runtime/pkg/client package, InNamespace is a functional option type used to restrict List and DeleteAllOf operations to a specific Kubernetes namespace [1][2][3]. Technical Details: - Definition: InNamespace is defined as a string type (type InNamespace string) [1][2]. - Functionality: It implements the ListOption and DeleteAllOfOption interfaces via an ApplyToList method, which updates the Namespace field in the ListOptions struct [1][2]. - Usage: It is passed as a variadic argument to client.List or client.DeleteAllOf [4][3][5]. Example: client.InNamespace("my-namespace") [5]. Important Behavioral Notes: - Validation: Recent versions of controller-runtime explicitly reject the use of InNamespace for cluster-scoped (non-namespaced) resources by returning an error, preventing accidental cluster-wide operations that previously might have occurred or been ignored [6]. - Integration: It is commonly used within reconcilers when interacting with namespaced resources [3]. When using a namespaced client (which is already scoped to a specific namespace), the client may automatically apply an InNamespace option to calls if the object being operated on is namespace-scoped [4].
Citations:
- 1: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/options.go
- 2: https://github.com/openshift/compliance-operator/blob/master/vendor/sigs.k8s.io/controller-runtime/pkg/client/options.go
- 3: https://master.sdk.operatorframework.io/docs/building-operators/golang/references/client/
- 4: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/client/namespaced_client.go
- 5: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/example_test.go
- 6: 🐛 Reject namespace filters for cluster-scoped resources kubernetes-sigs/controller-runtime#3556
🌐 Web query:
github.com/kubernetes-sigs/controller-runtime pkg client options.go type InNamespace string ApplyToList Namespace
💡 Result:
In the kubernetes-sigs/controller-runtime library, the InNamespace type and its associated ApplyToList method are defined in pkg/client/options.go [1][2][3]. The InNamespace type is defined as a string [1][2][3]: type InNamespace string It implements the ListOption interface by providing an ApplyToList method [1][2][3]. When used, this method sets the Namespace field in the provided ListOptions structure to the value of the InNamespace string [1][2][3]: func (n InNamespace) ApplyToList(opts *ListOptions) { opts.Namespace = string(n) } This mechanism allows developers to restrict list or delete operations to a specific namespace by passing client.InNamespace("my-namespace") as an option to functions like List or DeleteAllOf [4][5]. Because ListOptions are processed by applying these options sequentially, using InNamespace effectively sets the namespace field in the underlying Kubernetes API list request [1][2][3].
Citations:
- 1: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/options.go
- 2: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/client/options.go
- 3: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/options.go
- 4: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/example_test.go
- 5: https://master.sdk.operatorframework.io/docs/building-operators/golang/references/client/
Skip malformed object-store expansion targets before listing PVCs.
seaweedFSVolumeTargets writes spec.Name and spec.Namespace directly into targets at storage_expansion.go:165-184. If either field is empty, reconcileSeaweedFSVolumeExpansion can list PVCs across all namespaces under a matching label and patch PVCs in unrelated namespaces at lines 73 and 119. Reject or skip targets where target.desired != "" && (target.namespace == "" || target.name == "").
🤖 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/reconciler/storage_expansion.go` around lines 65 - 78, In
reconcileSeaweedFSVolumeExpansion, validate each target before the PVC lookup:
when target.desired is non-empty, skip targets with an empty target.namespace or
target.name. Ensure malformed targets never reach the client.List call or
subsequent PVC patching, while preserving processing for complete targets.
| current := pvc.Spec.Resources.Requests[corev1.ResourceStorage] | ||
| if desired.Cmp(current) > 0 { | ||
| storageClassName := "" | ||
| if pvc.Spec.StorageClassName != nil { | ||
| storageClassName = *pvc.Spec.StorageClassName | ||
| } | ||
| expandable, err := storageClassAllowsExpansion(ctx, c, storageClasses, storageClassName) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf( | ||
| "check StorageClass for PVC %s/%s: %w", | ||
| pvc.Namespace, | ||
| pvc.Name, | ||
| err, | ||
| ) | ||
| } | ||
| if !expandable { | ||
| blockers = append(blockers, volumeExpansionBlocker{ | ||
| component: "objectStore/" + target.component, | ||
| pvc: client.ObjectKeyFromObject(pvc), | ||
| storageClass: storageClassName, | ||
| current: current, | ||
| desired: desired, | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| before := pvc.DeepCopy() | ||
| pvc.Spec.Resources.Requests[corev1.ResourceStorage] = desired | ||
| if err := c.Patch(ctx, pvc, client.MergeFrom(before)); err != nil { | ||
| return nil, nil, fmt.Errorf("expand PVC %s/%s: %w", pvc.Namespace, pvc.Name, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Assignment to a nil Requests map panics.
Line 91 reads the storage request with a map index. When pvc.Spec.Resources.Requests is nil, that read returns a zero Quantity and does not panic. Line 92 then evaluates desired.Cmp(current) > 0 as true for any non-zero desired size. Line 118 assigns into the same nil map, and Go panics with "assignment to entry in nil map". That panic crashes the reconcile worker.
A PVC without a storage request is unusual but valid in the API, so this path is reachable.
🐛 Proposed fix
before := pvc.DeepCopy()
+ if pvc.Spec.Resources.Requests == nil {
+ pvc.Spec.Resources.Requests = corev1.ResourceList{}
+ }
pvc.Spec.Resources.Requests[corev1.ResourceStorage] = desired📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| current := pvc.Spec.Resources.Requests[corev1.ResourceStorage] | |
| if desired.Cmp(current) > 0 { | |
| storageClassName := "" | |
| if pvc.Spec.StorageClassName != nil { | |
| storageClassName = *pvc.Spec.StorageClassName | |
| } | |
| expandable, err := storageClassAllowsExpansion(ctx, c, storageClasses, storageClassName) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf( | |
| "check StorageClass for PVC %s/%s: %w", | |
| pvc.Namespace, | |
| pvc.Name, | |
| err, | |
| ) | |
| } | |
| if !expandable { | |
| blockers = append(blockers, volumeExpansionBlocker{ | |
| component: "objectStore/" + target.component, | |
| pvc: client.ObjectKeyFromObject(pvc), | |
| storageClass: storageClassName, | |
| current: current, | |
| desired: desired, | |
| }) | |
| continue | |
| } | |
| before := pvc.DeepCopy() | |
| pvc.Spec.Resources.Requests[corev1.ResourceStorage] = desired | |
| if err := c.Patch(ctx, pvc, client.MergeFrom(before)); err != nil { | |
| return nil, nil, fmt.Errorf("expand PVC %s/%s: %w", pvc.Namespace, pvc.Name, err) | |
| } | |
| current := pvc.Spec.Resources.Requests[corev1.ResourceStorage] | |
| if desired.Cmp(current) > 0 { | |
| storageClassName := "" | |
| if pvc.Spec.StorageClassName != nil { | |
| storageClassName = *pvc.Spec.StorageClassName | |
| } | |
| expandable, err := storageClassAllowsExpansion(ctx, c, storageClasses, storageClassName) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf( | |
| "check StorageClass for PVC %s/%s: %w", | |
| pvc.Namespace, | |
| pvc.Name, | |
| err, | |
| ) | |
| } | |
| if !expandable { | |
| blockers = append(blockers, volumeExpansionBlocker{ | |
| component: "objectStore/" + target.component, | |
| pvc: client.ObjectKeyFromObject(pvc), | |
| storageClass: storageClassName, | |
| current: current, | |
| desired: desired, | |
| }) | |
| continue | |
| } | |
| before := pvc.DeepCopy() | |
| if pvc.Spec.Resources.Requests == nil { | |
| pvc.Spec.Resources.Requests = corev1.ResourceList{} | |
| } | |
| pvc.Spec.Resources.Requests[corev1.ResourceStorage] = desired | |
| if err := c.Patch(ctx, pvc, client.MergeFrom(before)); err != nil { | |
| return nil, nil, fmt.Errorf("expand PVC %s/%s: %w", pvc.Namespace, pvc.Name, err) | |
| } |
🤖 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/reconciler/storage_expansion.go` around lines 91 - 121,
Initialize pvc.Spec.Resources.Requests before assigning the desired storage
quantity in the expansion path around storageClassAllowsExpansion and the
subsequent Patch call. Preserve the existing zero-value read and expansion
checks, but ensure the map is non-nil before updating corev1.ResourceStorage so
PVCs without an existing request do not panic.
1 participant
Changes are listed below
devsize implicitly controlled Sentinel, sodev→smallattempted a forbidden Redis topology change. We now preserve Redis topology across size changes.Changes: 8bfaca9
Changes: 3377328
ClickHouse waits during this process because it stores data in SeaweedFS.
Changes: 91f4458
4. Keeper replicas are Raft members; creating additional pods alone does not add them safely to the quorum. The Operator now enables dynamic reconfiguration, adds one Keeper replica, commits it through a membership Job, waits for the ensemble to become healthy, and then repeats for the next replica.
Changes: 3ad7bf4
5. Prevent ClickHouse from scaling in the same reconciliation as Keeper. It waits until Keeper reaches its complete topology and also requires the existing ClickHouse topology to be healthy before applying the new one and scale.
Changes: 2042b24
6. During a 1→3 scale-up, the single existing pod could therefore make the service appear ready before the other two pods existed. Readiness now calculates the expected pod count from the desired Keeper and ClickHouse topology, then requires every expected pod to be reported and running. Degraded and Unknown infrastructure states also keep the overall W&B resource unready.
Changes: c28c8d9
Summary by CodeRabbit