Conversation
AndrewChubatiuk
force-pushed
the
distributed-updates
branch
from
cffa24d to
4553d0e
Compare
Contributor
Author
|
@cubic-dev-ai review this PR |
Contributor
@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 37 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="api/operator/v1alpha1/vmdistributed_types.go">
<violation number="1" location="api/operator/v1alpha1/vmdistributed_types.go:300">
P2: Incorrect documentation comment: This comment says "return cluster name for zone" but the method returns the VMAgent name, not a cluster name. This appears to be copied from `VMClusterName`'s comment.
(Based on your team's feedback about documenting exported structs and public methods.) [FEEDBACK_USED]</violation>
<violation number="2" location="api/operator/v1alpha1/vmdistributed_types.go:394">
P2: Inconsistent error message: The message says "zone names should be unique or unset" but zone names are now required (per the check at line 393). The "or unset" phrase is incorrect.</violation>
</file>
<file name="docs/api.md">
<violation number="1" location="docs/api.md:1352">
P3: ReadyTimeout is described as a deadline even though the field was renamed to a timeout. Update the description to reflect it’s a relative timeout/duration.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmcluster.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmcluster.go:81">
P1: Bug: When a new VMCluster is created (NotFound case), `waitForStatus` is called with `&prevVMCluster` which is an empty/zero-value struct. Should use `cluster` instead to wait for the newly created resource.</violation>
<violation number="2" location="internal/controller/operator/factory/vmdistributed/vmcluster.go:86">
P1: Bug: `wg.Wait()` is inside the for loop, which causes sequential execution instead of parallel. Move `wg.Wait()` after the loop to allow goroutines to run concurrently.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmagent.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmagent.go:52">
P2: IPv6 endpoint addresses will produce malformed URLs. When the address contains `:` (IPv6), it must be wrapped in brackets to form a valid URL host (e.g., `[2001:db8::1]:8080` instead of `2001:db8::1:8080`). Consider checking if the address is IPv6 and wrapping it appropriately.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
AndrewChubatiuk
force-pushed
the
distributed-updates
branch
from
4553d0e to
adba036
Compare
Contributor
Author
|
@cubic-dev-ai review this PR |
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.
8 issues found across 40 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="docs/resources/vmdistributed.md">
<violation number="1" location="docs/resources/vmdistributed.md:59">
P3: Fix the typo "ommited" → "omitted" in the example comment for clarity.</violation>
<violation number="2" location="docs/resources/vmdistributed.md:129">
P2: Update the examples to use `vmcluster.name` instead of the removed `vmcluster.ref`; the current examples still document a field that no longer exists in the API.</violation>
</file>
<file name=".github/workflows/main.yaml">
<violation number="1" location=".github/workflows/main.yaml:88">
P2: Piping a remote install script to bash without pinning or verification introduces a supply‑chain risk (the script can change on `main`). Pin to a specific release/commit and verify its checksum before executing.</violation>
</file>
<file name="api/operator/v1alpha1/vmdistributed_types.go">
<violation number="1" location="api/operator/v1alpha1/vmdistributed_types.go:300">
P2: Godoc comment is incorrect - this method returns the VMAgent name, not "cluster name for zone". This appears to be a copy-paste error from `VMClusterName()`.
(Based on your team's feedback about documenting exported structs and public methods.) [FEEDBACK_USED]</violation>
<violation number="2" location="api/operator/v1alpha1/vmdistributed_types.go:394">
P3: Error message says "unique or unset" but zone names are now required (per the PR description and the `json:"name"` tag change). The "or unset" part is misleading.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmcluster.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmcluster.go:81">
P0: When a new VMCluster is created, `prevVMCluster` remains empty (Name="", Namespace="") but is passed to `waitForStatus`. This will cause the wait to poll for a non-existent object with empty name. Use `cluster` instead.</violation>
<violation number="2" location="internal/controller/operator/factory/vmdistributed/vmcluster.go:86">
P1: The `wg.Wait()` is inside the `for` loop, causing goroutines to run sequentially instead of in parallel as the function's comment suggests. Move `wg.Wait()` outside the loop to achieve actual parallel execution.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmdistributed.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmdistributed.go:46">
P1: Error from `syncVMClusters` is discarded (not wrapped with `%w`). Also contains typo 'VMDistribured' → 'VMDistributed'. Losing the original error makes debugging failed reconciliations very difficult.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
8 issues found across 41 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="api/operator/v1alpha1/vmdistributed_types.go">
<violation number="1" location="api/operator/v1alpha1/vmdistributed_types.go:391">
P3: The error message redundantly includes `=%s` for an empty value. Since we're checking `len(zone.Name) == 0`, we know the name is empty, making `=%s` confusing (will render as `name= is required`). Remove the redundant placeholder for a clearer error message.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmauth.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmauth.go:51">
P1: Missing deep copy of spec may cause mutation of the original CR. If `cr.Spec.VMAuth.Spec.UnauthorizedUserAccessSpec` is not nil, setting `TargetRefs` mutates the original CR's spec. Consider using `cr.Spec.VMAuth.Spec.DeepCopy()` or JSON marshal/unmarshal pattern used in `vmagent.go`.</violation>
<violation number="2" location="internal/controller/operator/factory/vmdistributed/vmauth.go:78">
P2: Typo in logging key: "nams" should be "name".</violation>
</file>
<file name=".github/workflows/main.yaml">
<violation number="1" location=".github/workflows/main.yaml:88">
P1: Avoid executing unpinned remote install scripts in CI. Pin mirrord to a specific version (or commit) and verify its checksum/signature before running the installer to reduce supply-chain risk.</violation>
</file>
<file name="docs/resources/vmdistributed.md">
<violation number="1" location="docs/resources/vmdistributed.md:58">
P2: The docs now define `vmcluster` using `name`/`spec`, but the examples still use `vmcluster.ref.name`. Update the examples to use `vmcluster.name` so they match the documented schema and are copy‑pasteable.</violation>
<violation number="2" location="docs/resources/vmdistributed.md:59">
P3: Fix the typo in the inline comment (“ommited” → “omitted”) so the example reads correctly.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmcluster.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmcluster.go:74">
P2: Newly created VMClusters have stale status in the slice after `waitForStatus` completes. The status is updated in `prevVMCluster` but not copied back to `cluster` (unlike the existing cluster case which has `cluster.Status = prevVMCluster.Status`). This causes the final sorting to use ObservedGeneration=0 for new clusters while existing ones have their actual values, potentially producing incorrect ordering.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmdistributed.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmdistributed.go:46">
P1: Error from `syncVMClusters` is not included in the returned error (missing `%w` and `err`), which will make debugging failures difficult. Also contains typo "VMDistribured" → "VMDistributed".</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
AndrewChubatiuk
force-pushed
the
distributed-updates
branch
3 times, most recently
from
79c7353 to
039ed1f
Compare
Contributor
Author
|
@cubic-dev-ai review this PR |
Contributor
@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete. |
AndrewChubatiuk
force-pushed
the
distributed-updates
branch
from
039ed1f to
3f65dd2
Compare
AndrewChubatiuk
force-pushed
the
distributed-updates
branch
from
3f65dd2 to
aa5f82f
Compare
Contributor
Author
|
@cubic-dev-ai review this PR |
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.
7 issues found across 48 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="internal/controller/operator/factory/vmdistributed/vmdistributed.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmdistributed.go:108">
P1: Potential nil pointer dereference: `cr.Spec.ZoneUpdatePause` is an optional pointer field (`*metav1.Duration`) but is accessed directly without nil check. This will panic if `ZoneUpdatePause` is not set in the spec.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmagent.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmagent.go:182">
P0: Nil pointer dereference: `FlushTimeout` is an optional pointer (`*metav1.Duration`) but is accessed directly without a nil check. This will panic if the user doesn't set `flushTimeout` in their spec. Add a nil check and use a sensible default.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmagent_test.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmagent_test.go:48">
P2: Parsing tsURL.Host with strings.Index breaks on IPv6 addresses (colon inside host), which can make this test fail on IPv6 environments. Use net.SplitHostPort for host/port parsing.</violation>
</file>
<file name="internal/controller/operator/vmdistributed_controller_test.go">
<violation number="1" location="internal/controller/operator/vmdistributed_controller_test.go:55">
P2: Zone name is required; the test VMDistributed spec omits it, which will fail validation/creation once the CRD enforces required `spec.zones[*].name`.</violation>
</file>
<file name="docs/resources/vmdistributed.md">
<violation number="1" location="docs/resources/vmdistributed.md:155">
P3: The text above the example still says to add zones "as refs", but the new example uses `vmcluster.name`. Update the wording to match the name-based reference model so users aren’t told to use refs that no longer exist.</violation>
</file>
<file name="docs/api.md">
<violation number="1" location="docs/api.md:1353">
P2: `spec.retain` is documented as required, but the API schema marks it optional (`json:"retain,omitempty"`). This misleads users about which fields they must set. Update the docs to mark it as optional.</violation>
</file>
<file name="api/operator/v1alpha1/vmdistributed_types.go">
<violation number="1" location="api/operator/v1alpha1/vmdistributed_types.go:397">
P1: Validation doesn't check computed cluster names for collisions. The check only validates explicitly set `vmcluster.name` values, but names can also be derived from `CommonZone.VMCluster.Name` (with `%ZONE%` replacement) or generated as `${cr.Name}-${zone.Name}`. This could allow name collisions when an explicit name matches a generated name (e.g., zone1 sets `vmcluster.name="myapp-zone2"` while zone2 named "zone2" generates the same). Consider using `zone.VMClusterName(cr)` instead of `zone.VMCluster.Name` for the uniqueness check.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
AndrewChubatiuk
force-pushed
the
distributed-updates
branch
2 times, most recently
from
1bb6563 to
dc94a23
Compare
Contributor
Author
|
@cubic-dev-ai review this PR |
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.
7 issues found across 50 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="docs/CHANGELOG.md">
<violation number="1" location="docs/CHANGELOG.md:18">
P2: Rule violated: **Changelog Review Agent**
Changelog entries must include a user‑centric before/after explanation and user‑visible improvement (rule: Required structure §3). The added BUGFIX items are implementation-focused and omit the before/after impact, so they do not meet the mandatory structure.</violation>
</file>
<file name="api/operator/v1alpha1/vmdistributed_types.go">
<violation number="1" location="api/operator/v1alpha1/vmdistributed_types.go:97">
P2: Outdated comment: this struct no longer supports references since the `ref` field was removed. Update the comment to reflect that this is now only a specification for a VMCluster.</violation>
</file>
<file name="internal/controller/operator/factory/reconcile/statefulset_pvc_expand.go">
<violation number="1" location="internal/controller/operator/factory/reconcile/statefulset_pvc_expand.go:121">
P3: Misleading comment: This comment says "check if pvc need to expand" but the function `isStorageClassExpandable` checks whether the storage class allows expansion, not whether the PVC needs to expand. Consider updating to match the actual operation being performed.</violation>
</file>
<file name="test/e2e/vmcluster_test.go">
<violation number="1" location="test/e2e/vmcluster_test.go:1528">
P3: Assertion message still says "at least 4 pods" even though the check now allows >= 3. Update the message to match the new threshold to avoid misleading failures.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmdistributed.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmdistributed.go:46">
P3: Typo in error message: "VMDistribured" should be "VMDistributed".</violation>
<violation number="2" location="internal/controller/operator/factory/vmdistributed/vmdistributed.go:86">
P2: Logic bug: Cluster is excluded from VMAuth even when only the owner reference was modified (no spec changes). The comment says "Drain cluster reads only if the spec has been modified" but the exclusion logic runs unconditionally after the continue check. Consider wrapping the drain/exclude logic in `if len(diff) > 0 { ... }`.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/util.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/util.go:267">
P2: When the object is not found, this silently continues polling and eventually times out with a confusing error message (`current status: unknown`). Given the call sites expect objects to already exist, consider returning an error on NotFound to surface the actual problem.
(Based on your team's feedback about adding comments when logic is non-obvious or surprising.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
AndrewChubatiuk
force-pushed
the
distributed-updates
branch
from
dc94a23 to
4cf7e7e
Compare
Contributor
Author
|
@cubic-dev-ai review this PR |
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.
5 issues found across 50 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="docs/CHANGELOG.md">
<violation number="1" location="docs/CHANGELOG.md:18">
P3: Add a space after the inline code block so the sentence renders correctly in markdown.</violation>
<violation number="2" location="docs/CHANGELOG.md:20">
P1: Rule violated: **Changelog Review Agent**
Changelog entry describes an internal implementation detail (adding a mutex) and omits the required user‑centric before/after explanation, violating the mandated changelog structure.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmdistributed.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmdistributed.go:98">
P1: The update operation uses the wrong object. `rclient.Update(ctx, vmCluster)` should be `rclient.Update(ctx, &prevVMCluster)` since `prevVMCluster` is the object fetched from the API with the correct `resourceVersion` and the owner reference changes.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/vmdistributed_test.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmdistributed_test.go:212">
P2: Test doesn't verify the claimed behavior. The test is titled 'should create VMAuth with default name' but doesn't assert that the default name was actually used. Consider adding an assertion to verify the created VMAuth's name matches the expected default (`d.cr.Name`).</violation>
</file>
<file name="docs/api.md">
<violation number="1" location="docs/api.md:1268">
P3: The FlushTimeout description still says "deadline" even though the field is now timeout-based. Update the wording to reflect a timeout to avoid confusing users about absolute vs relative time semantics.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic-dev-ai review this PR |
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 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 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/controller/operator/factory/vmdistributed/zone.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/zone.go:144">
P2: Variable shadowing bug: the inner loop variable `i` (iterating `zs.vmclusters`) shadows the outer loop variable `i` (iterating `cr.Spec.Zones`). Error messages here format `spec.zones[%d]` using the vmcluster index instead of the zone index, which will produce incorrect/misleading error output.</violation>
</file>
<file name="test/e2e/suite/suite.go">
<violation number="1" location="test/e2e/suite/suite.go:188">
P2: Stop the envtest environment after canceling the manager (and after it exits). Stopping the API server first can make RunManager return an error and fail the expectation in the manager goroutine.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic-dev-ai review this PR |
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.
4 issues found across 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 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/controller/operator/factory/vmdistributed/zone.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/zone.go:107">
P2: VMAgent is named using `z.VMClusterName(cr)` (which yields `${cr.Name}-${zone.name}`), but the PR description states VMAgent defaults to `${cr.Name}`. If each zone has its own VMAgent and needs a unique name, this is correct but the naming method is misleading. If VMAgent should truly default to `${cr.Name}`, this is a bug — consider using a dedicated `VMAgentName` method.</violation>
<violation number="2" location="internal/controller/operator/factory/vmdistributed/zone.go:310">
P2: `pollMetrics` returns on the first metric matching the cluster URL hash, but map iteration is unordered. If multiple metrics match the same hash, a zero-valued one could be checked first, causing a premature "drained" result while non-zero entries are skipped. Consider checking all matching metrics before declaring the queue empty.</violation>
<violation number="3" location="internal/controller/operator/factory/vmdistributed/zone.go:430">
P2: `done()` returns `true` when the map is empty, which can cause `waitForEmptyPQ` to return success prematurely if all endpoints disappear (e.g., during pod restarts) before any poller finishes. Consider distinguishing between "all pollers completed successfully" and "no pollers exist" — e.g., track total registered count separately.</violation>
</file>
<file name="api/operator/v1alpha1/vmdistributed_types.go">
<violation number="1" location="api/operator/v1alpha1/vmdistributed_types.go:67">
P2: Use pointers for optional nested objects (`VMCluster`, `VMAgent`, `RemoteWrite`, `Spec`) instead of structs.
Using structs prevents distinguishing between "omitted" (inherit from `ZoneCommon` or do nothing) and "present but empty" (use defaults). It also causes JSON bloat (e.g., `vmcluster: {spec: {}}`) because nested struct fields are always serialized. Switching to pointers ensures proper Kubernetes CRD conventions for optional fields and enables correct inheritance logic.
Note: Helper methods like `VMClusterName` and `Validate` will need nil checks when these become pointers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic-dev-ai review this PR |
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.
4 issues found across 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 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/controller/operator/factory/vmdistributed/vmauth.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmauth.go:20">
P2: Skipping VMClusters/VMAgents with zero CreationTimestamp can leave VMAuth with an empty UnauthorizedUserAccessSpec, which violates VMAuth validation and can break initial reconcile. Avoid filtering out not-yet-created resources or guard against empty targetRefs before overwriting URLMap/URLPrefix.</violation>
</file>
<file name="docs/resources/vmdistributed.md">
<violation number="1" location="docs/resources/vmdistributed.md:64">
P2: This example is not runnable as-is: VMDistributed validation requires vmcluster.spec.vminsert and vmcluster.spec.vmselect to be set either in zoneCommon or per-zone, but the example only lists vmcluster names. Please add the minimal zoneCommon.vmcluster.spec (or per-zone vmcluster.spec) to satisfy validation.
(Based on your team's feedback about documentation examples being complete and runnable.) [FEEDBACK_USED]</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/zone.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/zone.go:165">
P2: `sort.Slice` is not stable — new remote write entries not found in `orderMap` are treated as equal but may be reordered non-deterministically across reconciliations, potentially causing unnecessary spec updates. Use `sort.SliceStable` to preserve original ordering for equal elements.</violation>
<violation number="2" location="internal/controller/operator/factory/vmdistributed/zone.go:356">
P1: Race condition: after `m.delete(addr)` removes an endpoint, the polling goroutine's deferred `m.add(addr, nil)` unconditionally re-inserts it as a ghost entry. This can cause `m.done()` to return `true` prematurely (if no other active entries remain) or leave stale entries in the manager.
Guard the completion marker so it only applies to entries that still exist.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic-dev-ai review this PR |
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 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 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/controller/operator/factory/vmdistributed/vmauth.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/vmauth.go:20">
P2: Skipping VMCluster/VMAgent refs when CreationTimestamp is zero can leave VMAuth with an empty unauthorized_user config (no url_map/url_prefix/targetRefs), which fails validation for new VMDistributed resources. This blocks initial reconcile for fresh zones before the next resync populates timestamps.</violation>
</file>
<file name="docs/resources/vmdistributed.md">
<violation number="1" location="docs/resources/vmdistributed.md:60">
P2: The example VMDistributed manifest isn’t runnable as shown because `spec.vmauth` is required and a name-only entry implies an existing VMAuth, but the example doesn’t define one. Add an inline `vmauth.spec` (or include a VMAuth manifest) so users can apply the example as-is.
(Based on your team's feedback about keeping documentation examples runnable and fully defined.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic-dev-ai review this PR |
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.
3 issues found across 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 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="docs/resources/vmdistributed.md">
<violation number="1" location="docs/resources/vmdistributed.md:10">
P2: Keep the old /operator/resources/VMDistributed/ alias alongside the new lowercase alias to avoid breaking existing links.</violation>
</file>
<file name="test/e2e/vmdistributed_test.go">
<violation number="1" location="test/e2e/vmdistributed_test.go:52">
P2: The goroutine in createVMClusters captures the loop variable `i`, so parallel runs can index the wrong cluster (or panic) when `i` changes before the goroutine executes. Capture the element outside the goroutine before calling wg.Go.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/zone.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/zone.go:359">
P1: Premature completion: `gcancel()` is called when a single vmagent's persistent queue is drained, but this cancels polling for all other vmagents. The function will return success even if only one out of N vmagents has been verified as drained. This undermines the data safety guarantee during rolling upgrades.
Each vmagent goroutine creates its own `manager`, and when its `m.done()` returns true, the shared `gcancel()` stops all other agents. The `context.Canceled` errors from other agents are silently ignored (line 352), so no error is raised.
Consider tracking per-agent completion (e.g., via an `atomic.Int32` counter initialized to `len(zs.vmagents)`) and only calling `gcancel()` when all agents have reported done.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic-dev-ai review this PR |
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.
1 issue found across 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 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/controller/operator/factory/vmdistributed/zone.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/zone.go:342">
P1: `wait.UntilWithContext` uses `gctx` but should use `actx`. When all polls for a vmagent complete, `acancel()` is called to signal completion, but the discovery loop never observes this cancellation because it's bound to `gctx`. This causes `waitForEmptyPQ` to always block until the full parent timeout, even when all persistent queues have been drained.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic-dev-ai review this PR |
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 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 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="Makefile">
<violation number="1" location="Makefile:165">
P2: `-linkmode=external` requires cgo; this command no longer forces CGO_ENABLED=1, so e2e tests will fail in environments where cgo is disabled. Re-add CGO_ENABLED=1 to keep external linking working reliably.</violation>
</file>
<file name="config/examples/vmdistributed-migrate-from-chart.yaml">
<violation number="1" location="config/examples/vmdistributed-migrate-from-chart.yaml:17">
P2: Replication factor 2 requires at least two vmstorage replicas; this example configures only one vmstorage replica per zone, so the sample VMCluster won’t be able to satisfy the replication requirement.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
@cubic-dev-ai review this PR |
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.
1 issue found across 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 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="docs/resources/vmdistributed.md">
<violation number="1" location="docs/resources/vmdistributed.md:47">
P3: Clarify that `vmcluster.name` is optional and defaults to `${cr.Name}-${zone.name}` (or `zoneCommon.vmcluster.name` with `%ZONE%`) when omitted; the current text reads like a required field and conflicts with the API’s defaulting behavior.
(Based on your team's feedback about clarifying behavior and roles in documentation text.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
vrutkovs
requested changes
Feb 16, 2026
| OwnerReferences: []metav1.OwnerReference{cr.AsOwner()}, | ||
| } | ||
| } | ||
| vmAgentCustomSpec, err := mergeSpecs(&cr.Spec.ZoneCommon.VMAgent.Spec, &z.VMAgent.Spec, z.Name) |
Collaborator
There was a problem hiding this comment.
This also renders placeholders which may have unintended effects. Why not build.MergeDeep here?
Contributor
Author
There was a problem hiding this comment.
placeholders are applied to common spec only, which is intended
then it's merged with per-zone spec
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 76 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 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=".github/workflows/main.yaml">
<violation number="1" location=".github/workflows/main.yaml:89">
P2: Checksum verification is ineffective when the checksum file is downloaded from the same source as the binary. Pin the expected SHA256 (or verify a signed release) to avoid executing a potentially compromised artifact in CI.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/zone.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/zone.go:399">
P1: Bug: `stop()` unconditionally sets `m.cancels[id] = nil` even if the entry was already removed by `delete()`, re-inserting a ghost entry. This can cause `cancelIfNeeded` to fire prematurely when an endpoint disappears and its pollMetrics goroutine subsequently calls `stop`. The discovery loop would terminate before replacement endpoints can be discovered.
The fix is to return early from `stop` if the entry no longer exists in the map.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if ok && cancel != nil { | ||
| cancel() | ||
| } | ||
| m.cancels[id] = nil |
Contributor
There was a problem hiding this comment.
P1: Bug: stop() unconditionally sets m.cancels[id] = nil even if the entry was already removed by delete(), re-inserting a ghost entry. This can cause cancelIfNeeded to fire prematurely when an endpoint disappears and its pollMetrics goroutine subsequently calls stop. The discovery loop would terminate before replacement endpoints can be discovered.
The fix is to return early from stop if the entry no longer exists in the map.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/operator/factory/vmdistributed/zone.go, line 399:
<comment>Bug: `stop()` unconditionally sets `m.cancels[id] = nil` even if the entry was already removed by `delete()`, re-inserting a ghost entry. This can cause `cancelIfNeeded` to fire prematurely when an endpoint disappears and its pollMetrics goroutine subsequently calls `stop`. The discovery loop would terminate before replacement endpoints can be discovered.
The fix is to return early from `stop` if the entry no longer exists in the map.</comment>
<file context>
@@ -0,0 +1,442 @@
+ if ok && cancel != nil {
+ cancel()
+ }
+ m.cancels[id] = nil
+ m.cancelIfNeeded()
+}
</file context>
vrutkovs
reviewed
Feb 16, 2026
Comment on lines
+45
to
+58
| statusI := zs.vmclusters[i].Status.UpdateStatus | ||
| statusJ := zs.vmclusters[j].Status.UpdateStatus | ||
| if statusI != statusJ { | ||
| return statusI == vmv1beta1.UpdateStatusFailed | ||
| } | ||
| if zs.vmclusters[i].CreationTimestamp.IsZero() && !zs.vmclusters[j].CreationTimestamp.IsZero() { | ||
| return true | ||
| } | ||
| if zs.vmclusters[j].CreationTimestamp.IsZero() && !zs.vmclusters[i].CreationTimestamp.IsZero() { | ||
| return false | ||
| } | ||
| if zs.vmclusters[i].Status.ObservedGeneration != zs.vmclusters[j].Status.ObservedGeneration { | ||
| return zs.vmclusters[i].Status.ObservedGeneration > zs.vmclusters[j].Status.ObservedGeneration | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggested change
| statusI := zs.vmclusters[i].Status.UpdateStatus | |
| statusJ := zs.vmclusters[j].Status.UpdateStatus | |
| if statusI != statusJ { | |
| return statusI == vmv1beta1.UpdateStatusFailed | |
| } | |
| if zs.vmclusters[i].CreationTimestamp.IsZero() && !zs.vmclusters[j].CreationTimestamp.IsZero() { | |
| return true | |
| } | |
| if zs.vmclusters[j].CreationTimestamp.IsZero() && !zs.vmclusters[i].CreationTimestamp.IsZero() { | |
| return false | |
| } | |
| if zs.vmclusters[i].Status.ObservedGeneration != zs.vmclusters[j].Status.ObservedGeneration { | |
| return zs.vmclusters[i].Status.ObservedGeneration > zs.vmclusters[j].Status.ObservedGeneration | |
| } | |
| statusI := zs.vmclusters[i].Status | |
| statusJ := zs.vmclusters[j].Status | |
| if statusI.UpdateStatus != statusJ.UpdateStatus { | |
| return statusI.UpdateStatus == vmv1beta1.UpdateStatusFailed | |
| } | |
| isZeroI := zs.vmclusters[i].CreationTimestamp.IsZero() | |
| isZeroJ := zs.vmclusters[j].CreationTimestamp.IsZero() | |
| if isZeroI != isZeroJ { | |
| return isZeroI | |
| } | |
| if statusI.ObservedGeneration != statusJ.ObservedGeneration { | |
| return statusI.ObservedGeneration > statusJ.ObservedGeneration | |
| } |
A bit less duplication
vrutkovs
approved these changes
Feb 16, 2026
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
2 participants
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.
follow-up for ad6c5e8
spec.zones[*].vmcluster.ref,nowspec.zones[*].vmcluster.namewill be used for both getting existing objects and creating new onesspec.retainto keep managed objects in case of VMDistributed removalspec.readyTimeouttospec.zoneCommon.readyTimeoutand it defines timeout for a whole zone upgradespec.zoneUpdatePausetospec.zoneCommon.updatePausespec.vmagenttospec.zones[*].vmagentspec.flushDeadlineit's replaced by a singlespec.zoneCommon.readyTimeoutfor a whole zone updatespec.vmagent.labelSelectorspec.zones[*].nameparameter required and madespec.zones[*].vmcluster.name,spec.zones[*].vmagent.nameandspec.vmauth.nameoptional. when skipped vmauth are named after${cr.Name}and vmcluster, vmagent -${cr.Name}-${zone.name}/insert/.+path for write operations load balancingSummary by cubic
Rebuilt VMDistributed for status-driven, zone-by-zone rollouts with spec-only reconciliation and deep-mergeable per-zone overrides. Added a zone orchestrator with per-kind reconcilers (VMCluster, VMAgent, VMAuth), required zone names and %ZONE% placeholders, stricter defaulting/validation, refreshed CRDs/docs/examples (including chart migration), and CI mirrord with updated ginkgo/gomega.
Bug Fixes
Migration
Written for commit ac9309a. Summary will update on new commits.