helm-converter: support migrate - #2524
Closed
AndrewChubatiuk wants to merge 1 commit into
Closed
Conversation
AndrewChubatiuk
marked this pull request as draft
August 20, 2026 12:19
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 81 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
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/convert.go">
<violation number="1" location="internal/migrate/convert.go:24">
P1: Target names from 28 through 35 pass this check but make the buffer agent's generated StatefulSet pod hostname exceed Kubernetes's 63-character limit, so migration fails when creating the buffer agent. Set the limit to the maximum derived-name-safe length (27 characters, including the StatefulSet ordinal).</violation>
</file>
<file name="internal/migrate/drain.go">
<violation number="1" location="internal/migrate/drain.go:64">
P1: When a buffer agent has multiple replicas and one pod is unready, this loop ignores that pod's queue and can report the migration drained after only the remaining ready pods reach zero. Deleting the buffer agent then removes the workload before the unready pod's queued data is flushed, risking data loss; require every expected buffer-agent pod to be accounted for, or fail/continue draining while any pod is unready.</violation>
</file>
<file name="internal/migrate/utils.go">
<violation number="1" location="internal/migrate/utils.go:343">
P1: When another custom resource references an old release's ConfigMap or Secret, this cleanup deletes it because `deleteDependentConfigs` only scans built-in workloads and ServiceAccounts. Enumerate supported CRD references before deletion, or leave candidates with unverified consumers undeleted.</violation>
</file>
<file name="internal/migrate/cluster.go">
<violation number="1" location="internal/migrate/cluster.go:612">
P1: NoDowntime never makes the old insert/storage path read-only before snapshotting. Existing connections can write after the snapshot, and those acknowledged writes are missing from the restored PVC; block old writes before the snapshot or fail instead of proceeding.</violation>
<violation number="2" location="internal/migrate/cluster.go:701">
P1: When the target cluster uses a custom component Service name, this check probes the default `PrefixedName` Service instead of the Service resolved from the target spec. The migration can time out after redirecting the old Service to the buffer; resolve and wait on each target component's configured Service name before final cutover.</violation>
</file>
<file name="internal/migrate/singlenode.go">
<violation number="1" location="internal/migrate/singlenode.go:271">
P2: When a WithDowntime run fails after deleting the old Deployment but before target readiness, a retry enters `resuming` and never records the old pod's dependent ConfigMaps or Secrets. Preserve those references before deleting the Deployment or rediscover them during resume so interrupted migrations still clean up the old Helm resources.</violation>
</file>
<file name="internal/migrate/vm/cluster.go">
<violation number="1" location="internal/migrate/vm/cluster.go:73">
P2: When an existing owned buffer agent has no `resources.requests.storage`, `AgentMatches` dereferences a nil `Storage()` result and crashes the migration. Check both storage quantities for nil before comparing them so an invalid existing agent returns the normal mismatch error.</violation>
</file>
<file name="internal/migrate/urls.go">
<violation number="1" location="internal/migrate/urls.go:267">
P3: `forceMergeURL` has no production caller, so this URL-building path is dead while force-merge uses `podutil.DiscoverEndpointAddrs` directly. Remove these unused helpers or use them in the production force-merge path.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| return convertSpec(chart, defaults, valuesFile, targetName, targetNamespace) | ||
| } | ||
|
|
||
| const maxTargetNameLength = 35 |
Contributor
There was a problem hiding this comment.
P1: Target names from 28 through 35 pass this check but make the buffer agent's generated StatefulSet pod hostname exceed Kubernetes's 63-character limit, so migration fails when creating the buffer agent. Set the limit to the maximum derived-name-safe length (27 characters, including the StatefulSet ordinal).
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 24:
<comment>Target names from 28 through 35 pass this check but make the buffer agent's generated StatefulSet pod hostname exceed Kubernetes's 63-character limit, so migration fails when creating the buffer agent. Set the limit to the maximum derived-name-safe length (27 characters, including the StatefulSet ordinal).</comment>
<file context>
@@ -0,0 +1,63 @@
+ return convertSpec(chart, defaults, valuesFile, targetName, targetNamespace)
+}
+
+const maxTargetNameLength = 35
+
+func convertSpec(chart Chart, defaults []byte, valuesFile, targetName, targetNamespace string) (any, error) {
</file context>
Suggested change
| const maxTargetNameLength = 35 | |
| const maxTargetNameLength = 27 |
| consecutiveTransientFailures := 0 | ||
| consecutiveDrained := 0 | ||
| err := wait.PollUntilContextTimeout(ctx, DrainPollInterval, DrainTimeout, true, func(ctx context.Context) (bool, error) { | ||
| addrs, err := podutil.DiscoverEndpointAddrs(ctx, c, agent.GetNamespace(), agent.PrefixedName(), "http", agent.ProbeScheme(), agent.GetMetricsPath()) |
Contributor
There was a problem hiding this comment.
P1: When a buffer agent has multiple replicas and one pod is unready, this loop ignores that pod's queue and can report the migration drained after only the remaining ready pods reach zero. Deleting the buffer agent then removes the workload before the unready pod's queued data is flushed, risking data loss; require every expected buffer-agent pod to be accounted for, or fail/continue draining while any pod is unready.
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 64:
<comment>When a buffer agent has multiple replicas and one pod is unready, this loop ignores that pod's queue and can report the migration drained after only the remaining ready pods reach zero. Deleting the buffer agent then removes the workload before the unready pod's queued data is flushed, risking data loss; require every expected buffer-agent pod to be accounted for, or fail/continue draining while any pod is unready.</comment>
<file context>
@@ -0,0 +1,122 @@
+ consecutiveTransientFailures := 0
+ consecutiveDrained := 0
+ err := wait.PollUntilContextTimeout(ctx, DrainPollInterval, DrainTimeout, true, func(ctx context.Context) (bool, error) {
+ addrs, err := podutil.DiscoverEndpointAddrs(ctx, c, agent.GetNamespace(), agent.PrefixedName(), "http", agent.ProbeScheme(), agent.GetMetricsPath())
+ if err != nil {
+ return false, err
</file context>
| } | ||
|
|
||
| for i := range configMaps { | ||
| if referencedInTarget(configMaps[i].Name) || liveCMNames[configMaps[i].Name] { |
Contributor
There was a problem hiding this comment.
P1: When another custom resource references an old release's ConfigMap or Secret, this cleanup deletes it because deleteDependentConfigs only scans built-in workloads and ServiceAccounts. Enumerate supported CRD references before deletion, or leave candidates with unverified consumers undeleted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/utils.go, line 343:
<comment>When another custom resource references an old release's ConfigMap or Secret, this cleanup deletes it because `deleteDependentConfigs` only scans built-in workloads and ServiceAccounts. Enumerate supported CRD references before deletion, or leave candidates with unverified consumers undeleted.</comment>
<file context>
@@ -0,0 +1,515 @@
+ }
+
+ for i := range configMaps {
+ if referencedInTarget(configMaps[i].Name) || liveCMNames[configMaps[i].Name] {
+ continue
+ }
</file context>
| if pathPrefix, err := httpPathPrefix(dc.sts.Spec.Template.Spec.Containers); err != nil { | ||
| fmt.Printf("warning: cannot resolve http.pathPrefix for %s (continuing, snapshot will still be crash-consistent): %v\n", d.Services[0].Name, err) | ||
| } else { | ||
| forceMergeComponentPods(ctx, c, httpClient, opts.Namespace, d.Services[0].Name, "http", pathPrefix, dc.sts.Spec.Template.Spec.Containers) |
Contributor
There was a problem hiding this comment.
P1: NoDowntime never makes the old insert/storage path read-only before snapshotting. Existing connections can write after the snapshot, and those acknowledged writes are missing from the restored PVC; block old writes before the snapshot or fail instead of proceeding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/cluster.go, line 612:
<comment>NoDowntime never makes the old insert/storage path read-only before snapshotting. Existing connections can write after the snapshot, and those acknowledged writes are missing from the restored PVC; block old writes before the snapshot or fail instead of proceeding.</comment>
<file context>
@@ -0,0 +1,886 @@
+ if pathPrefix, err := httpPathPrefix(dc.sts.Spec.Template.Spec.Containers); err != nil {
+ fmt.Printf("warning: cannot resolve http.pathPrefix for %s (continuing, snapshot will still be crash-consistent): %v\n", d.Services[0].Name, err)
+ } else {
+ forceMergeComponentPods(ctx, c, httpClient, opts.Namespace, d.Services[0].Name, "http", pathPrefix, dc.sts.Spec.Template.Spec.Containers)
+ }
+ }
</file context>
| } | ||
| } | ||
|
|
||
| if err := waitEndpointsReady(ctx, c, target.GetNamespace(), target.PrefixedName(vmv1beta1.ClusterComponentInsert)); err != nil { |
Contributor
There was a problem hiding this comment.
P1: When the target cluster uses a custom component Service name, this check probes the default PrefixedName Service instead of the Service resolved from the target spec. The migration can time out after redirecting the old Service to the buffer; resolve and wait on each target component's configured Service name before final cutover.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/cluster.go, line 701:
<comment>When the target cluster uses a custom component Service name, this check probes the default `PrefixedName` Service instead of the Service resolved from the target spec. The migration can time out after redirecting the old Service to the buffer; resolve and wait on each target component's configured Service name before final cutover.</comment>
<file context>
@@ -0,0 +1,886 @@
+ }
+ }
+
+ if err := waitEndpointsReady(ctx, c, target.GetNamespace(), target.PrefixedName(vmv1beta1.ClusterComponentInsert)); err != nil {
+ return fmt.Errorf("component %q's target Service did not become ready: %w", engine.componentLabel(vmv1beta1.ClusterComponentInsert), err)
+ }
</file context>
| maps.Equal(existing.Spec.ExtraArgs, want.Spec.ExtraArgs) && | ||
| existing.Spec.RemoteWriteSettings != nil && existing.Spec.RemoteWriteSettings.UseMultiTenantMode && | ||
| existing.Spec.StatefulMode && existing.Spec.StatefulStorage != nil && existing.Spec.StatefulStorage.EmptyDir == nil && | ||
| existing.Spec.StatefulStorage.VolumeClaimTemplate.Spec.Resources.Requests.Storage().Cmp(*want.Spec.StatefulStorage.VolumeClaimTemplate.Spec.Resources.Requests.Storage()) >= 0 && |
Contributor
There was a problem hiding this comment.
P2: When an existing owned buffer agent has no resources.requests.storage, AgentMatches dereferences a nil Storage() result and crashes the migration. Check both storage quantities for nil before comparing them so an invalid existing agent returns the normal mismatch error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/vm/cluster.go, line 73:
<comment>When an existing owned buffer agent has no `resources.requests.storage`, `AgentMatches` dereferences a nil `Storage()` result and crashes the migration. Check both storage quantities for nil before comparing them so an invalid existing agent returns the normal mismatch error.</comment>
<file context>
@@ -0,0 +1,98 @@
+ maps.Equal(existing.Spec.ExtraArgs, want.Spec.ExtraArgs) &&
+ existing.Spec.RemoteWriteSettings != nil && existing.Spec.RemoteWriteSettings.UseMultiTenantMode &&
+ existing.Spec.StatefulMode && existing.Spec.StatefulStorage != nil && existing.Spec.StatefulStorage.EmptyDir == nil &&
+ existing.Spec.StatefulStorage.VolumeClaimTemplate.Spec.Resources.Requests.Storage().Cmp(*want.Spec.StatefulStorage.VolumeClaimTemplate.Spec.Resources.Requests.Storage()) >= 0 &&
+ existing.Spec.IngestOnlyMode != nil && *existing.Spec.IngestOnlyMode
+ },
</file context>
Suggested change
| existing.Spec.StatefulStorage.VolumeClaimTemplate.Spec.Resources.Requests.Storage().Cmp(*want.Spec.StatefulStorage.VolumeClaimTemplate.Spec.Resources.Requests.Storage()) >= 0 && | |
| \t\t\t\texisting.Spec.StatefulStorage.VolumeClaimTemplate.Spec.Resources.Requests.Storage() != nil && | |
| \t\t\t\twant.Spec.StatefulStorage.VolumeClaimTemplate.Spec.Resources.Requests.Storage() != nil && | |
| \t\t\t\texisting.Spec.StatefulStorage.VolumeClaimTemplate.Spec.Resources.Requests.Storage().Cmp(*want.Spec.StatefulStorage.VolumeClaimTemplate.Spec.Resources.Requests.Storage()) >= 0 && |
| return fmt.Sprintf("%s://%s.%s.svc:%d", scheme, svc.Name, svc.Namespace, port), nil | ||
| } | ||
|
|
||
| func forceMergeURL(svc *corev1.Service, pathPrefix string) (string, error) { |
Contributor
There was a problem hiding this comment.
P3: forceMergeURL has no production caller, so this URL-building path is dead while force-merge uses podutil.DiscoverEndpointAddrs directly. Remove these unused helpers or use them in the production force-merge path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/urls.go, line 267:
<comment>`forceMergeURL` has no production caller, so this URL-building path is dead while force-merge uses `podutil.DiscoverEndpointAddrs` directly. Remove these unused helpers or use them in the production force-merge path.</comment>
<file context>
@@ -0,0 +1,273 @@
+ return fmt.Sprintf("%s://%s.%s.svc:%d", scheme, svc.Name, svc.Namespace, port), nil
+}
+
+func forceMergeURL(svc *corev1.Service, pathPrefix string) (string, error) {
+ base, err := serviceBaseURL(svc, "http", "http")
+ if err != nil {
</file context>
AndrewChubatiuk
force-pushed
the
converter-migrate
branch
from
August 20, 2026 13:04
bb89cb5 to
502ca2f
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.
2 issues found across 81 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
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/singlenode.go">
<violation number="1" location="internal/migrate/singlenode.go:101">
P1: When a rebound PVC is stale or recreated with this annotation, the resume path can make the target mount an unrelated PersistentVolume because it never validates the PV or claimRef. Validate the target PVC against the original source volume before adopting it, reusing the checks in `rebindPVC`.</violation>
</file>
<file name="internal/migrate/snapshot_test.go">
<violation number="1" location="internal/migrate/snapshot_test.go:29">
P2: In TestCreateAndWaitVolumeSnapshot, the spawned goroutine mutates the shared fake client with c.Update (writing status.readyToUse) while the main goroutine's waitVolumeSnapshotReady polls the same client with c.Get. controller-runtime's fake client wraps client-go's ObjectTracker, which is a plain map with no locking, so a concurrent read and write on the same object is a data race. Under `go test -race` or under load it can fail with "concurrent map read and map write" or a sporadically corrupt read, making the test flaky. Schedule the status write so it cannot race the polling reads (e.g., guard the client with a mutex, or set the snapshot ready before the concurrent wait is reached and use a single-writer_client), or drop the fixed 30ms sleep and poll deterministically on a condition instead of relying on wall-clock timing.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| return fmt.Errorf("cannot check for an already-rebound target PVC %s/%s: %w", target.GetNamespace(), targetPVCName, getErr) | ||
| case existingTargetPVC.Status.Phase != corev1.ClaimBound: | ||
| return fmt.Errorf("target PVC %s/%s exists from a previous attempt but isn't Bound (phase=%q) — inspect it manually before retrying", target.GetNamespace(), targetPVCName, existingTargetPVC.Status.Phase) | ||
| case existingTargetPVC.Annotations[reboundFromAnnotation] != opts.Namespace+"/"+opts.ReleaseName: |
Contributor
There was a problem hiding this comment.
P1: When a rebound PVC is stale or recreated with this annotation, the resume path can make the target mount an unrelated PersistentVolume because it never validates the PV or claimRef. Validate the target PVC against the original source volume before adopting it, reusing the checks in rebindPVC.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/singlenode.go, line 101:
<comment>When a rebound PVC is stale or recreated with this annotation, the resume path can make the target mount an unrelated PersistentVolume because it never validates the PV or claimRef. Validate the target PVC against the original source volume before adopting it, reusing the checks in `rebindPVC`.</comment>
<file context>
@@ -0,0 +1,375 @@
+ return fmt.Errorf("cannot check for an already-rebound target PVC %s/%s: %w", target.GetNamespace(), targetPVCName, getErr)
+ case existingTargetPVC.Status.Phase != corev1.ClaimBound:
+ return fmt.Errorf("target PVC %s/%s exists from a previous attempt but isn't Bound (phase=%q) — inspect it manually before retrying", target.GetNamespace(), targetPVCName, existingTargetPVC.Status.Phase)
+ case existingTargetPVC.Annotations[reboundFromAnnotation] != opts.Namespace+"/"+opts.ReleaseName:
+ return fmt.Errorf("no Deployment found for release %q in namespace %q, and PVC %s/%s exists but wasn't created by this tool's rebindPVC from this release's source PVC (annotation %q=%q, expected %q) — refusing to adopt what may be an unrelated, coincidentally-named PVC from another release in the same namespace; delete it manually first if that's intended, or investigate why the old Deployment is missing", opts.ReleaseName, opts.Namespace, target.GetNamespace(), targetPVCName, reboundFromAnnotation, existingTargetPVC.Annotations[reboundFromAnnotation], opts.Namespace+"/"+opts.ReleaseName)
+ }
</file context>
| return s | ||
| } | ||
|
|
||
| func TestCreateAndWaitVolumeSnapshot(t *testing.T) { |
Contributor
There was a problem hiding this comment.
P2: In TestCreateAndWaitVolumeSnapshot, the spawned goroutine mutates the shared fake client with c.Update (writing status.readyToUse) while the main goroutine's waitVolumeSnapshotReady polls the same client with c.Get. controller-runtime's fake client wraps client-go's ObjectTracker, which is a plain map with no locking, so a concurrent read and write on the same object is a data race. Under go test -race or under load it can fail with "concurrent map read and map write" or a sporadically corrupt read, making the test flaky. Schedule the status write so it cannot race the polling reads (e.g., guard the client with a mutex, or set the snapshot ready before the concurrent wait is reached and use a single-writer_client), or drop the fixed 30ms sleep and poll deterministically on a condition instead of relying on wall-clock timing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/migrate/snapshot_test.go, line 29:
<comment>In TestCreateAndWaitVolumeSnapshot, the spawned goroutine mutates the shared fake client with c.Update (writing status.readyToUse) while the main goroutine's waitVolumeSnapshotReady polls the same client with c.Get. controller-runtime's fake client wraps client-go's ObjectTracker, which is a plain map with no locking, so a concurrent read and write on the same object is a data race. Under `go test -race` or under load it can fail with "concurrent map read and map write" or a sporadically corrupt read, making the test flaky. Schedule the status write so it cannot race the polling reads (e.g., guard the client with a mutex, or set the snapshot ready before the concurrent wait is reached and use a single-writer_client), or drop the fixed 30ms sleep and poll deterministically on a condition instead of relying on wall-clock timing.</comment>
<file context>
@@ -0,0 +1,87 @@
+ return s
+}
+
+func TestCreateAndWaitVolumeSnapshot(t *testing.T) {
+ origInterval, origTimeout := SnapshotPollInterval, SnapshotTimeout
+ t.Cleanup(func() { SnapshotPollInterval, SnapshotTimeout = origInterval, origTimeout })
</file context>
AndrewChubatiuk
force-pushed
the
converter-migrate
branch
from
August 20, 2026 14:48
502ca2f to
7e4e043
Compare
AndrewChubatiuk
force-pushed
the
converter-migrate
branch
from
August 20, 2026 15:14
7e4e043 to
f1d7689
Compare
Contributor
Author
|
@cubic-ai-dev |
Contributor
@AndrewChubatiuk I can't start this review because your workspace has reached its monthly review limit. cubic has reviewed 403,827 of the 400,000 allowed lines of code this month. Reviews resume on 26 August 2026 (in 7 days). Enable flex capacity to cover overages automatically and resume reviews now. Learn how flex capacity works. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
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:
Added PR for VTAgent to support migration from charts for traces