helm-converter: support migrate - #2412
Closed
AndrewChubatiuk wants to merge 1 commit into
Closed
Conversation
AndrewChubatiuk
marked this pull request as draft
July 17, 2026 06:18
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 79 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmd/helm-converter/main.go">
<violation number="1" location="cmd/helm-converter/main.go:190">
P2: NoDowntime migration can hang forever if the old storage's force-merge endpoint never completes. The default HTTP client and background context provide no request deadline; use a client with a bounded timeout (or a deadline-bearing context) for these calls.</violation>
</file>
<file name="internal/podutil/metrics.go">
<violation number="1" location="internal/podutil/metrics.go:88">
P3: Samples whose requested label is present but empty are silently dropped. Track label presence separately from its value and retain it under the empty-string map key; still skip samples where the label is absent.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| if err != nil { | ||
| return fmt.Errorf("cannot parse value %q: %w", line, err) | ||
| } | ||
| if len(dimValue) > 0 { |
Contributor
There was a problem hiding this comment.
P3: Samples whose requested label is present but empty are silently dropped. Track label presence separately from its value and retain it under the empty-string map key; still skip samples where the label is absent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/podutil/metrics.go, line 88:
<comment>Samples whose requested label is present but empty are silently dropped. Track label presence separately from its value and retain it under the empty-string map key; still skip samples where the label is absent.</comment>
<file context>
@@ -0,0 +1,257 @@
+ if err != nil {
+ return fmt.Errorf("cannot parse value %q: %w", line, err)
+ }
+ if len(dimValue) > 0 {
+ values[dimValue] = v
+ }
</file context>
Contributor
Author
There was a problem hiding this comment.
is this relevant still?
Contributor
There was a problem hiding this comment.
@AndrewChubatiuk Yes, it's still relevant. The if len(dimValue) > 0 guard in unmarshalMetric is still there, and it conflates two cases:
- label absent →
dimValue == ""(correct to skip) - label present but empty, e.g.
{dimension=""}→dimValue == ""(also silently dropped)
So any sample where the requested dimension exists with an empty value gets thrown away even though it should map to the "" key. If that matters for your use case, track presence separately and only skip samples where the label is truly missing:
dimValue, ok := tags[dimension]
...
if ok {
values[dimValue] = v
}That said, whether it's worth fixing depends on whether an empty dimension value is a real case for the metrics you're scraping. If it can't occur, it's fine to dismiss as P3.
Tip: get faster answers by chatting with cubic’s review copilot in the review UI
AndrewChubatiuk
force-pushed
the
converter-migrate
branch
2 times, most recently
from
July 28, 2026 15:04
2f9b139 to
f7bdb2c
Compare
AndrewChubatiuk
force-pushed
the
converter-migrate
branch
5 times, most recently
from
August 17, 2026 19:01
e868223 to
ee86a4c
Compare
Contributor
Author
|
@cubic-ai-dev |
Contributor
@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
10 issues found across 84 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/podutil/discover.go">
<violation number="1" location="internal/podutil/discover.go:21">
P3: This function duplicates `GetMetricsAddrs`'s endpoint-discovery loop, and the copies have already diverged on `Ready == nil` handling. Refactor `GetMetricsAddrs` to delegate to this helper or extract one shared implementation so endpoint semantics stay consistent.</violation>
</file>
<file name="docs/CHANGELOG.md">
<violation number="1" location="docs/CHANGELOG.md:23">
P3: The `## tip` section now contains two notes both numbered `Update note 1` (line 16 for the validation-webhook note and this new note for the `useLegacyNaming` removal). Number the new note 2 so readers and the merge workflow keep a stable reference to each note.</violation>
</file>
<file name="internal/migrate/snapshot.go">
<violation number="1" location="internal/migrate/snapshot.go:45">
P1: After a failed migration is rolled back, a retry reuses this fixed-name snapshot and can create the target from stale data, losing writes accepted by the old PVC meanwhile. Use a fresh or source-validated snapshot per attempt, and handle any already-restored target PVC consistently instead of blindly resuming.</violation>
<violation number="2" location="internal/migrate/snapshot.go:57">
P2: When the snapshot API returns an error or the CSI controller sets `status.error`, this loop silently polls until the 15-minute timeout. Return non-NotFound GET errors and surface the snapshot status error so migration fails immediately with the actual cause.</violation>
<violation number="3" location="internal/migrate/snapshot.go:79">
P1: When the source StorageClass uses `WaitForFirstConsumer`, this restored PVC remains Pending until a target pod is scheduled. The migration waits for Bound before creating the target CR, so NoDowntime migrations time out; create the target workload before waiting for binding or use a provisioning class that does not require a consumer.</violation>
</file>
<file name="internal/migrate/vl/agent.go">
<violation number="1" location="internal/migrate/vl/agent.go:21">
P2: When `--agent-buffer-size` is `0` or negative, this check returns a VLAgent instead of rejecting the non-positive capacity. Reject quantities with `size.Sign() <= 0` before building the CR.</violation>
</file>
<file name="internal/migrate/vm/nodowntime.go">
<violation number="1" location="internal/migrate/vm/nodowntime.go:80">
P1: During the snapshot and backfill window, this redirects all requests—not only writes—to the ingest-only VMAgent. Queries through the existing VMSingle Service therefore fail or become unavailable, contradicting the No Downtime strategy; preserve the old read endpoint while routing writes through a separate path-aware proxy or endpoint.</violation>
</file>
<file name="internal/migrate/vm/nodowntime_cluster.go">
<violation number="1" location="internal/migrate/vm/nodowntime_cluster.go:110">
P1: After this selector cutover, existing keep-alive connections and in-flight writes to old vminsert remain active; deleting the alias does not quiesce them. Stop or make old vminsert read-only, then wait for active writes to finish before snapshotting.</violation>
<violation number="2" location="internal/migrate/vm/nodowntime_cluster.go:114">
P2: The old vminsert Deployment is never scaled down before vmstorage's PVCs are snapshotted, so the still-running old vminsert pods can commit writes to old vmstorage after force-merge but before/while the snapshot is taken. Those writes are lost on restore and are also not captured by the buffer agent (new writes only reach the agent after cutover), so a 'No Downtime' migration can still drop data. Scale the old vminsert workload to zero and let it fully flush before force-merging and snapshotting vmstorage.</violation>
</file>
<file name="internal/migrate/nodowntime_cluster.go">
<violation number="1" location="internal/migrate/nodowntime_cluster.go:47">
P1: When a previous attempt leaves this deterministic snapshot, the code reuses any ready object without verifying its source PVC. Validate the existing snapshot's source and class before using it, or fail to prevent migrating stale data.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 7 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| * Dependency: [vmoperator](https://docs.victoriametrics.com/operator/): Updated default versions for VT apps to [v0.10.0](https://github.com/VictoriaMetrics/VictoriaTraces/releases/tag/v0.10.0) version. | ||
| * Dependency: [vmoperator](https://docs.victoriametrics.com/operator/): Updated default versions for VMAnomaly to [v1.30.0](https://docs.victoriametrics.com/anomaly-detection/changelog/#v1300) version | ||
|
|
||
| **Update note 1**: [vmoperator](https://docs.victoriametrics.com/operator/): removed `spec.useLegacyNaming`, introduced in v0.73.0, from all CRDs. Toggling it was already unsafe on its own: since it changes the naming convention for StatefulSets/Deployments/PVCs, flipping it on an existing CR causes the operator to rename/recreate those resources, and for PVCs specifically this meant binding to a *different*, empty PersistentVolumeClaim rather than migrating any existing data — a footgun the operator never covered and that would have taken significant work to handle safely (PV/PVC adoption, storage rebinding). The new `helm-converter migrate` command (see below) replaces its intended use case: it migrates by explicitly discovering and rebinding/repointing existing resources by identity, rather than relying on naming convention. Any existing CR with `useLegacyNaming: true` will have the field silently dropped by the updated CRD schema and revert to the operator's standard naming on the next reconcile — pin to the previous operator version until you've completed migration, or re-migrate using the new tool. |
Contributor
There was a problem hiding this comment.
P3: The ## tip section now contains two notes both numbered Update note 1 (line 16 for the validation-webhook note and this new note for the useLegacyNaming removal). Number the new note 2 so readers and the merge workflow keep a stable reference to each note.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/CHANGELOG.md, line 23:
<comment>The `## tip` section now contains two notes both numbered `Update note 1` (line 16 for the validation-webhook note and this new note for the `useLegacyNaming` removal). Number the new note 2 so readers and the merge workflow keep a stable reference to each note.</comment>
<file context>
@@ -20,23 +20,27 @@ aliases:
* Dependency: [vmoperator](https://docs.victoriametrics.com/operator/): Updated default versions for VT apps to [v0.10.0](https://github.com/VictoriaMetrics/VictoriaTraces/releases/tag/v0.10.0) version.
* Dependency: [vmoperator](https://docs.victoriametrics.com/operator/): Updated default versions for VMAnomaly to [v1.30.0](https://docs.victoriametrics.com/anomaly-detection/changelog/#v1300) version
+**Update note 1**: [vmoperator](https://docs.victoriametrics.com/operator/): removed `spec.useLegacyNaming`, introduced in v0.73.0, from all CRDs. Toggling it was already unsafe on its own: since it changes the naming convention for StatefulSets/Deployments/PVCs, flipping it on an existing CR causes the operator to rename/recreate those resources, and for PVCs specifically this meant binding to a *different*, empty PersistentVolumeClaim rather than migrating any existing data — a footgun the operator never covered and that would have taken significant work to handle safely (PV/PVC adoption, storage rebinding). The new `helm-converter migrate` command (see below) replaces its intended use case: it migrates by explicitly discovering and rebinding/repointing existing resources by identity, rather than relying on naming convention. Any existing CR with `useLegacyNaming: true` will have the field silently dropped by the updated CRD schema and revert to the operator's standard naming on the next reconcile — pin to the previous operator version until you've completed migration, or re-migrate using the new tool.
+
* FEATURE: [vmanomaly](https://docs.victoriametrics.com/operator/resources/vmanomaly/): support Temporal Envelope models, online-model history strength and warmup settings, causal and complexity-aware autotune options, and separate reader fetch and processing controls introduced in vmanomaly v1.30.0. See [#2440](https://github.com/VictoriaMetrics/operator/pull/2440).
</file context>
Suggested change
| **Update note 1**: [vmoperator](https://docs.victoriametrics.com/operator/): removed `spec.useLegacyNaming`, introduced in v0.73.0, from all CRDs. Toggling it was already unsafe on its own: since it changes the naming convention for StatefulSets/Deployments/PVCs, flipping it on an existing CR causes the operator to rename/recreate those resources, and for PVCs specifically this meant binding to a *different*, empty PersistentVolumeClaim rather than migrating any existing data — a footgun the operator never covered and that would have taken significant work to handle safely (PV/PVC adoption, storage rebinding). The new `helm-converter migrate` command (see below) replaces its intended use case: it migrates by explicitly discovering and rebinding/repointing existing resources by identity, rather than relying on naming convention. Any existing CR with `useLegacyNaming: true` will have the field silently dropped by the updated CRD schema and revert to the operator's standard naming on the next reconcile — pin to the previous operator version until you've completed migration, or re-migrate using the new tool. | |
| **Update note 2**: [vmoperator](https://docs.victoriametrics.com/operator/): removed `spec.useLegacyNaming`, introduced in v0.73.0, from all CRDs. Toggling it was already unsafe on its own: since it changes the naming convention for StatefulSets/Deployments/PVCs, flipping it on an existing CR causes the operator to rename/recreate those resources, and for PVCs specifically this meant binding to a *different*, empty PersistentVolumeClaim rather than migrating any existing data — a footgun the operator never covered and that would have taken significant work to handle safely (PV/PVC adoption, storage rebinding). The new `helm-converter migrate` command (see below) replaces its intended use case: it migrates by explicitly discovering and rebinding/repointing existing resources by identity, rather than relying on naming convention. Any existing CR with `useLegacyNaming: true` will have the field silently dropped by the updated CRD schema and revert to the operator's standard naming on the next reconcile — pin to the previous operator version until you've completed migration, or re-migrate using the new tool. |
AndrewChubatiuk
force-pushed
the
converter-migrate
branch
2 times, most recently
from
August 17, 2026 20:45
55163f2 to
e19beb1
Compare
Contributor
Author
|
@cubic-ai-dev |
Contributor
@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete. |
AndrewChubatiuk
force-pushed
the
converter-migrate
branch
from
August 17, 2026 20:50
e19beb1 to
1449f67
Compare
AndrewChubatiuk
force-pushed
the
converter-migrate
branch
from
August 17, 2026 20:52
1449f67 to
7123727
Compare
Contributor
There was a problem hiding this comment.
34 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/migrate/vm/nodowntime.go">
<violation number="1" location="internal/migrate/vm/nodowntime.go:122">
P1: On a retry with an existing target CR, this unconditionally deletes the target PVC before checking or adopting the CR. Inspect the target first and reuse a compatible existing PVC instead of treating it as stale.</violation>
<violation number="2" location="internal/migrate/vm/nodowntime.go:125">
P1: When the converted target has no usable `Spec.Storage` (or uses `StorageDataPath`), the restored PVC is never mounted and the new VMSingle starts without the migrated data. Validate the target storage mode before snapshot/PVC mutations and reject unsupported targets.</violation>
<violation number="3" location="internal/migrate/vm/nodowntime.go:177">
P1: In NoDowntimeSingleNode, the buffer agent is deleted (`c.Delete(ctx, agent)`) with only a warning on failure, even though `WaitQueueDrained` just reported that the queue did NOT fully drain. If `WaitQueueDrained` after final cutover fails (returns an error), the code prints a warning but still proceeds to delete the agent, discarding the persistent-queue data that still holds in-flight writes that were never flushed to the new storage. Deleting a VMAgent whose StatefulMode PVC holds undrained writes permanently loses those writes. On the drain-failure path the agent should be left in place for manual recovery rather than deleted.</violation>
</file>
<file name="internal/migrate/withdowntime_cluster.go">
<violation number="1" location="internal/migrate/withdowntime_cluster.go:96">
P2: Components are deleted and their PVs rebound inside the loop before the target CR is created. If a later component's delete/rebind fails, or c.Create(target) fails, or WaitForOperational times out (15 min default), the components already processed are left destroyed with their PVs rebound to orphaned PVCs and no rollback. Consider creating/validating the target and proving the new setup is recoverable before mutating the loop, or documenting the required manual recovery steps.</violation>
<violation number="2" location="internal/migrate/withdowntime_cluster.go:102">
P2: When a stateless component is deleted, the function can create the target while old pods are still running because it waits only for PVC-bearing StatefulSets. Wait for the deleted workload's pods to disappear on every deletion path before creating the target.</violation>
<violation number="3" location="internal/migrate/withdowntime_cluster.go:108">
P1: When any source PVC is Pending or lacks its bound PV, `RebindPVC` fails after the old StatefulSet has been deleted. Validate every source PVC is Bound and its PV is readable during preflight, before entering the destructive loop.</violation>
<violation number="4" location="internal/migrate/withdowntime_cluster.go:114">
P0: When the target CR already exists, including after a retry, this call runs only after deleting the old workloads and rebinding their PVCs. `Create` then returns `AlreadyExists`, leaving the migration without the new setup; reject or validate an existing target before any destructive operation.</violation>
<violation number="5" location="internal/migrate/withdowntime_cluster.go:154">
P1: When a source StatefulSet has a missing tail PVC, this loop treats the remaining ordinals as complete and lets the target create a fresh empty claim for the missing ordinal. Pass the expected replica count and require every ordinal from 0 through `replicas-1` before rebinding.</violation>
</file>
<file name="internal/migrate/vm/urls.go">
<violation number="1" location="internal/migrate/vm/urls.go:21">
P3: insertRemoteWriteURL is load-bearing for the NoDowntime cluster migration (misbuilding it writes buffered data to the wrong path and can fork/lose data), yet it has no test in internal/migrate/vm/urls_test.go, which tests only remoteWriteURL. Add a case asserting the returned URL and the error path for a service without an 'http' port, matching TestRemoteWriteURL/TestRemoteWriteURL_NoHTTPPort.</violation>
</file>
<file name="internal/migrate/vm/cluster.go">
<violation number="1" location="internal/migrate/vm/cluster.go:37">
P2: When `VMSelect.StorageSpec` is set without `CacheMountPath`, the target operator does not create or mount a VMSelect PVC, but this descriptor tells migration to rebind old PVCs to that target template. The migrated cache is therefore left unattached; require `CacheMountPath` here as the target StatefulSet does.</violation>
</file>
<file name="internal/migrate/pvcrebind.go">
<violation number="1" location="internal/migrate/pvcrebind.go:78">
P1: After this deletion succeeds, any later PV update, target creation, or bind timeout can return with the source PVC gone and no rollback. Since the WithDowntime caller has already stopped the old workload, recreate the source claim and restore the PV claimRef on every post-delete failure, or otherwise persist and execute a recovery path before returning.</violation>
<violation number="2" location="internal/migrate/pvcrebind.go:101">
P2: After clearing pv.Spec.ClaimRef the PV becomes Available and can be claimed by any matching pending PVC in the cluster, not just the target PVC created a few calls later. In clusters with pending dynamically-provisioned or static PVCs, another claim could bind this PV in that window, so the new target would fail to bind or a foreign pod would attach to the old data. Creating the target PVC with VolumeName set mitigates only the target's own bind, not the window. Consider creating the target PVC (pre-bound via VolumeName) before clearing claimRef, or deleting source and immediately creating target in the same transaction sequence to shrink the available window.</violation>
<violation number="3" location="internal/migrate/pvcrebind.go:121">
P2: RebindPVC switches the PV reclaim policy to Retain up front but only calls restorePVReclaimPolicy on the success and existing-target paths. If deletion of the source PVC times out, clearing claimRef fails, creating the target PVC fails, or bind-wait times out, the function returns an error with the PV permanently left on Retain — silently changing the storage's reclaim behavior from whatever the admin configured (e.g. Delete) for that PV, even though no migration happened. Restore the original policy on all error paths, e.g. with a defer after the Retain switch that runs on every return.</violation>
</file>
<file name="internal/migrate/vt/cluster.go">
<violation number="1" location="internal/migrate/vt/cluster.go:29">
P1: When the target VTCluster configures `storage.storage.emptyDir`, this condition still advertises a PVC template and the migration rebinds old data PVCs that the new StatefulSet will not mount. Check `EmptyDir == nil` before setting `TargetPVCTemplateName`, so the migration refuses rather than orphaning the old data.</violation>
</file>
<file name="internal/migrate/vl/nodowntime_cluster_test.go">
<violation number="1" location="internal/migrate/vl/nodowntime_cluster_test.go:188">
P3: The mock admin server always returns `vlagent_queue_rows_pending{...} 0`, so `WaitQueueDrained` passes trivially, and the test only asserts the buffer agent's *initial* write URL (`initialWriteURL`), never its post-cutover `RemoteWrite` URL. The core NoDowntime property—that buffered writes are actually flushed to the new storage after repoint—is therefore never validated; the test would pass even if the agent were never repointed or data never drained. Capture the repointed URL (e.g. via an `Update` interceptor) and/or serve a non-empty queue on the first poll and assert it reaches 0.</violation>
</file>
<file name="internal/migrate/nodowntime_cluster.go">
<violation number="1" location="internal/migrate/nodowntime_cluster.go:58">
P1: When a previous migration attempt has already created and populated the target cluster, a retry deletes its target PVC before recreating it from the old source snapshot. This can be blocked by PVC protection or destroy the target's restored data and lose writes already migrated; refuse to delete claims belonging to an existing target CR, or validate and reuse the existing migration PVC instead.</violation>
</file>
<file name="internal/migrate/vl/nodowntime.go">
<violation number="1" location="internal/migrate/vl/nodowntime.go:39">
P1: When `target.Spec.StorageDataPath` is set, the VLSingle operator ignores `Spec.Storage`, so the restored PVC is not mounted and the target starts without the snapshot data. Reject non-empty `StorageDataPath` here before proceeding with the migration.</violation>
<violation number="2" location="internal/migrate/vl/nodowntime.go:48">
P3: The plan `fmt.Printf` in NoDowntimeSingleNode has 10 argument-consuming `%s` verbs but only 8 arguments. The trailing "point read-only clients at %s/%s" prints `%!s(MISSING)%!s(MISSING)`, and the "Service %s/%s" pair receives `readsAlias` instead of `target.Name`, so the printed migration plan misreports the Service name during a destructive operation. Add the missing `target.Name` argument.</violation>
</file>
<file name="internal/migrate/snapshot_test.go">
<violation number="1" location="internal/migrate/snapshot_test.go:29">
P2: TestCreateAndWaitVolumeSnapshot runs a background goroutine that reads and writes the shared fake client (c.Get then c.Update) at the same time WaitVolumeSnapshotReady's poll loop reads it via c.Get. The controller-runtime fake client only guards writes with trackerWriteLock while reads take schemeLock.RLock, so this concurrent read/write on the shared tracker is unsynchronized and can trip `go test -race` or flake. Since WaitVolumeSnapshotReady polls with immediate=true, set status.readyToUse synchronously after CreateVolumeSnapshot and drop the goroutine, making the test deterministic.</violation>
</file>
<file name="internal/migrate/vm/nodowntime_test.go">
<violation number="1" location="internal/migrate/vm/nodowntime_test.go:70">
P3: These tests mutate package-global migrate timing vars (WriteQuiesceGracePeriod, SnapshotPollInterval, DrainPollInterval, etc.) and never restore them, so the process-global state persists for every later test in the vm package. The stale-agent test only resets TargetReady* and silently depends on the first test having left the others tiny. Restore the previous values (or use t.Setenv-style helper) so each test is self-contained and safe under shuffling/`-count` reruns.</violation>
</file>
<file name="internal/migrate/vl/nodowntime_cluster.go">
<violation number="1" location="internal/migrate/vl/nodowntime_cluster.go:132">
P1: When the target `VLCluster` already exists, including after a partially failed migration, this call deletes its PVCs before the code checks or adopts the CR. Validate the existing target CR before replacing target PVCs, or refuse the migration while it is running, to avoid disrupting the target and risking restored data.</violation>
</file>
<file name="internal/migrate/vl/nodowntime_test.go">
<violation number="1" location="internal/migrate/vl/nodowntime_test.go:58">
P3: vl/nodowntime.go has an explicit error path for a stale buffer VLAgent (AlreadyExists with a mismatched remote-write URL), but this test file lands with no coverage for that branch, unlike its sibling vm/nodowntime_test.go which adds TestNoDowntimeSingleNode_StaleBufferAgentIsAnError. Add a VL equivalent to guard that non-trivial error path.</violation>
</file>
<file name="internal/migrate/withdowntime.go">
<violation number="1" location="internal/migrate/withdowntime.go:108">
P1: When the target CR already exists, this check runs only after deleting the old workload and rebinding its PVC. `c.Create` then returns `AlreadyExists`, leaving the old service stopped with migration state partially changed; preflight the target CR before any destructive mutation.</violation>
<violation number="2" location="internal/migrate/withdowntime.go:207">
P2: When a Service selector shares a key with the Deployment selector but has a different value, this helper accepts it and `CutoverServices` redirects that Service to the target pods. Compare selector values as well as keys before allowing cutover.</violation>
<violation number="3" location="internal/migrate/withdowntime.go:257">
P3: When c.Update fails at index i, CutoverServices reverts indices [0,i) but leaves services[i].Spec.Selector mutated to newSelector in memory even though the failed Update was never persisted. This makes the object's in-memory state disagree with the server, contradicting the function's stated re-invocability on the same slice. Revert services[i] as well before returning the error.</violation>
</file>
<file name="internal/migrate/vm/nodowntime_cluster.go">
<violation number="1" location="internal/migrate/vm/nodowntime_cluster.go:133">
P1: When the target VMCluster already exists, including after a retry, this call deletes its PVCs before the existing-CR/spec check runs. Resolve and validate an existing target before replacing PVCs, or refuse to continue when target PVCs already exist.</violation>
<violation number="2" location="internal/migrate/vm/nodowntime_cluster.go:166">
P1: When the target uses a custom `ServiceSpec.Name`, these readiness checks query the default component name instead of the Service the operator created. Resolve each component's configured service name before calling `WaitEndpointsReady`, otherwise migration times out despite healthy target pods.</violation>
<violation number="3" location="internal/migrate/vm/nodowntime_cluster.go:175">
P0: When the buffer queue does not drain after final cutover, this warning is followed by deletion of the buffer VMAgent, which can discard undelivered writes. Return the error and leave the buffer agent in place for manual recovery instead of deleting it.</violation>
</file>
<file name="cmd/helm-converter/main.go">
<violation number="1" location="cmd/helm-converter/main.go:44">
P2: The legacy no-subcommand path promises existing scripts keep working, but this change drops the previous -chart default of victoria-metrics-single and adds a hard requirement. A script that previously ran `helm-converter -input=vals.yaml -output=out.yaml` (relying on the default chart) now exits with -chart is required, contradicting the comment above main(). Restore the default to keep the backward-compat alias working; migrate can still require -chart explicitly.</violation>
</file>
<file name="internal/migrate/drain.go">
<violation number="1" location="internal/migrate/drain.go:83">
P3: When a pod returns no samples for the queue metric, the poll keeps retrying for the full 6-hour DrainTimeout because `len(samples) == 0` maps to `false, nil`. A wrong metric name or missing `url` dimension is then indistinguishable from a transient state and stalls the migration for hours. After the first few polls confirm the metric persists at zero samples, surface an error instead of polling to timeout.</violation>
</file>
<file name="internal/migrate/convert.go">
<violation number="1" location="internal/migrate/convert.go:14">
P3: ConvertSpec is newly added but never called anywhere in the codebase; only ConvertSpecAtVersion is used (from runMigrate). This is dead code that fetches the latest chart defaults, which is precisely the behavior migrate intentionally avoids. Drop it (or add a caller) to avoid maintaining a second, subtly-wrong conversion entry point.</violation>
</file>
<file name="internal/migrate/alias.go">
<violation number="1" location="internal/migrate/alias.go:57">
P3: When the caller supplies ports with empty Protocol/TargetPort, the API server defaults them on the persisted Service, so a later reuse of the same owned alias compares defaulted ports (from `existing`) against non-defaulted ports (from `aliasPorts`) and spuriously fails the equality check. Normalize the caller's ports (default Protocol to TCP and TargetPort to Port) before building `aliasPorts` and comparing, or make `servicePortsEqual` treat empty Protocol as `TCP` and empty TargetPort as equal to `Port`.</violation>
</file>
<file name="docs/CHANGELOG.md">
<violation number="1" location="docs/CHANGELOG.md:23">
P2: Custom agent: **Changelog Review Agent**
The new `helm-converter migrate` changelog entry has no issue or PR reference. Add the relevant migration issue/PR link to satisfy the mandatory References section.</violation>
<violation number="2" location="docs/CHANGELOG.md:25">
P2: Custom agent: **Changelog Review Agent**
The `NoDowntime` changelog bullet exposes implementation mechanics instead of only the user-visible outcome. Summarize it as a no-downtime cutover that keeps the existing release serving writes while the operator-managed CR is prepared, then switches traffic without requiring service downtime; keep CSI snapshots, buffering, and PVC force-merging in `helm-converter.md`.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| } | ||
| } | ||
|
|
||
| if err := c.Create(ctx, target); err != nil { |
Contributor
There was a problem hiding this comment.
P0: When the target CR already exists, including after a retry, this call runs only after deleting the old workloads and rebinding their PVCs. Create then returns AlreadyExists, leaving the migration without the new setup; reject or validate an existing target before any destructive operation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/withdowntime_cluster.go, line 114:
<comment>When the target CR already exists, including after a retry, this call runs only after deleting the old workloads and rebinding their PVCs. `Create` then returns `AlreadyExists`, leaving the migration without the new setup; reject or validate an existing target before any destructive operation.</comment>
<file context>
@@ -0,0 +1,186 @@
+ }
+ }
+
+ if err := c.Create(ctx, target); err != nil {
+ return fmt.Errorf("cannot create target CR %s/%s: %w", target.GetNamespace(), target.GetName(), err)
+ }
</file context>
|
|
||
| time.Sleep(migrate.WriteQuiesceGracePeriod) | ||
| if err := migrate.WaitQueueDrained(ctx, c, httpClient, agent, vmv1beta1.VMAgentQueueMetricName, 0); err != nil { | ||
| fmt.Printf("warning: buffer VMAgent queue did not fully drain after final cutover, some in-flight writes may need manual replay: %v\n", err) |
Contributor
There was a problem hiding this comment.
P0: When the buffer queue does not drain after final cutover, this warning is followed by deletion of the buffer VMAgent, which can discard undelivered writes. Return the error and leave the buffer agent in place for manual recovery instead of deleting it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/vm/nodowntime_cluster.go, line 175:
<comment>When the buffer queue does not drain after final cutover, this warning is followed by deletion of the buffer VMAgent, which can discard undelivered writes. Return the error and leave the buffer agent in place for manual recovery instead of deleting it.</comment>
<file context>
@@ -0,0 +1,190 @@
+
+ time.Sleep(migrate.WriteQuiesceGracePeriod)
+ if err := migrate.WaitQueueDrained(ctx, c, httpClient, agent, vmv1beta1.VMAgentQueueMetricName, 0); err != nil {
+ fmt.Printf("warning: buffer VMAgent queue did not fully drain after final cutover, some in-flight writes may need manual replay: %v\n", err)
+ }
+ if err := c.Delete(ctx, agent); err != nil && !k8serrors.IsNotFound(err) {
</file context>
| if err := migrate.WaitQueueDrained(ctx, c, httpClient, agent, vmv1beta1.VMAgentQueueMetricName, 0); err != nil { | ||
| fmt.Printf("warning: buffer VMAgent queue did not fully drain after final cutover, some in-flight writes may need manual replay: %v\n", err) | ||
| } | ||
| if err := c.Delete(ctx, agent); err != nil && !k8serrors.IsNotFound(err) { |
Contributor
There was a problem hiding this comment.
P1: In NoDowntimeSingleNode, the buffer agent is deleted (c.Delete(ctx, agent)) with only a warning on failure, even though WaitQueueDrained just reported that the queue did NOT fully drain. If WaitQueueDrained after final cutover fails (returns an error), the code prints a warning but still proceeds to delete the agent, discarding the persistent-queue data that still holds in-flight writes that were never flushed to the new storage. Deleting a VMAgent whose StatefulMode PVC holds undrained writes permanently loses those writes. On the drain-failure path the agent should be left in place for manual recovery rather than deleted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/vm/nodowntime.go, line 177:
<comment>In NoDowntimeSingleNode, the buffer agent is deleted (`c.Delete(ctx, agent)`) with only a warning on failure, even though `WaitQueueDrained` just reported that the queue did NOT fully drain. If `WaitQueueDrained` after final cutover fails (returns an error), the code prints a warning but still proceeds to delete the agent, discarding the persistent-queue data that still holds in-flight writes that were never flushed to the new storage. Deleting a VMAgent whose StatefulMode PVC holds undrained writes permanently loses those writes. On the drain-failure path the agent should be left in place for manual recovery rather than deleted.</comment>
<file context>
@@ -0,0 +1,186 @@
+ if err := migrate.WaitQueueDrained(ctx, c, httpClient, agent, vmv1beta1.VMAgentQueueMetricName, 0); err != nil {
+ fmt.Printf("warning: buffer VMAgent queue did not fully drain after final cutover, some in-flight writes may need manual replay: %v\n", err)
+ }
+ if err := c.Delete(ctx, agent); err != nil && !k8serrors.IsNotFound(err) {
+ fmt.Printf("warning: cannot delete buffer VMAgent %s/%s, please remove it manually: %v\n", agent.Namespace, agent.Name, err)
+ }
</file context>
| if err := waitPodsGone(ctx, c, sts.Namespace, sts.Spec.Selector.MatchLabels); err != nil { | ||
| return fmt.Errorf("component %q: %w", comp.HelmComponentLabel, err) | ||
| } | ||
| if err := rebindClusterPVCs(ctx, c, d.PVCs, comp.TargetPVCTemplateName, comp.TargetWorkloadName, target.GetNamespace()); err != nil { |
Contributor
There was a problem hiding this comment.
P1: When any source PVC is Pending or lacks its bound PV, RebindPVC fails after the old StatefulSet has been deleted. Validate every source PVC is Bound and its PV is readable during preflight, before entering the destructive loop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/withdowntime_cluster.go, line 108:
<comment>When any source PVC is Pending or lacks its bound PV, `RebindPVC` fails after the old StatefulSet has been deleted. Validate every source PVC is Bound and its PV is readable during preflight, before entering the destructive loop.</comment>
<file context>
@@ -0,0 +1,186 @@
+ if err := waitPodsGone(ctx, c, sts.Namespace, sts.Spec.Selector.MatchLabels); err != nil {
+ return fmt.Errorf("component %q: %w", comp.HelmComponentLabel, err)
+ }
+ if err := rebindClusterPVCs(ctx, c, d.PVCs, comp.TargetPVCTemplateName, comp.TargetWorkloadName, target.GetNamespace()); err != nil {
+ return fmt.Errorf("component %q: %w", comp.HelmComponentLabel, err)
+ }
</file context>
| if err != nil { | ||
| return err | ||
| } | ||
| for i := 0; i < len(byOrdinal); i++ { |
Contributor
There was a problem hiding this comment.
P1: When a source StatefulSet has a missing tail PVC, this loop treats the remaining ordinals as complete and lets the target create a fresh empty claim for the missing ordinal. Pass the expected replica count and require every ordinal from 0 through replicas-1 before rebinding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/withdowntime_cluster.go, line 154:
<comment>When a source StatefulSet has a missing tail PVC, this loop treats the remaining ordinals as complete and lets the target create a fresh empty claim for the missing ordinal. Pass the expected replica count and require every ordinal from 0 through `replicas-1` before rebinding.</comment>
<file context>
@@ -0,0 +1,186 @@
+ if err != nil {
+ return err
+ }
+ for i := 0; i < len(byOrdinal); i++ {
+ pvc, ok := byOrdinal[i]
+ if !ok {
</file context>
| "create %s/%s from the snapshot, backfill, then cut traffic over to it — the old Deployment and PVC are never deleted. "+ | ||
| "Service %s/%s serves both reads and writes, and the buffer agent only handles writes, so queries against %[1]s/%[2]s "+ | ||
| "will fail until final cutover; point read-only clients at %s/%s in the meantime if that matters\n", | ||
| oldSvc.Namespace, oldSvc.Name, pvc.Namespace, pvc.Name, target.GetNamespace(), target.GetName(), target.Namespace, readsAlias) |
Contributor
There was a problem hiding this comment.
P3: The plan fmt.Printf in NoDowntimeSingleNode has 10 argument-consuming %s verbs but only 8 arguments. The trailing "point read-only clients at %s/%s" prints %!s(MISSING)%!s(MISSING), and the "Service %s/%s" pair receives readsAlias instead of target.Name, so the printed migration plan misreports the Service name during a destructive operation. Add the missing target.Name argument.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/vl/nodowntime.go, line 48:
<comment>The plan `fmt.Printf` in NoDowntimeSingleNode has 10 argument-consuming `%s` verbs but only 8 arguments. The trailing "point read-only clients at %s/%s" prints `%!s(MISSING)%!s(MISSING)`, and the "Service %s/%s" pair receives `readsAlias` instead of `target.Name`, so the printed migration plan misreports the Service name during a destructive operation. Add the missing `target.Name` argument.</comment>
<file context>
@@ -0,0 +1,184 @@
+ "create %s/%s from the snapshot, backfill, then cut traffic over to it — the old Deployment and PVC are never deleted. "+
+ "Service %s/%s serves both reads and writes, and the buffer agent only handles writes, so queries against %[1]s/%[2]s "+
+ "will fail until final cutover; point read-only clients at %s/%s in the meantime if that matters\n",
+ oldSvc.Namespace, oldSvc.Name, pvc.Namespace, pvc.Name, target.GetNamespace(), target.GetName(), target.Namespace, readsAlias)
+
+ if opts.DryRun {
</file context>
Suggested change
| oldSvc.Namespace, oldSvc.Name, pvc.Namespace, pvc.Name, target.GetNamespace(), target.GetName(), target.Namespace, readsAlias) | |
| oldSvc.Namespace, oldSvc.Name, pvc.Namespace, pvc.Name, target.GetNamespace(), target.GetName(), target.Namespace, target.Name, target.Namespace, readsAlias) |
| svc.Spec.Selector = newSelector | ||
| if err := c.Update(ctx, svc); err != nil { | ||
| updateErr := fmt.Errorf("cannot patch Service %s/%s selector: %w", svc.Namespace, svc.Name, err) | ||
| for j := range i { |
Contributor
There was a problem hiding this comment.
P3: When c.Update fails at index i, CutoverServices reverts indices [0,i) but leaves services[i].Spec.Selector mutated to newSelector in memory even though the failed Update was never persisted. This makes the object's in-memory state disagree with the server, contradicting the function's stated re-invocability on the same slice. Revert services[i] as well before returning the error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/withdowntime.go, line 257:
<comment>When c.Update fails at index i, CutoverServices reverts indices [0,i) but leaves services[i].Spec.Selector mutated to newSelector in memory even though the failed Update was never persisted. This makes the object's in-memory state disagree with the server, contradicting the function's stated re-invocability on the same slice. Revert services[i] as well before returning the error.</comment>
<file context>
@@ -0,0 +1,288 @@
+ svc.Spec.Selector = newSelector
+ if err := c.Update(ctx, svc); err != nil {
+ updateErr := fmt.Errorf("cannot patch Service %s/%s selector: %w", svc.Namespace, svc.Name, err)
+ for j := range i {
+ services[j].Spec.Selector = originalSelectors[j]
+ if revertErr := c.Update(ctx, services[j]); revertErr != nil {
</file context>
| return false, fmt.Errorf("scraping %s: %w", addr, err) | ||
| } | ||
| samples := metrics[metricName] | ||
| if len(samples) == 0 { |
Contributor
There was a problem hiding this comment.
P3: When a pod returns no samples for the queue metric, the poll keeps retrying for the full 6-hour DrainTimeout because len(samples) == 0 maps to false, nil. A wrong metric name or missing url dimension is then indistinguishable from a transient state and stalls the migration for hours. After the first few polls confirm the metric persists at zero samples, surface an error instead of polling to timeout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/drain.go, line 83:
<comment>When a pod returns no samples for the queue metric, the poll keeps retrying for the full 6-hour DrainTimeout because `len(samples) == 0` maps to `false, nil`. A wrong metric name or missing `url` dimension is then indistinguishable from a transient state and stalls the migration for hours. After the first few polls confirm the metric persists at zero samples, surface an error instead of polling to timeout.</comment>
<file context>
@@ -0,0 +1,98 @@
+ return false, fmt.Errorf("scraping %s: %w", addr, err)
+ }
+ samples := metrics[metricName]
+ if len(samples) == 0 {
+ return false, nil
+ }
</file context>
|
|
||
| // ConvertSpec reads valuesFile, merges it over the chart's latest published defaults, and | ||
| // converts it to the operator CR for the given chart and target name/namespace. | ||
| func ConvertSpec(chart Chart, valuesFile, targetName, targetNamespace string) (any, error) { |
Contributor
There was a problem hiding this comment.
P3: ConvertSpec is newly added but never called anywhere in the codebase; only ConvertSpecAtVersion is used (from runMigrate). This is dead code that fetches the latest chart defaults, which is precisely the behavior migrate intentionally avoids. Drop it (or add a caller) to avoid maintaining a second, subtly-wrong conversion entry point.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/convert.go, line 14:
<comment>ConvertSpec is newly added but never called anywhere in the codebase; only ConvertSpecAtVersion is used (from runMigrate). This is dead code that fetches the latest chart defaults, which is precisely the behavior migrate intentionally avoids. Drop it (or add a caller) to avoid maintaining a second, subtly-wrong conversion entry point.</comment>
<file context>
@@ -0,0 +1,113 @@
+
+// ConvertSpec reads valuesFile, merges it over the chart's latest published defaults, and
+// converts it to the operator CR for the given chart and target name/namespace.
+func ConvertSpec(chart Chart, valuesFile, targetName, targetNamespace string) (any, error) {
+ defaults, err := converter.FetchChartDefaults(string(chart))
+ if err != nil {
</file context>
| return nil, fmt.Errorf("existing alias Service %s/%s has type %q, expected %q — refusing to reuse it", | ||
| namespace, name, existing.Spec.Type, corev1.ServiceTypeClusterIP) | ||
| } | ||
| if !reflect.DeepEqual(existing.Spec.Selector, selector) || !servicePortsEqual(existing.Spec.Ports, aliasPorts) { |
Contributor
There was a problem hiding this comment.
P3: When the caller supplies ports with empty Protocol/TargetPort, the API server defaults them on the persisted Service, so a later reuse of the same owned alias compares defaulted ports (from existing) against non-defaulted ports (from aliasPorts) and spuriously fails the equality check. Normalize the caller's ports (default Protocol to TCP and TargetPort to Port) before building aliasPorts and comparing, or make servicePortsEqual treat empty Protocol as TCP and empty TargetPort as equal to Port.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/alias.go, line 57:
<comment>When the caller supplies ports with empty Protocol/TargetPort, the API server defaults them on the persisted Service, so a later reuse of the same owned alias compares defaulted ports (from `existing`) against non-defaulted ports (from `aliasPorts`) and spuriously fails the equality check. Normalize the caller's ports (default Protocol to TCP and TargetPort to Port) before building `aliasPorts` and comparing, or make `servicePortsEqual` treat empty Protocol as `TCP` and empty TargetPort as equal to `Port`.</comment>
<file context>
@@ -0,0 +1,87 @@
+ return nil, fmt.Errorf("existing alias Service %s/%s has type %q, expected %q — refusing to reuse it",
+ namespace, name, existing.Spec.Type, corev1.ServiceTypeClusterIP)
+ }
+ if !reflect.DeepEqual(existing.Spec.Selector, selector) || !servicePortsEqual(existing.Spec.Ports, aliasPorts) {
+ return nil, fmt.Errorf("a Service %s/%s already exists but its selector/ports don't match the backend this migration expects — "+
+ "it may be left over from a previous migration attempt against a different backend; "+
</file context>
Contributor
There was a problem hiding this comment.
34 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/migrate/vm/nodowntime.go">
<violation number="1" location="internal/migrate/vm/nodowntime.go:122">
P1: On a retry with an existing target CR, this unconditionally deletes the target PVC before checking or adopting the CR. Inspect the target first and reuse a compatible existing PVC instead of treating it as stale.</violation>
<violation number="2" location="internal/migrate/vm/nodowntime.go:125">
P1: When the converted target has no usable `Spec.Storage` (or uses `StorageDataPath`), the restored PVC is never mounted and the new VMSingle starts without the migrated data. Validate the target storage mode before snapshot/PVC mutations and reject unsupported targets.</violation>
<violation number="3" location="internal/migrate/vm/nodowntime.go:177">
P1: In NoDowntimeSingleNode, the buffer agent is deleted (`c.Delete(ctx, agent)`) with only a warning on failure, even though `WaitQueueDrained` just reported that the queue did NOT fully drain. If `WaitQueueDrained` after final cutover fails (returns an error), the code prints a warning but still proceeds to delete the agent, discarding the persistent-queue data that still holds in-flight writes that were never flushed to the new storage. Deleting a VMAgent whose StatefulMode PVC holds undrained writes permanently loses those writes. On the drain-failure path the agent should be left in place for manual recovery rather than deleted.</violation>
</file>
<file name="internal/migrate/withdowntime_cluster.go">
<violation number="1" location="internal/migrate/withdowntime_cluster.go:96">
P2: Components are deleted and their PVs rebound inside the loop before the target CR is created. If a later component's delete/rebind fails, or c.Create(target) fails, or WaitForOperational times out (15 min default), the components already processed are left destroyed with their PVs rebound to orphaned PVCs and no rollback. Consider creating/validating the target and proving the new setup is recoverable before mutating the loop, or documenting the required manual recovery steps.</violation>
<violation number="2" location="internal/migrate/withdowntime_cluster.go:102">
P2: When a stateless component is deleted, the function can create the target while old pods are still running because it waits only for PVC-bearing StatefulSets. Wait for the deleted workload's pods to disappear on every deletion path before creating the target.</violation>
<violation number="3" location="internal/migrate/withdowntime_cluster.go:108">
P1: When any source PVC is Pending or lacks its bound PV, `RebindPVC` fails after the old StatefulSet has been deleted. Validate every source PVC is Bound and its PV is readable during preflight, before entering the destructive loop.</violation>
<violation number="4" location="internal/migrate/withdowntime_cluster.go:114">
P0: When the target CR already exists, including after a retry, this call runs only after deleting the old workloads and rebinding their PVCs. `Create` then returns `AlreadyExists`, leaving the migration without the new setup; reject or validate an existing target before any destructive operation.</violation>
<violation number="5" location="internal/migrate/withdowntime_cluster.go:154">
P1: When a source StatefulSet has a missing tail PVC, this loop treats the remaining ordinals as complete and lets the target create a fresh empty claim for the missing ordinal. Pass the expected replica count and require every ordinal from 0 through `replicas-1` before rebinding.</violation>
</file>
<file name="internal/migrate/vm/urls.go">
<violation number="1" location="internal/migrate/vm/urls.go:21">
P3: insertRemoteWriteURL is load-bearing for the NoDowntime cluster migration (misbuilding it writes buffered data to the wrong path and can fork/lose data), yet it has no test in internal/migrate/vm/urls_test.go, which tests only remoteWriteURL. Add a case asserting the returned URL and the error path for a service without an 'http' port, matching TestRemoteWriteURL/TestRemoteWriteURL_NoHTTPPort.</violation>
</file>
<file name="internal/migrate/vm/cluster.go">
<violation number="1" location="internal/migrate/vm/cluster.go:37">
P2: When `VMSelect.StorageSpec` is set without `CacheMountPath`, the target operator does not create or mount a VMSelect PVC, but this descriptor tells migration to rebind old PVCs to that target template. The migrated cache is therefore left unattached; require `CacheMountPath` here as the target StatefulSet does.</violation>
</file>
<file name="internal/migrate/pvcrebind.go">
<violation number="1" location="internal/migrate/pvcrebind.go:78">
P1: After this deletion succeeds, any later PV update, target creation, or bind timeout can return with the source PVC gone and no rollback. Since the WithDowntime caller has already stopped the old workload, recreate the source claim and restore the PV claimRef on every post-delete failure, or otherwise persist and execute a recovery path before returning.</violation>
<violation number="2" location="internal/migrate/pvcrebind.go:101">
P2: After clearing pv.Spec.ClaimRef the PV becomes Available and can be claimed by any matching pending PVC in the cluster, not just the target PVC created a few calls later. In clusters with pending dynamically-provisioned or static PVCs, another claim could bind this PV in that window, so the new target would fail to bind or a foreign pod would attach to the old data. Creating the target PVC with VolumeName set mitigates only the target's own bind, not the window. Consider creating the target PVC (pre-bound via VolumeName) before clearing claimRef, or deleting source and immediately creating target in the same transaction sequence to shrink the available window.</violation>
<violation number="3" location="internal/migrate/pvcrebind.go:121">
P2: RebindPVC switches the PV reclaim policy to Retain up front but only calls restorePVReclaimPolicy on the success and existing-target paths. If deletion of the source PVC times out, clearing claimRef fails, creating the target PVC fails, or bind-wait times out, the function returns an error with the PV permanently left on Retain — silently changing the storage's reclaim behavior from whatever the admin configured (e.g. Delete) for that PV, even though no migration happened. Restore the original policy on all error paths, e.g. with a defer after the Retain switch that runs on every return.</violation>
</file>
<file name="internal/migrate/vt/cluster.go">
<violation number="1" location="internal/migrate/vt/cluster.go:29">
P1: When the target VTCluster configures `storage.storage.emptyDir`, this condition still advertises a PVC template and the migration rebinds old data PVCs that the new StatefulSet will not mount. Check `EmptyDir == nil` before setting `TargetPVCTemplateName`, so the migration refuses rather than orphaning the old data.</violation>
</file>
<file name="internal/migrate/vl/nodowntime_cluster_test.go">
<violation number="1" location="internal/migrate/vl/nodowntime_cluster_test.go:188">
P3: The mock admin server always returns `vlagent_queue_rows_pending{...} 0`, so `WaitQueueDrained` passes trivially, and the test only asserts the buffer agent's *initial* write URL (`initialWriteURL`), never its post-cutover `RemoteWrite` URL. The core NoDowntime property—that buffered writes are actually flushed to the new storage after repoint—is therefore never validated; the test would pass even if the agent were never repointed or data never drained. Capture the repointed URL (e.g. via an `Update` interceptor) and/or serve a non-empty queue on the first poll and assert it reaches 0.</violation>
</file>
<file name="internal/migrate/nodowntime_cluster.go">
<violation number="1" location="internal/migrate/nodowntime_cluster.go:58">
P1: When a previous migration attempt has already created and populated the target cluster, a retry deletes its target PVC before recreating it from the old source snapshot. This can be blocked by PVC protection or destroy the target's restored data and lose writes already migrated; refuse to delete claims belonging to an existing target CR, or validate and reuse the existing migration PVC instead.</violation>
</file>
<file name="internal/migrate/vl/nodowntime.go">
<violation number="1" location="internal/migrate/vl/nodowntime.go:39">
P1: When `target.Spec.StorageDataPath` is set, the VLSingle operator ignores `Spec.Storage`, so the restored PVC is not mounted and the target starts without the snapshot data. Reject non-empty `StorageDataPath` here before proceeding with the migration.</violation>
<violation number="2" location="internal/migrate/vl/nodowntime.go:48">
P3: The plan `fmt.Printf` in NoDowntimeSingleNode has 10 argument-consuming `%s` verbs but only 8 arguments. The trailing "point read-only clients at %s/%s" prints `%!s(MISSING)%!s(MISSING)`, and the "Service %s/%s" pair receives `readsAlias` instead of `target.Name`, so the printed migration plan misreports the Service name during a destructive operation. Add the missing `target.Name` argument.</violation>
</file>
<file name="internal/migrate/snapshot_test.go">
<violation number="1" location="internal/migrate/snapshot_test.go:29">
P2: TestCreateAndWaitVolumeSnapshot runs a background goroutine that reads and writes the shared fake client (c.Get then c.Update) at the same time WaitVolumeSnapshotReady's poll loop reads it via c.Get. The controller-runtime fake client only guards writes with trackerWriteLock while reads take schemeLock.RLock, so this concurrent read/write on the shared tracker is unsynchronized and can trip `go test -race` or flake. Since WaitVolumeSnapshotReady polls with immediate=true, set status.readyToUse synchronously after CreateVolumeSnapshot and drop the goroutine, making the test deterministic.</violation>
</file>
<file name="internal/migrate/vm/nodowntime_test.go">
<violation number="1" location="internal/migrate/vm/nodowntime_test.go:70">
P3: These tests mutate package-global migrate timing vars (WriteQuiesceGracePeriod, SnapshotPollInterval, DrainPollInterval, etc.) and never restore them, so the process-global state persists for every later test in the vm package. The stale-agent test only resets TargetReady* and silently depends on the first test having left the others tiny. Restore the previous values (or use t.Setenv-style helper) so each test is self-contained and safe under shuffling/`-count` reruns.</violation>
</file>
<file name="internal/migrate/vl/nodowntime_cluster.go">
<violation number="1" location="internal/migrate/vl/nodowntime_cluster.go:132">
P1: When the target `VLCluster` already exists, including after a partially failed migration, this call deletes its PVCs before the code checks or adopts the CR. Validate the existing target CR before replacing target PVCs, or refuse the migration while it is running, to avoid disrupting the target and risking restored data.</violation>
</file>
<file name="internal/migrate/vl/nodowntime_test.go">
<violation number="1" location="internal/migrate/vl/nodowntime_test.go:58">
P3: vl/nodowntime.go has an explicit error path for a stale buffer VLAgent (AlreadyExists with a mismatched remote-write URL), but this test file lands with no coverage for that branch, unlike its sibling vm/nodowntime_test.go which adds TestNoDowntimeSingleNode_StaleBufferAgentIsAnError. Add a VL equivalent to guard that non-trivial error path.</violation>
</file>
<file name="internal/migrate/withdowntime.go">
<violation number="1" location="internal/migrate/withdowntime.go:108">
P1: When the target CR already exists, this check runs only after deleting the old workload and rebinding its PVC. `c.Create` then returns `AlreadyExists`, leaving the old service stopped with migration state partially changed; preflight the target CR before any destructive mutation.</violation>
<violation number="2" location="internal/migrate/withdowntime.go:207">
P2: When a Service selector shares a key with the Deployment selector but has a different value, this helper accepts it and `CutoverServices` redirects that Service to the target pods. Compare selector values as well as keys before allowing cutover.</violation>
<violation number="3" location="internal/migrate/withdowntime.go:257">
P3: When c.Update fails at index i, CutoverServices reverts indices [0,i) but leaves services[i].Spec.Selector mutated to newSelector in memory even though the failed Update was never persisted. This makes the object's in-memory state disagree with the server, contradicting the function's stated re-invocability on the same slice. Revert services[i] as well before returning the error.</violation>
</file>
<file name="internal/migrate/vm/nodowntime_cluster.go">
<violation number="1" location="internal/migrate/vm/nodowntime_cluster.go:133">
P1: When the target VMCluster already exists, including after a retry, this call deletes its PVCs before the existing-CR/spec check runs. Resolve and validate an existing target before replacing PVCs, or refuse to continue when target PVCs already exist.</violation>
<violation number="2" location="internal/migrate/vm/nodowntime_cluster.go:166">
P1: When the target uses a custom `ServiceSpec.Name`, these readiness checks query the default component name instead of the Service the operator created. Resolve each component's configured service name before calling `WaitEndpointsReady`, otherwise migration times out despite healthy target pods.</violation>
<violation number="3" location="internal/migrate/vm/nodowntime_cluster.go:175">
P0: When the buffer queue does not drain after final cutover, this warning is followed by deletion of the buffer VMAgent, which can discard undelivered writes. Return the error and leave the buffer agent in place for manual recovery instead of deleting it.</violation>
</file>
<file name="cmd/helm-converter/main.go">
<violation number="1" location="cmd/helm-converter/main.go:44">
P2: The legacy no-subcommand path promises existing scripts keep working, but this change drops the previous -chart default of victoria-metrics-single and adds a hard requirement. A script that previously ran `helm-converter -input=vals.yaml -output=out.yaml` (relying on the default chart) now exits with -chart is required, contradicting the comment above main(). Restore the default to keep the backward-compat alias working; migrate can still require -chart explicitly.</violation>
</file>
<file name="internal/migrate/drain.go">
<violation number="1" location="internal/migrate/drain.go:83">
P3: When a pod returns no samples for the queue metric, the poll keeps retrying for the full 6-hour DrainTimeout because `len(samples) == 0` maps to `false, nil`. A wrong metric name or missing `url` dimension is then indistinguishable from a transient state and stalls the migration for hours. After the first few polls confirm the metric persists at zero samples, surface an error instead of polling to timeout.</violation>
</file>
<file name="internal/migrate/convert.go">
<violation number="1" location="internal/migrate/convert.go:14">
P3: ConvertSpec is newly added but never called anywhere in the codebase; only ConvertSpecAtVersion is used (from runMigrate). This is dead code that fetches the latest chart defaults, which is precisely the behavior migrate intentionally avoids. Drop it (or add a caller) to avoid maintaining a second, subtly-wrong conversion entry point.</violation>
</file>
<file name="internal/migrate/alias.go">
<violation number="1" location="internal/migrate/alias.go:57">
P3: When the caller supplies ports with empty Protocol/TargetPort, the API server defaults them on the persisted Service, so a later reuse of the same owned alias compares defaulted ports (from `existing`) against non-defaulted ports (from `aliasPorts`) and spuriously fails the equality check. Normalize the caller's ports (default Protocol to TCP and TargetPort to Port) before building `aliasPorts` and comparing, or make `servicePortsEqual` treat empty Protocol as `TCP` and empty TargetPort as equal to `Port`.</violation>
</file>
<file name="docs/CHANGELOG.md">
<violation number="1" location="docs/CHANGELOG.md:23">
P2: Custom agent: **Changelog Review Agent**
The new `helm-converter migrate` changelog entry has no issue or PR reference. Add the relevant migration issue/PR link to satisfy the mandatory References section.</violation>
<violation number="2" location="docs/CHANGELOG.md:25">
P2: Custom agent: **Changelog Review Agent**
The `NoDowntime` changelog bullet exposes implementation mechanics instead of only the user-visible outcome. Summarize it as a no-downtime cutover that keeps the existing release serving writes while the operator-managed CR is prepared, then switches traffic without requiring service downtime; keep CSI snapshots, buffering, and PVC force-merging in `helm-converter.md`.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| } | ||
| } | ||
|
|
||
| if err := c.Create(ctx, target); err != nil { |
Contributor
There was a problem hiding this comment.
P0: When the target CR already exists, including after a retry, this call runs only after deleting the old workloads and rebinding their PVCs. Create then returns AlreadyExists, leaving the migration without the new setup; reject or validate an existing target before any destructive operation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/withdowntime_cluster.go, line 114:
<comment>When the target CR already exists, including after a retry, this call runs only after deleting the old workloads and rebinding their PVCs. `Create` then returns `AlreadyExists`, leaving the migration without the new setup; reject or validate an existing target before any destructive operation.</comment>
<file context>
@@ -0,0 +1,186 @@
+ }
+ }
+
+ if err := c.Create(ctx, target); err != nil {
+ return fmt.Errorf("cannot create target CR %s/%s: %w", target.GetNamespace(), target.GetName(), err)
+ }
</file context>
|
|
||
| time.Sleep(migrate.WriteQuiesceGracePeriod) | ||
| if err := migrate.WaitQueueDrained(ctx, c, httpClient, agent, vmv1beta1.VMAgentQueueMetricName, 0); err != nil { | ||
| fmt.Printf("warning: buffer VMAgent queue did not fully drain after final cutover, some in-flight writes may need manual replay: %v\n", err) |
Contributor
There was a problem hiding this comment.
P0: When the buffer queue does not drain after final cutover, this warning is followed by deletion of the buffer VMAgent, which can discard undelivered writes. Return the error and leave the buffer agent in place for manual recovery instead of deleting it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/vm/nodowntime_cluster.go, line 175:
<comment>When the buffer queue does not drain after final cutover, this warning is followed by deletion of the buffer VMAgent, which can discard undelivered writes. Return the error and leave the buffer agent in place for manual recovery instead of deleting it.</comment>
<file context>
@@ -0,0 +1,190 @@
+
+ time.Sleep(migrate.WriteQuiesceGracePeriod)
+ if err := migrate.WaitQueueDrained(ctx, c, httpClient, agent, vmv1beta1.VMAgentQueueMetricName, 0); err != nil {
+ fmt.Printf("warning: buffer VMAgent queue did not fully drain after final cutover, some in-flight writes may need manual replay: %v\n", err)
+ }
+ if err := c.Delete(ctx, agent); err != nil && !k8serrors.IsNotFound(err) {
</file context>
| if err := migrate.WaitQueueDrained(ctx, c, httpClient, agent, vmv1beta1.VMAgentQueueMetricName, 0); err != nil { | ||
| fmt.Printf("warning: buffer VMAgent queue did not fully drain after final cutover, some in-flight writes may need manual replay: %v\n", err) | ||
| } | ||
| if err := c.Delete(ctx, agent); err != nil && !k8serrors.IsNotFound(err) { |
Contributor
There was a problem hiding this comment.
P1: In NoDowntimeSingleNode, the buffer agent is deleted (c.Delete(ctx, agent)) with only a warning on failure, even though WaitQueueDrained just reported that the queue did NOT fully drain. If WaitQueueDrained after final cutover fails (returns an error), the code prints a warning but still proceeds to delete the agent, discarding the persistent-queue data that still holds in-flight writes that were never flushed to the new storage. Deleting a VMAgent whose StatefulMode PVC holds undrained writes permanently loses those writes. On the drain-failure path the agent should be left in place for manual recovery rather than deleted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/vm/nodowntime.go, line 177:
<comment>In NoDowntimeSingleNode, the buffer agent is deleted (`c.Delete(ctx, agent)`) with only a warning on failure, even though `WaitQueueDrained` just reported that the queue did NOT fully drain. If `WaitQueueDrained` after final cutover fails (returns an error), the code prints a warning but still proceeds to delete the agent, discarding the persistent-queue data that still holds in-flight writes that were never flushed to the new storage. Deleting a VMAgent whose StatefulMode PVC holds undrained writes permanently loses those writes. On the drain-failure path the agent should be left in place for manual recovery rather than deleted.</comment>
<file context>
@@ -0,0 +1,186 @@
+ if err := migrate.WaitQueueDrained(ctx, c, httpClient, agent, vmv1beta1.VMAgentQueueMetricName, 0); err != nil {
+ fmt.Printf("warning: buffer VMAgent queue did not fully drain after final cutover, some in-flight writes may need manual replay: %v\n", err)
+ }
+ if err := c.Delete(ctx, agent); err != nil && !k8serrors.IsNotFound(err) {
+ fmt.Printf("warning: cannot delete buffer VMAgent %s/%s, please remove it manually: %v\n", agent.Namespace, agent.Name, err)
+ }
</file context>
| if err := waitPodsGone(ctx, c, sts.Namespace, sts.Spec.Selector.MatchLabels); err != nil { | ||
| return fmt.Errorf("component %q: %w", comp.HelmComponentLabel, err) | ||
| } | ||
| if err := rebindClusterPVCs(ctx, c, d.PVCs, comp.TargetPVCTemplateName, comp.TargetWorkloadName, target.GetNamespace()); err != nil { |
Contributor
There was a problem hiding this comment.
P1: When any source PVC is Pending or lacks its bound PV, RebindPVC fails after the old StatefulSet has been deleted. Validate every source PVC is Bound and its PV is readable during preflight, before entering the destructive loop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/withdowntime_cluster.go, line 108:
<comment>When any source PVC is Pending or lacks its bound PV, `RebindPVC` fails after the old StatefulSet has been deleted. Validate every source PVC is Bound and its PV is readable during preflight, before entering the destructive loop.</comment>
<file context>
@@ -0,0 +1,186 @@
+ if err := waitPodsGone(ctx, c, sts.Namespace, sts.Spec.Selector.MatchLabels); err != nil {
+ return fmt.Errorf("component %q: %w", comp.HelmComponentLabel, err)
+ }
+ if err := rebindClusterPVCs(ctx, c, d.PVCs, comp.TargetPVCTemplateName, comp.TargetWorkloadName, target.GetNamespace()); err != nil {
+ return fmt.Errorf("component %q: %w", comp.HelmComponentLabel, err)
+ }
</file context>
| if err != nil { | ||
| return err | ||
| } | ||
| for i := 0; i < len(byOrdinal); i++ { |
Contributor
There was a problem hiding this comment.
P1: When a source StatefulSet has a missing tail PVC, this loop treats the remaining ordinals as complete and lets the target create a fresh empty claim for the missing ordinal. Pass the expected replica count and require every ordinal from 0 through replicas-1 before rebinding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/withdowntime_cluster.go, line 154:
<comment>When a source StatefulSet has a missing tail PVC, this loop treats the remaining ordinals as complete and lets the target create a fresh empty claim for the missing ordinal. Pass the expected replica count and require every ordinal from 0 through `replicas-1` before rebinding.</comment>
<file context>
@@ -0,0 +1,186 @@
+ if err != nil {
+ return err
+ }
+ for i := 0; i < len(byOrdinal); i++ {
+ pvc, ok := byOrdinal[i]
+ if !ok {
</file context>
| "create %s/%s from the snapshot, backfill, then cut traffic over to it — the old Deployment and PVC are never deleted. "+ | ||
| "Service %s/%s serves both reads and writes, and the buffer agent only handles writes, so queries against %[1]s/%[2]s "+ | ||
| "will fail until final cutover; point read-only clients at %s/%s in the meantime if that matters\n", | ||
| oldSvc.Namespace, oldSvc.Name, pvc.Namespace, pvc.Name, target.GetNamespace(), target.GetName(), target.Namespace, readsAlias) |
Contributor
There was a problem hiding this comment.
P3: The plan fmt.Printf in NoDowntimeSingleNode has 10 argument-consuming %s verbs but only 8 arguments. The trailing "point read-only clients at %s/%s" prints %!s(MISSING)%!s(MISSING), and the "Service %s/%s" pair receives readsAlias instead of target.Name, so the printed migration plan misreports the Service name during a destructive operation. Add the missing target.Name argument.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/vl/nodowntime.go, line 48:
<comment>The plan `fmt.Printf` in NoDowntimeSingleNode has 10 argument-consuming `%s` verbs but only 8 arguments. The trailing "point read-only clients at %s/%s" prints `%!s(MISSING)%!s(MISSING)`, and the "Service %s/%s" pair receives `readsAlias` instead of `target.Name`, so the printed migration plan misreports the Service name during a destructive operation. Add the missing `target.Name` argument.</comment>
<file context>
@@ -0,0 +1,184 @@
+ "create %s/%s from the snapshot, backfill, then cut traffic over to it — the old Deployment and PVC are never deleted. "+
+ "Service %s/%s serves both reads and writes, and the buffer agent only handles writes, so queries against %[1]s/%[2]s "+
+ "will fail until final cutover; point read-only clients at %s/%s in the meantime if that matters\n",
+ oldSvc.Namespace, oldSvc.Name, pvc.Namespace, pvc.Name, target.GetNamespace(), target.GetName(), target.Namespace, readsAlias)
+
+ if opts.DryRun {
</file context>
Suggested change
| oldSvc.Namespace, oldSvc.Name, pvc.Namespace, pvc.Name, target.GetNamespace(), target.GetName(), target.Namespace, readsAlias) | |
| oldSvc.Namespace, oldSvc.Name, pvc.Namespace, pvc.Name, target.GetNamespace(), target.GetName(), target.Namespace, target.Name, target.Namespace, readsAlias) |
| svc.Spec.Selector = newSelector | ||
| if err := c.Update(ctx, svc); err != nil { | ||
| updateErr := fmt.Errorf("cannot patch Service %s/%s selector: %w", svc.Namespace, svc.Name, err) | ||
| for j := range i { |
Contributor
There was a problem hiding this comment.
P3: When c.Update fails at index i, CutoverServices reverts indices [0,i) but leaves services[i].Spec.Selector mutated to newSelector in memory even though the failed Update was never persisted. This makes the object's in-memory state disagree with the server, contradicting the function's stated re-invocability on the same slice. Revert services[i] as well before returning the error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/withdowntime.go, line 257:
<comment>When c.Update fails at index i, CutoverServices reverts indices [0,i) but leaves services[i].Spec.Selector mutated to newSelector in memory even though the failed Update was never persisted. This makes the object's in-memory state disagree with the server, contradicting the function's stated re-invocability on the same slice. Revert services[i] as well before returning the error.</comment>
<file context>
@@ -0,0 +1,288 @@
+ svc.Spec.Selector = newSelector
+ if err := c.Update(ctx, svc); err != nil {
+ updateErr := fmt.Errorf("cannot patch Service %s/%s selector: %w", svc.Namespace, svc.Name, err)
+ for j := range i {
+ services[j].Spec.Selector = originalSelectors[j]
+ if revertErr := c.Update(ctx, services[j]); revertErr != nil {
</file context>
| return false, fmt.Errorf("scraping %s: %w", addr, err) | ||
| } | ||
| samples := metrics[metricName] | ||
| if len(samples) == 0 { |
Contributor
There was a problem hiding this comment.
P3: When a pod returns no samples for the queue metric, the poll keeps retrying for the full 6-hour DrainTimeout because len(samples) == 0 maps to false, nil. A wrong metric name or missing url dimension is then indistinguishable from a transient state and stalls the migration for hours. After the first few polls confirm the metric persists at zero samples, surface an error instead of polling to timeout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/drain.go, line 83:
<comment>When a pod returns no samples for the queue metric, the poll keeps retrying for the full 6-hour DrainTimeout because `len(samples) == 0` maps to `false, nil`. A wrong metric name or missing `url` dimension is then indistinguishable from a transient state and stalls the migration for hours. After the first few polls confirm the metric persists at zero samples, surface an error instead of polling to timeout.</comment>
<file context>
@@ -0,0 +1,98 @@
+ return false, fmt.Errorf("scraping %s: %w", addr, err)
+ }
+ samples := metrics[metricName]
+ if len(samples) == 0 {
+ return false, nil
+ }
</file context>
|
|
||
| // ConvertSpec reads valuesFile, merges it over the chart's latest published defaults, and | ||
| // converts it to the operator CR for the given chart and target name/namespace. | ||
| func ConvertSpec(chart Chart, valuesFile, targetName, targetNamespace string) (any, error) { |
Contributor
There was a problem hiding this comment.
P3: ConvertSpec is newly added but never called anywhere in the codebase; only ConvertSpecAtVersion is used (from runMigrate). This is dead code that fetches the latest chart defaults, which is precisely the behavior migrate intentionally avoids. Drop it (or add a caller) to avoid maintaining a second, subtly-wrong conversion entry point.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/convert.go, line 14:
<comment>ConvertSpec is newly added but never called anywhere in the codebase; only ConvertSpecAtVersion is used (from runMigrate). This is dead code that fetches the latest chart defaults, which is precisely the behavior migrate intentionally avoids. Drop it (or add a caller) to avoid maintaining a second, subtly-wrong conversion entry point.</comment>
<file context>
@@ -0,0 +1,113 @@
+
+// ConvertSpec reads valuesFile, merges it over the chart's latest published defaults, and
+// converts it to the operator CR for the given chart and target name/namespace.
+func ConvertSpec(chart Chart, valuesFile, targetName, targetNamespace string) (any, error) {
+ defaults, err := converter.FetchChartDefaults(string(chart))
+ if err != nil {
</file context>
| return nil, fmt.Errorf("existing alias Service %s/%s has type %q, expected %q — refusing to reuse it", | ||
| namespace, name, existing.Spec.Type, corev1.ServiceTypeClusterIP) | ||
| } | ||
| if !reflect.DeepEqual(existing.Spec.Selector, selector) || !servicePortsEqual(existing.Spec.Ports, aliasPorts) { |
Contributor
There was a problem hiding this comment.
P3: When the caller supplies ports with empty Protocol/TargetPort, the API server defaults them on the persisted Service, so a later reuse of the same owned alias compares defaulted ports (from existing) against non-defaulted ports (from aliasPorts) and spuriously fails the equality check. Normalize the caller's ports (default Protocol to TCP and TargetPort to Port) before building aliasPorts and comparing, or make servicePortsEqual treat empty Protocol as TCP and empty TargetPort as equal to Port.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/alias.go, line 57:
<comment>When the caller supplies ports with empty Protocol/TargetPort, the API server defaults them on the persisted Service, so a later reuse of the same owned alias compares defaulted ports (from `existing`) against non-defaulted ports (from `aliasPorts`) and spuriously fails the equality check. Normalize the caller's ports (default Protocol to TCP and TargetPort to Port) before building `aliasPorts` and comparing, or make `servicePortsEqual` treat empty Protocol as `TCP` and empty TargetPort as equal to `Port`.</comment>
<file context>
@@ -0,0 +1,87 @@
+ return nil, fmt.Errorf("existing alias Service %s/%s has type %q, expected %q — refusing to reuse it",
+ namespace, name, existing.Spec.Type, corev1.ServiceTypeClusterIP)
+ }
+ if !reflect.DeepEqual(existing.Spec.Selector, selector) || !servicePortsEqual(existing.Spec.Ports, aliasPorts) {
+ return nil, fmt.Errorf("a Service %s/%s already exists but its selector/ports don't match the backend this migration expects — "+
+ "it may be left over from a previous migration attempt against a different backend; "+
</file context>
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.
proposing instead of adding UseLegacyNaming to extend helm-converter with migrate functionality that supports two migrate strategies for VM and VL instances: