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.
1 issue 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="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
| @@ -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>
da11265 to
0962b46
Compare
| } | ||
|
|
||
| func TestWaitForEmptyPQ(t *testing.T) { | ||
| // Pre-compute the expected backend URL for VMCluster named "test" in ns "default" |
There was a problem hiding this comment.
This may be useful for setups where users decided to hide the url (I think its on by default?), but I'm not sure if its a good idea - hashing format may change?
| if lastStatus != nil && !limiter.Throttle() { | ||
| logger.WithContext(ctx).V(1).Info(fmt.Sprintf("waiting for %T=%s to be ready, current status: %s", obj, nsn.String(), string(lastStatus.UpdateStatus))) | ||
| } | ||
| if lastStatus != nil && lastStatus.UpdateStatus == vmv1beta1.UpdateStatusFailed && isCluster { |
There was a problem hiding this comment.
why is this only for cluster?
There was a problem hiding this comment.
There is no failure condition for VMAuth, for instance
| @@ -0,0 +1,143 @@ | |||
| package vldistributed | |||
There was a problem hiding this comment.
looks like whole file can be moved to shared code and be used by both distributed specs
| @@ -0,0 +1,446 @@ | |||
| package vldistributed | |||
There was a problem hiding this comment.
this whole file also looks like candidate to be shared between different distributed CRs
There was a problem hiding this comment.
Agree, but I'd prefer to optimize this later - I suspect there might be VM / VL specifics here
There was a problem hiding this comment.
1 issue found across 7 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/podutil/vmauth.go">
<violation number="1" location="internal/podutil/vmauth.go:18">
P2: A recreated distributed CR can include stale objects from the prior same-named CR in its VMAuth targets, because ownership is matched without `UID`. Include `UID` so only objects owned by this CR instance are routed to.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| func HasOwnerReference(owners []metav1.OwnerReference, owner *metav1.OwnerReference) bool { | ||
| for i := range owners { | ||
| o := &owners[i] | ||
| if o.APIVersion == owner.APIVersion && o.Kind == owner.Kind && o.Name == owner.Name { |
There was a problem hiding this comment.
P2: A recreated distributed CR can include stale objects from the prior same-named CR in its VMAuth targets, because ownership is matched without UID. Include UID so only objects owned by this CR instance are routed to.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/podutil/vmauth.go, line 18:
<comment>A recreated distributed CR can include stale objects from the prior same-named CR in its VMAuth targets, because ownership is matched without `UID`. Include `UID` so only objects owned by this CR instance are routed to.</comment>
<file context>
@@ -0,0 +1,72 @@
+func HasOwnerReference(owners []metav1.OwnerReference, owner *metav1.OwnerReference) bool {
+ for i := range owners {
+ o := &owners[i]
+ if o.APIVersion == owner.APIVersion && o.Kind == owner.Kind && o.Name == owner.Name {
+ return true
+ }
</file context>
| if o.APIVersion == owner.APIVersion && o.Kind == owner.Kind && o.Name == owner.Name { | |
| if o.APIVersion == owner.APIVersion && o.Kind == owner.Kind && o.Name == owner.Name && o.UID == owner.UID { |
| EnableDefaultPreStopHook bool `default:"true" env:"VM_ENABLE_DEFAULT_PRESTOP_HOOK"` | ||
| // Disables automatic -remoteWrite.showURL=true injection for VMDistributed-managed VMAgents. | ||
| // Use it during upgrades from operator versions that did not set this flag to avoid pod rollouts. | ||
| VMDistributedDisableRemoteWriteShowURL bool `default:"false" env:"VM_VMDISTRIBUTED_DISABLE_REMOTE_WRITE_SHOW_URL"` |
There was a problem hiding this comment.
I'm afraid of this param as expect requests for global defaults for other flags as well and why spec.zoneCommon doesn't work for this purpose? also why this vmagent param is specific to distributed CR? i would rather avoid adding it at all
There was a problem hiding this comment.
This is "don't configure vmagent to show URL", so it needs to be at VMDistributed level. Let me play around with this some more - I think its not needed
… it has url label
There was a problem hiding this comment.
6 issues found across 70 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/controller/operator/factory/finalize/vldistributed.go">
<violation number="1" location="internal/controller/operator/factory/finalize/vldistributed.go:37">
P1: `spec.retain: true` still deletes an existing VLSingle when its controller is disabled before VLDistributed deletion: this condition skips removing its owner reference, then removing the parent finalizer lets Kubernetes GC it. Retain cleanup should disown previously created resources regardless of current controller enablement.</violation>
</file>
<file name="internal/podutil/util.go">
<violation number="1" location="internal/podutil/util.go:187">
P3: This new generic polling implementation is unreachable, so queue-drain behavior remains duplicated and future fixes can diverge. Remove it or migrate both zone controllers to call it.</violation>
<violation number="2" location="internal/podutil/util.go:202">
P2: `WaitForEmptyPQ` calls `opts.MatchesBackend(labelVal, backendURL)` without checking whether `opts.MatchesBackend` is nil. If a caller constructs a `WaitForEmptyPQOpts` without setting this field (e.g., because the struct is zero-valued), the function will panic at runtime with a nil function call. This is especially risky because the field is a function type and Go structs with function fields are commonly expected to support zero-value safety. Add a nil guard — when `MatchesBackend` is nil, fall back to a direct equality comparison `labelVal == backendURL`, or return early with an error.</violation>
</file>
<file name="api/operator/v1/vlcluster_types.go">
<violation number="1" location="api/operator/v1/vlcluster_types.go:903">
P1: GetRemoteWriteURL returns a broken path-only URL when VLInsert is nil. AsURL returns `""` for a nil VLInsert, but then `"/insert/native"` is appended, producing a URL with no host (e.g., `"/insert/native"`). Callers using this as a remote write target will silently write to a garbage path. Add a nil guard matching VMCluster.GetRemoteWriteURL.</violation>
</file>
<file name="internal/controller/operator/factory/reconcile/reconcile.go">
<violation number="1" location="internal/controller/operator/factory/reconcile/reconcile.go:230">
P1: Error wrapping for cluster types uses `%v` instead of `%w`, breaking error chain for callers that use `errors.Is()` or `errors.As()` to check for `wait.Interrupted`. Downstream `IsRetryable()` relies on `wait.Interrupted(err)` to detect retryable timeout errors — using `%v` here means the interrupted signal is lost and the error will not be treated as retryable, causing an unrecoverable failure.</violation>
</file>
<file name="api/operator/v1/vlsingle_types.go">
<violation number="1" location="api/operator/v1/vlsingle_types.go:363">
P1: The new method doesn't respect the `http.pathPrefix` flag when building the remote write URL. Every other path-building method in this file (ProbePath, GetMetricsPath) and the analogous VMSingle.GetRemoteWriteURL all use `BuildPathWithPrefixFlag` to honor this flag. Without it, if a user configures `http.pathPrefix`, the native insert path `/insert/native` won't be prefixed and the remote write will target the wrong URL.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| }, | ||
| }) | ||
| } | ||
| if !build.IsControllerDisabled("VLSingle") { |
There was a problem hiding this comment.
P1: spec.retain: true still deletes an existing VLSingle when its controller is disabled before VLDistributed deletion: this condition skips removing its owner reference, then removing the parent finalizer lets Kubernetes GC it. Retain cleanup should disown previously created resources regardless of current controller enablement.
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.go, line 37:
<comment>`spec.retain: true` still deletes an existing VLSingle when its controller is disabled before VLDistributed deletion: this condition skips removing its owner reference, then removing the parent finalizer lets Kubernetes GC it. Retain cleanup should disown previously created resources regardless of current controller enablement.</comment>
<file context>
@@ -0,0 +1,101 @@
+ },
+ })
+ }
+ if !build.IsControllerDisabled("VLSingle") {
+ objsToDisown = append(objsToDisown, &vmv1.VLSingle{
+ ObjectMeta: metav1.ObjectMeta{
</file context>
| } | ||
|
|
||
| // GetRemoteWriteURL returns the insert URL for VLCluster (used by VLDistributed) | ||
| func (cr *VLCluster) GetRemoteWriteURL() string { |
There was a problem hiding this comment.
P1: GetRemoteWriteURL returns a broken path-only URL when VLInsert is nil. AsURL returns "" for a nil VLInsert, but then "/insert/native" is appended, producing a URL with no host (e.g., "/insert/native"). Callers using this as a remote write target will silently write to a garbage path. Add a nil guard matching VMCluster.GetRemoteWriteURL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/operator/v1/vlcluster_types.go, line 903:
<comment>GetRemoteWriteURL returns a broken path-only URL when VLInsert is nil. AsURL returns `""` for a nil VLInsert, but then `"/insert/native"` is appended, producing a URL with no host (e.g., `"/insert/native"`). Callers using this as a remote write target will silently write to a garbage path. Add a nil guard matching VMCluster.GetRemoteWriteURL.</comment>
<file context>
@@ -899,6 +899,11 @@ func (cr *VLCluster) AsURL(kind vmv1beta1.ClusterComponent, isExtra bool) string
}
+// GetRemoteWriteURL returns the insert URL for VLCluster (used by VLDistributed)
+func (cr *VLCluster) GetRemoteWriteURL() string {
+ return cr.AsURL(vmv1beta1.ClusterComponentInsert, false) + "/insert/native"
+}
</file context>
| if lastStatus != nil { | ||
| updateStatus = string(lastStatus.UpdateStatus) | ||
| } | ||
| if isCluster && wait.Interrupted(err) { |
There was a problem hiding this comment.
P1: Error wrapping for cluster types uses %v instead of %w, breaking error chain for callers that use errors.Is() or errors.As() to check for wait.Interrupted. Downstream IsRetryable() relies on wait.Interrupted(err) to detect retryable timeout errors — using %v here means the interrupted signal is lost and the error will not be treated as retryable, causing an unrecoverable failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/operator/factory/reconcile/reconcile.go, line 230:
<comment>Error wrapping for cluster types uses `%v` instead of `%w`, breaking error chain for callers that use `errors.Is()` or `errors.As()` to check for `wait.Interrupted`. Downstream `IsRetryable()` relies on `wait.Interrupted(err)` to detect retryable timeout errors — using `%v` here means the interrupted signal is lost and the error will not be treated as retryable, causing an unrecoverable failure.</comment>
<file context>
@@ -213,13 +217,19 @@ func waitForStatus[T client.Object, ST StatusWithMetadata[STC], STC any](
if lastStatus != nil {
updateStatus = string(lastStatus.UpdateStatus)
}
+ if isCluster && wait.Interrupted(err) {
+ return fmt.Errorf("failed to wait for %T=%s to be ready: %v, current status: %s", obj, nsn.String(), err, updateStatus)
+ }
</file context>
| func (cr *VLSingle) GetRemoteWriteURL() string { | ||
| return cr.AsURL(false) + "/insert/native" | ||
| } |
There was a problem hiding this comment.
P1: The new method doesn't respect the http.pathPrefix flag when building the remote write URL. Every other path-building method in this file (ProbePath, GetMetricsPath) and the analogous VMSingle.GetRemoteWriteURL all use BuildPathWithPrefixFlag to honor this flag. Without it, if a user configures http.pathPrefix, the native insert path /insert/native won't be prefixed and the remote write will target the wrong URL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/operator/v1/vlsingle_types.go, line 363:
<comment>The new method doesn't respect the `http.pathPrefix` flag when building the remote write URL. Every other path-building method in this file (ProbePath, GetMetricsPath) and the analogous VMSingle.GetRemoteWriteURL all use `BuildPathWithPrefixFlag` to honor this flag. Without it, if a user configures `http.pathPrefix`, the native insert path `/insert/native` won't be prefixed and the remote write will target the wrong URL.</comment>
<file context>
@@ -358,3 +358,8 @@ func (cr *VLSingle) Paused() bool {
}
+
+// GetRemoteWriteURL returns the native insert URL for VLSingle (used by VLDistributed)
+func (cr *VLSingle) GetRemoteWriteURL() string {
+ return cr.AsURL(false) + "/insert/native"
+}
</file context>
| func (cr *VLSingle) GetRemoteWriteURL() string { | |
| return cr.AsURL(false) + "/insert/native" | |
| } | |
| func (cr *VLSingle) GetRemoteWriteURL() string { | |
| return cr.AsURL(false) + vmv1beta1.BuildPathWithPrefixFlag(cr.Spec.ExtraArgs, "/insert/native") | |
| } |
| @@ -0,0 +1,291 @@ | |||
| package podutil | |||
There was a problem hiding this comment.
P2: WaitForEmptyPQ calls opts.MatchesBackend(labelVal, backendURL) without checking whether opts.MatchesBackend is nil. If a caller constructs a WaitForEmptyPQOpts without setting this field (e.g., because the struct is zero-valued), the function will panic at runtime with a nil function call. This is especially risky because the field is a function type and Go structs with function fields are commonly expected to support zero-value safety. Add a nil guard — when MatchesBackend is nil, fall back to a direct equality comparison labelVal == backendURL, or return early with an error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/podutil/util.go, line 202:
<comment>`WaitForEmptyPQ` calls `opts.MatchesBackend(labelVal, backendURL)` without checking whether `opts.MatchesBackend` is nil. If a caller constructs a `WaitForEmptyPQOpts` without setting this field (e.g., because the struct is zero-valued), the function will panic at runtime with a nil function call. This is especially risky because the field is a function type and Go structs with function fields are commonly expected to support zero-value safety. Add a nil guard — when `MatchesBackend` is nil, fall back to a direct equality comparison `labelVal == backendURL`, or return early with an error.</comment>
<file context>
@@ -0,0 +1,291 @@
+ logger.WithContext(ctx).Error(err, "attempt to get metrics failed", "url", addr, "agent", agentName)
+ return false, nil
+ }
+ for labelVal, v := range metricValues {
+ if !opts.MatchesBackend(labelVal, backendURL) {
+ continue
</file context>
|
|
||
| // WaitForEmptyPQ polls agent metrics across all agents until the persistent queue | ||
| // for backendURL is empty (or the context is cancelled). | ||
| func WaitForEmptyPQ[Agent AgentMetrics]( |
There was a problem hiding this comment.
P3: This new generic polling implementation is unreachable, so queue-drain behavior remains duplicated and future fixes can diverge. Remove it or migrate both zone controllers to call it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/podutil/util.go, line 187:
<comment>This new generic polling implementation is unreachable, so queue-drain behavior remains duplicated and future fixes can diverge. Remove it or migrate both zone controllers to call it.</comment>
<file context>
@@ -0,0 +1,291 @@
+
+// WaitForEmptyPQ polls agent metrics across all agents until the persistent queue
+// for backendURL is empty (or the context is cancelled).
+func WaitForEmptyPQ[Agent AgentMetrics](
+ ctx context.Context,
+ rclient client.Client,
</file context>
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