helm-converter: support migrate

[AndrewChubatiuk]

proposing instead of adding UseLegacyNaming to extend helm-converter with migrate functionality that supports two migrate strategies for VM and VL instances:

• No Downtime: create agent for buffering purposes, make old setup read-only, make data snapshot, create new instance from this snapshot, delete old setup
• With Downtime: create agent for buffering purposes, stop old setup, create PVC from old PVs and create new setup

[cubic-dev-ai]

37 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/migrate/discover.go">

<violation number="1" location="internal/migrate/discover.go:35">
P0: Default cluster PVCs are not returned because this selector requires Helm and component labels that the chart does not put on `volumeClaimTemplates`. Discover PVCs from the discovered StatefulSet's claim-template names/ordinals (or another ownership-based relation) so cluster migration preserves existing data.</violation>
</file>

<file name="internal/migrate/vm/cluster.go">

<violation number="1" location="internal/migrate/vm/cluster.go:38">
P1: An explicit `storage.emptyDir` is not PVC-backed, but this assigns a PVC template name and migrates old PVCs anyway. The new vmstorage/vmselect runs with an empty directory while source PVCs have been deleted/rebound but are unused; distinguish `EmptyDir` and validate incompatible source PVCs before deleting old workloads.</violation>
</file>

<file name="internal/migrate/vm/agent.go">

<violation number="1" location="internal/migrate/vm/agent.go:21">
P2: A zero or negative `--agent-buffer-size` passes parsing, yielding a buffer agent with no usable persistent queue (or a rejected PVC) after migration has already created the alias Service. Reject non-positive quantities so NoDowntime cannot proceed without its required write buffer.</violation>
</file>

<file name="internal/migrate/vm/nodowntime_cluster.go">

<violation number="1" location="internal/migrate/vm/nodowntime_cluster.go:87">
P0: Writes accepted while the target is being created can be lost: the buffer agent continues forwarding them to the old writable vminsert after the storage snapshot. Make the old write path reject/stop writes after the agent is ready so those samples remain queued and are replayed to the target.</violation>

<violation number="2" location="internal/migrate/vm/nodowntime_cluster.go:96">
P2: Retrying after a failure leaves the existing buffer VMAgent unusable because it is never fetched after `AlreadyExists`; its later update lacks a ResourceVersion. Fetch and validate the existing agent before continuing, or fail early with a cleanup instruction.</violation>

<violation number="3" location="internal/migrate/vm/nodowntime_cluster.go:100">
P1: Incoming writes can receive no Service endpoints during buffer-agent startup, so this NoDowntime path has an availability gap. Wait until the VMAgent is operational and has ready endpoints before redirecting `oldInsertSvc`.</violation>
</file>

<file name="internal/migrate/vl/nodowntime_cluster.go">

<violation number="1" location="internal/migrate/vl/nodowntime_cluster.go:93">
P0: Writes accepted after the storage snapshots complete can be lost at cutover: this agent forwards them successfully to old vlinsert, so they leave its queue, but the new PVCs were already cloned from earlier snapshots. Transition old vlinsert to read-only/unavailable immediately after snapshotting so subsequent writes remain buffered until the agent is repointed to the target.</violation>
</file>

<file name="internal/migrate/alias.go">

<violation number="1" location="internal/migrate/alias.go:23">
P1: No-downtime migration fails before buffering for source Services configured as NodePort or LoadBalancer: copied `ServicePort.NodePort` is invalid on this default ClusterIP alias. Build alias ports with `NodePort` cleared (or explicitly choose a compatible Service type).</violation>

<violation number="2" location="internal/migrate/alias.go:30">
P1: A pre-existing Service with this generated name is silently reused even when it does not match the requested backend. Validate its selector and ports (or mark aliases with an identifying annotation) and fail on mismatch, rather than sending buffered writes through an arbitrary Service.</violation>
</file>

<file name="internal/migrate/pvcrebind_test.go">

<violation number="1" location="internal/migrate/pvcrebind_test.go:98">
P3: This only asserts some error occurred, but doesn't verify the old PVC was actually deleted (NotFound). If RebindPVC returns a different error (e.g., a permission or timeout error), this assertion still passes, masking the root cause. Use `require.True(t, k8serrors.IsNotFound(err))` to validate the expected outcome.</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>

<file name="internal/migrate/convert.go">

<violation number="1" location="internal/migrate/convert.go:20">
P1: Migrating an older Helm release can create a target CR with newer-chart defaults rather than the source release's effective configuration. This is particularly unsafe for `WithDowntime`, which deletes/rebinds the old workload after this conversion; accept or discover the installed chart version and fetch its matching defaults (or require fully rendered values).</violation>
</file>

<file name="internal/migrate/vm/nodowntime.go">

<violation number="1" location="internal/migrate/vm/nodowntime.go:26">
P3: Future changes to the no-downtime lifecycle require parallel edits in two near-identical implementations, risking strategy drift. Consider moving common orchestration into `internal/migrate` behind small VM/VL adapters, leaving only agent construction and URL selection engine-specific.</violation>

<violation number="2" location="internal/migrate/vm/nodowntime.go:70">
P0: Writes received between the snapshot and agent repoint are missing from the new VMSingle: the buffer forwards them successfully to old storage, so its persistent queue is empty when cutover occurs. Make old storage reject writes (or otherwise retain this interval durably in the agent) before snapshotting, then replay only after the new target is ready.</violation>

<violation number="3" location="internal/migrate/vm/nodowntime.go:74">
P2: A rerun after a partial migration cannot repoint its existing buffer agent: the ignored `AlreadyExists` path never reloads its ResourceVersion, so the later update fails. Fetch the existing CR before continuing (and reconcile it to the expected initial configuration if resumability is intended).</violation>

<violation number="4" location="internal/migrate/vm/nodowntime.go:78">
P1: The first cutover can leave the write Service with no endpoints while the operator creates and starts the buffer VMAgent, breaking the advertised no-downtime path. Wait for the agent to become operational and have ready endpoints before redirecting `oldSvc`.</violation>
</file>

<file name="internal/migrate/vl/nodowntime.go">

<violation number="1" location="internal/migrate/vl/nodowntime.go:69">
P0: Writes accepted by old storage after the snapshot are lost from the new instance: VLAgent forwards them to the still-writable alias, then only retains data when that endpoint is unavailable. Make old storage reject writes after traffic reaches the agent (while retaining the alias for force-merge), so post-snapshot data remains queued and is replayed after repointing.</violation>

<violation number="2" location="internal/migrate/vl/nodowntime.go:77">
P1: The advertised no-downtime path briefly drops writes because this selector switches before the new VLAgent has any Ready endpoint. Wait for the buffer agent workload/endpoints to become ready before redirecting `oldSvc`.</violation>
</file>

<file name="internal/migrate/snapshot.go">

<violation number="1" location="internal/migrate/snapshot.go:57">
P2: Snapshot read failures are hidden until the full timeout, so migrations report a misleading timeout instead of RBAC/API/deletion errors. Return the `Get` error from the poll callback so the caller can revert traffic and show the actual failure.</violation>

<violation number="2" location="internal/migrate/snapshot.go:79">
P1: Block-mode PVC migrations cannot bind because the restored claim defaults to `Filesystem` instead of preserving the snapshot source mode. Copy `source.Spec.VolumeMode` into the target PVC spec.</violation>
</file>

<file name="internal/migrate/vm/urls.go">

<violation number="1" location="internal/migrate/vm/urls.go:18">
P2: `insertRemoteWriteURL` hardcodes the path suffix but VMCluster.GetRemoteWriteURL() uses `BuildPathWithPrefixFlag` to respect `--http.pathPrefix` configured on vminsert. A service-based URL builder cannot replicate that because the Service object doesn't carry the VMCluster's ExtraArgs. Either document this limitation clearly in the comment and note the contexts where the hardcoded path is acceptable, or pass the prefix flag through so the URL can match what VMCluster would produce.</violation>
</file>

<file name="internal/migrate/drain.go">

<violation number="1" location="internal/migrate/drain.go:45">
P1: A metrics response missing the queue gauge is treated as drained, so migration can delete the buffer agent and cut over while its queue was never verified. Keep polling (or return an error) unless every discovered pod exposes at least one `metricName` sample.</violation>

<violation number="2" location="internal/migrate/drain.go:51">
P1: The first successful scrape can finish migration before the buffer agent applies its new `RemoteWrite` destination. Wait for the agent rollout/config generation after updating it, then begin queue-drain polling; otherwise writes accepted during cutover can be lost when the old-config agent is deleted.</violation>
</file>

<file name="internal/migrate/nodowntime_cluster.go">

<violation number="1" location="internal/migrate/nodowntime_cluster.go:41">
P2: A duplicate ordinal is silently dropped, allowing migration to continue with one storage PVC unsnapshotted. Treat a map length smaller than the discovered PVC count as an invalid component before iterating.</violation>
</file>

<file name="internal/converter/converter.go">

<violation number="1" location="internal/converter/converter.go:933">
P2: Converted Helm `securityContext.seLinuxOptions` or `windowsOptions` is silently discarded. Promote these fields to `PodSecurityContext` with the same nil-only precedence rule used for the other container-level settings.</violation>
</file>

<file name="internal/migrate/withdowntime_cluster.go">

<violation number="1" location="internal/migrate/withdowntime_cluster.go:64">
P1: Unsupported later components leave earlier components stopped because this loop mutates before validating every component's topology and storage requirements. Preflight all `SingleStatefulSet` and PVC-template checks before the first `Delete`.</violation>

<violation number="2" location="internal/migrate/withdowntime_cluster.go:67">
P2: Deployment-mode components with persistent storage leave their source PVC/PV orphaned because this branch skips the PVC compatibility check and `RebindPVC`. Handle standalone Deployment claims here, or reject that chart shape before deleting it.</violation>

<violation number="3" location="internal/migrate/withdowntime_cluster.go:154">
P1: Multiple PVCs with the same ordinal are silently discarded by the map, leaving one old claim unrebound. Reject duplicate ordinals instead of overwriting the first entry.</violation>
</file>

<file name="internal/migrate/pvcrebind.go">

<violation number="1" location="internal/migrate/pvcrebind.go:41">
P2: Migrations from `Delete` permanently change later cleanup behavior: deleting the new PVC leaves the PV and backing storage orphaned. Save the original reclaim policy and restore it after the target has bound.</violation>

<violation number="2" location="internal/migrate/pvcrebind.go:52">
P1: A colliding target PVC bound to another PV is treated as a successful retry, so migration can start the target and cut over onto unrelated storage. Verify `existing.Spec.VolumeName == pvName` before returning it.</violation>

<violation number="3" location="internal/migrate/pvcrebind.go:90">
P1: Block-mode source volumes cannot rebind because this target PVC defaults to `Filesystem`. Copy `sourcePVC.Spec.VolumeMode` into the target spec.</violation>

<violation number="4" location="internal/migrate/pvcrebind.go:100">
P3: PVC polling hides API failures for up to two minutes and reports them as timeouts. Return non-NotFound `Get` errors from each callback so migration fails with the actual cause.</violation>
</file>

<file name="internal/migrate/withdowntime.go">

<violation number="1" location="internal/migrate/withdowntime.go:109">
P2: The `dependentConfigsOf` function and `deleteDependentConfigs` function are both referenced from `withdowntime.go` (single-node) and `withdowntime_cluster.go` (cluster), which is fine. However, `dependentConfigsOf` iterates over `podSpec.Volumes`, `podSpec.Containers[*].EnvFrom`, and `podSpec.Containers[*].Env` but does **not** check `podSpec.InitContainers` or `podSpec.EphemeralContainers`. If a Helm chart puts ConfigMap/Secret references in an init container (e.g., init container pulling config before the main app), those dependent configs would not be deleted and the migration could leave behind stale objects with the Helm labels. Consider scanning init containers and ephemeral containers as well.</violation>

<violation number="2" location="internal/migrate/withdowntime.go:123">
P3: ConfigMaps or Secrets used only by an init container or projected volume are not treated as dependencies, leaving Helm-release artifacts after migration. Include `InitContainers` and `VolumeSource.Projected.Sources` in the reference scan.</violation>

<violation number="3" location="internal/migrate/withdowntime.go:178">
P2: Authentication, RBAC, or API failures while waiting become a misleading 15-minute readiness timeout after old workload deletion. Retry only NotFound; return other `Get` errors immediately.</violation>

<violation number="4" location="internal/migrate/withdowntime.go:191">
P2: A failed update of a later Service leaves earlier Services on new pods while remaining Services select deleted Helm pods. Preserve original selectors and roll back Services already updated before returning the error.</violation>
</file>

<sub>Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic</sub>

[cubic-dev-ai]

P0: Default cluster PVCs are not returned because this selector requires Helm and component labels that the chart does not put on volumeClaimTemplates. Discover PVCs from the discovered StatefulSet's claim-template names/ordinals (or another ownership-based relation) so cluster migration preserves existing data.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/discover.go, line 35:

<comment>Default cluster PVCs are not returned because this selector requires Helm and component labels that the chart does not put on `volumeClaimTemplates`. Discover PVCs from the discovered StatefulSet's claim-template names/ordinals (or another ownership-based relation) so cluster migration preserves existing data.</comment>

<file context>
@@ -0,0 +1,109 @@
+// release, via the app.kubernetes.io/component label the VictoriaMetrics Helm charts set.
+func DiscoverComponent(ctx context.Context, c client.Client, namespace, releaseName, component string) (*Discovered, error) {
+	l := helmLabels(releaseName)
+	l["app.kubernetes.io/component"] = component
+	return discover(ctx, c, namespace, releaseName, l)
+}
</file context>

[cubic-dev-ai]

P0: Writes accepted while the target is being created can be lost: the buffer agent continues forwarding them to the old writable vminsert after the storage snapshot. Make the old write path reject/stop writes after the agent is ready so those samples remain queued and are replayed to the target.

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 87:

<comment>Writes accepted while the target is being created can be lost: the buffer agent continues forwarding them to the old writable vminsert after the storage snapshot. Make the old write path reject/stop writes after the agent is ready so those samples remain queued and are replayed to the target.</comment>

<file context>
@@ -0,0 +1,142 @@
+		return fmt.Errorf("cannot create alias Service to preserve a stable path to the old vminsert: %w", err)
+	}
+
+	oldWriteURL, err := insertRemoteWriteURL(aliasSvc)
+	if err != nil {
+		return fmt.Errorf("cannot compute old vminsert's write URL: %w", err)
</file context>

[cubic-dev-ai]

P0: Writes accepted after the storage snapshots complete can be lost at cutover: this agent forwards them successfully to old vlinsert, so they leave its queue, but the new PVCs were already cloned from earlier snapshots. Transition old vlinsert to read-only/unavailable immediately after snapshotting so subsequent writes remain buffered until the agent is repointed to the target.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/vl/nodowntime_cluster.go, line 93:

<comment>Writes accepted after the storage snapshots complete can be lost at cutover: this agent forwards them successfully to old vlinsert, so they leave its queue, but the new PVCs were already cloned from earlier snapshots. Transition old vlinsert to read-only/unavailable immediately after snapshotting so subsequent writes remain buffered until the agent is repointed to the target.</comment>

<file context>
@@ -0,0 +1,143 @@
+		return fmt.Errorf("cannot compute old vlinsert's write URL: %w", err)
+	}
+	agentName := target.PrefixedName(vmv1beta1.ClusterComponentInsert) + "-migration-buffer"
+	agent, err := newBufferAgent(agentName, target.Namespace, oldWriteURL, opts.AgentBufferSize)
+	if err != nil {
+		return fmt.Errorf("cannot build buffer agent spec: %w", err)
</file context>

[cubic-dev-ai]

P0: Writes received between the snapshot and agent repoint are missing from the new VMSingle: the buffer forwards them successfully to old storage, so its persistent queue is empty when cutover occurs. Make old storage reject writes (or otherwise retain this interval durably in the agent) before snapshotting, then replay only after the new target is ready.

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 70:

<comment>Writes received between the snapshot and agent repoint are missing from the new VMSingle: the buffer forwards them successfully to old storage, so its persistent queue is empty when cutover occurs. Make old storage reject writes (or otherwise retain this interval durably in the agent) before snapshotting, then replay only after the new target is ready.</comment>

<file context>
@@ -0,0 +1,133 @@
+		return fmt.Errorf("cannot compute old storage's write URL: %w", err)
+	}
+	agentName := target.PrefixedName() + "-migration-buffer"
+	agent, err := newBufferAgent(agentName, target.Namespace, oldWriteURL, opts.AgentBufferSize)
+	if err != nil {
+		return fmt.Errorf("cannot build buffer agent spec: %w", err)
</file context>

[cubic-dev-ai]

P0: Writes accepted by old storage after the snapshot are lost from the new instance: VLAgent forwards them to the still-writable alias, then only retains data when that endpoint is unavailable. Make old storage reject writes after traffic reaches the agent (while retaining the alias for force-merge), so post-snapshot data remains queued and is replayed after repointing.

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 69:

<comment>Writes accepted by old storage after the snapshot are lost from the new instance: VLAgent forwards them to the still-writable alias, then only retains data when that endpoint is unavailable. Make old storage reject writes after traffic reaches the agent (while retaining the alias for force-merge), so post-snapshot data remains queued and is replayed after repointing.</comment>

<file context>
@@ -0,0 +1,132 @@
+		return fmt.Errorf("cannot compute old storage's write URL: %w", err)
+	}
+	agentName := target.PrefixedName() + "-migration-buffer"
+	agent, err := newBufferAgent(agentName, target.Namespace, oldWriteURL, opts.AgentBufferSize)
+	if err != nil {
+		return fmt.Errorf("cannot build buffer agent spec: %w", err)
</file context>

[cubic-dev-ai]

P3: This only asserts some error occurred, but doesn't verify the old PVC was actually deleted (NotFound). If RebindPVC returns a different error (e.g., a permission or timeout error), this assertion still passes, masking the root cause. Use require.True(t, k8serrors.IsNotFound(err)) to validate the expected outcome.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/pvcrebind_test.go, line 98:

<comment>This only asserts some error occurred, but doesn't verify the old PVC was actually deleted (NotFound). If RebindPVC returns a different error (e.g., a permission or timeout error), this assertion still passes, masking the root cause. Use `require.True(t, k8serrors.IsNotFound(err))` to validate the expected outcome.</comment>

<file context>
@@ -0,0 +1,109 @@
+	// old PVC must be gone
+	var check corev1.PersistentVolumeClaim
+	err = c.Get(ctx, types.NamespacedName{Name: "old-release-data", Namespace: "default"}, &check)
+	assert.Error(t, err)
+
+	// PV must survive, now Retain and bound to the new PVC's claim.
</file context>

[cubic-dev-ai]

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>

[cubic-dev-ai]

P3: Future changes to the no-downtime lifecycle require parallel edits in two near-identical implementations, risking strategy drift. Consider moving common orchestration into internal/migrate behind small VM/VL adapters, leaving only agent construction and URL selection engine-specific.

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 26:

<comment>Future changes to the no-downtime lifecycle require parallel edits in two near-identical implementations, risking strategy drift. Consider moving common orchestration into `internal/migrate` behind small VM/VL adapters, leaving only agent construction and URL selection engine-specific.</comment>

<file context>
@@ -0,0 +1,133 @@
+// NoDowntimeSingleNode runs the NoDowntime strategy for victoria-metrics-single: never
+// touches the old Deployment/PVC, provisions fresh storage from a CSI snapshot while
+// buffering writes through a VMAgent proxy, then repoints traffic once caught up.
+func NoDowntimeSingleNode(ctx context.Context, c client.Client, httpClient *http.Client, opts migrate.Options, target *vmv1beta1.VMSingle) error {
+	d, err := migrate.Discover(ctx, c, opts.Namespace, opts.ReleaseName)
+	if err != nil {
</file context>

[cubic-dev-ai]

P3: PVC polling hides API failures for up to two minutes and reports them as timeouts. Return non-NotFound Get errors from each callback so migration fails with the actual cause.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/pvcrebind.go, line 100:

<comment>PVC polling hides API failures for up to two minutes and reports them as timeouts. Return non-NotFound `Get` errors from each callback so migration fails with the actual cause.</comment>

<file context>
@@ -0,0 +1,125 @@
+
+	if err := wait.PollUntilContextTimeout(ctx, PVCBindPollInterval, PVCBindTimeout, true, func(ctx context.Context) (bool, error) {
+		var check corev1.PersistentVolumeClaim
+		if err := c.Get(ctx, types.NamespacedName{Name: targetName, Namespace: targetNamespace}, &check); err != nil {
+			return false, nil
+		}
</file context>

[cubic-dev-ai]

P3: ConfigMaps or Secrets used only by an init container or projected volume are not treated as dependencies, leaving Helm-release artifacts after migration. Include InitContainers and VolumeSource.Projected.Sources in the reference scan.

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 123:

<comment>ConfigMaps or Secrets used only by an init container or projected volume are not treated as dependencies, leaving Helm-release artifacts after migration. Include `InitContainers` and `VolumeSource.Projected.Sources` in the reference scan.</comment>

<file context>
@@ -0,0 +1,217 @@
+	for _, v := range podSpec.Volumes {
+		addVolume(v)
+	}
+	for _, cnt := range podSpec.Containers {
+		for _, ef := range cnt.EnvFrom {
+			if ef.ConfigMapRef != nil {
</file context>