VLDistributed implementation#2413
Conversation
| ) | ||
|
|
||
| func fetchMetricValues(ctx context.Context, httpClient *http.Client, url, metricName, dimension string) (map[string]float64, error) { |
There was a problem hiding this comment.
in this PR moved it to internal/podutil, going to use it later for migration tool as well
There was a problem hiding this comment.
same is about endpoint slices discovery
There was a problem hiding this comment.
moved to internal/podutil
81c7b97 to
6683dd1
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/controller/operator/factory/vldistributed/zone.go">
<violation number="1" location="internal/controller/operator/factory/vldistributed/zone.go:158">
P1: Queue draining always succeeds without checking the target queue because vlagent masks `url` labels by default, so traffic can return to a zone while its persistent queue still has data. Enable `ShowURL` on controller-managed VLAgents before using that label to gate LB restoration.</violation>
</file>
<file name="test/e2e/vldistributed_test.go">
<violation number="1" location="test/e2e/vldistributed_test.go:107">
P3: This test file structurally duplicates most of `vmdistributed_test.go` (helper functions, test contexts, assertion patterns). Consider extracting shared helpers into a common test utility package to reduce maintenance burden as the PR description itself acknowledges.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| if err != nil { | ||
| return nil, fmt.Errorf("spec.zones[%d].vlagent.spec: %w", i, err) | ||
| } | ||
| if vlAgentSpec.RemoteWriteSettings == nil { |
There was a problem hiding this comment.
P1: Queue draining always succeeds without checking the target queue because vlagent masks url labels by default, so traffic can return to a zone while its persistent queue still has data. Enable ShowURL on controller-managed VLAgents before using that label to gate LB restoration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/operator/factory/vldistributed/zone.go, line 158:
<comment>Queue draining always succeeds without checking the target queue because vlagent masks `url` labels by default, so traffic can return to a zone while its persistent queue still has data. Enable `ShowURL` on controller-managed VLAgents before using that label to gate LB restoration.</comment>
<file context>
@@ -0,0 +1,446 @@
+ if err != nil {
+ return nil, fmt.Errorf("spec.zones[%d].vlagent.spec: %w", i, err)
+ }
+ if vlAgentSpec.RemoteWriteSettings == nil {
+ vlAgentSpec.RemoteWriteSettings = &vmv1.VLAgentRemoteWriteSettings{}
+ }
</file context>
| @@ -0,0 +1,1255 @@ | |||
| package e2e | |||
There was a problem hiding this comment.
P3: This test file structurally duplicates most of vmdistributed_test.go (helper functions, test contexts, assertion patterns). Consider extracting shared helpers into a common test utility package to reduce maintenance burden as the PR description itself acknowledges.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/e2e/vldistributed_test.go, line 107:
<comment>This test file structurally duplicates most of `vmdistributed_test.go` (helper functions, test contexts, assertion patterns). Consider extracting shared helpers into a common test utility package to reduce maintenance burden as the PR description itself acknowledges.</comment>
<file context>
@@ -0,0 +1,1255 @@
+ }
+}
+
+//nolint:dupl,lll
+var _ = Describe("e2e VLDistributed", Label("vl", "vldistributed"), func() {
+ ctx := context.Background()
</file context>
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
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/controller/operator/factory/finalize/vldistributed_test.go">
<violation number="1" location="internal/controller/operator/factory/finalize/vldistributed_test.go:57">
P3: The base `cr` fixture is mutated by `removeFinalizers` during the first test case, and `crRetain`/`crChanged` inherit the mutated state (nil finalizers) via `DeepCopy()`. The retain and backend-type-change test cases never exercise the finalizer removal path because their CRs start without finalizers. Create a fresh CR per test case instead of reusing a mutated base, or reset `Finalizers` after the first call.</violation>
</file>
<file name="internal/controller/operator/factory/vmdistributed/zone.go">
<violation number="1" location="internal/controller/operator/factory/vmdistributed/zone.go:390">
P1: vldistributed `waitForEmptyPQ` still uses the old URL label matching and was not updated alongside vmdistributed. The `waitForEmptyPQ` in `vldistributed/zone.go` still calls `FetchMetricValues` with `"url"` as the dimension and does a direct string comparison (`u != backendURL`), while the vmdistributed counterpart was updated to use `"path"` with xxhash-based matching. Once VMAgent/VLAgent switches to emitting path-based metrics (containing URL hashes instead of raw URLs), the vldistributed path will fail to match backends and persistent queue drain detection will break silently — it will either never drain (always treating backends as non-empty) or prematurely drain (matching wrong backends). Apply the same hash-based matching pattern to `vldistributed/zone.go` `waitForEmptyPQ`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| // Query each discovered ip. If any returns non-zero metric, continue polling. | ||
| var metricValues map[string]float64 | ||
| if metricValues, err = fetchMetricValues(ctx, zs.httpClient, addr, vmAgentQueueMetricName, "path"); err != nil { | ||
| if metricValues, err = podutil.FetchMetricValues(ctx, zs.httpClient, addr, vmAgentQueueMetricName, "path"); err != nil { |
There was a problem hiding this comment.
P1: vldistributed waitForEmptyPQ still uses the old URL label matching and was not updated alongside vmdistributed. The waitForEmptyPQ in vldistributed/zone.go still calls FetchMetricValues with "url" as the dimension and does a direct string comparison (u != backendURL), while the vmdistributed counterpart was updated to use "path" with xxhash-based matching. Once VMAgent/VLAgent switches to emitting path-based metrics (containing URL hashes instead of raw URLs), the vldistributed path will fail to match backends and persistent queue drain detection will break silently — it will either never drain (always treating backends as non-empty) or prematurely drain (matching wrong backends). Apply the same hash-based matching pattern to vldistributed/zone.go waitForEmptyPQ.
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 390:
<comment>vldistributed `waitForEmptyPQ` still uses the old URL label matching and was not updated alongside vmdistributed. The `waitForEmptyPQ` in `vldistributed/zone.go` still calls `FetchMetricValues` with `"url"` as the dimension and does a direct string comparison (`u != backendURL`), while the vmdistributed counterpart was updated to use `"path"` with xxhash-based matching. Once VMAgent/VLAgent switches to emitting path-based metrics (containing URL hashes instead of raw URLs), the vldistributed path will fail to match backends and persistent queue drain detection will break silently — it will either never drain (always treating backends as non-empty) or prematurely drain (matching wrong backends). Apply the same hash-based matching pattern to `vldistributed/zone.go` `waitForEmptyPQ`.</comment>
<file context>
@@ -385,13 +387,14 @@ func (zs *zones) waitForEmptyPQ(ctx context.Context, rclient client.Client, inte
// Query each discovered ip. If any returns non-zero metric, continue polling.
var metricValues map[string]float64
- if metricValues, err = podutil.FetchMetricValues(ctx, zs.httpClient, addr, vmAgentQueueMetricName, "url"); err != nil {
+ if metricValues, err = podutil.FetchMetricValues(ctx, zs.httpClient, addr, vmAgentQueueMetricName, "path"); err != nil {
logger.WithContext(ctx).Error(err, "attempt to get metrics failed", "url", addr, "name", nsn.String())
// Treat fetch errors as transient -> not ready, continue polling.
</file context>
| }, | ||
| } | ||
|
|
||
| f(opts{ |
There was a problem hiding this comment.
P3: The base cr fixture is mutated by removeFinalizers during the first test case, and crRetain/crChanged inherit the mutated state (nil finalizers) via DeepCopy(). The retain and backend-type-change test cases never exercise the finalizer removal path because their CRs start without finalizers. Create a fresh CR per test case instead of reusing a mutated base, or reset Finalizers after the first call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/operator/factory/finalize/vldistributed_test.go, line 57:
<comment>The base `cr` fixture is mutated by `removeFinalizers` during the first test case, and `crRetain`/`crChanged` inherit the mutated state (nil finalizers) via `DeepCopy()`. The retain and backend-type-change test cases never exercise the finalizer removal path because their CRs start without finalizers. Create a fresh CR per test case instead of reusing a mutated base, or reset `Finalizers` after the first call.</comment>
<file context>
@@ -0,0 +1,181 @@
+ },
+ }
+
+ f(opts{
+ cr: cr,
+ predefinedObjects: []runtime.Object{cr},
</file context>
ea73c89 to
f0fe371
Compare
…stead of URL, as it can be hidden
2 participants
Similar to
VMDistributedthis implements an object to control multiple region-separated cluster.The PR abstracts metrics handling, but the main control flow is duplicated - in case some resource-specific changes would be required. Later we can abstract zone handling
Fixes #1960