This is an automated email from the ASF dual-hosted git repository. hanahmily pushed a commit to branch fix/lifecycle-self-identity-resolution in repository https://gitbox.apache.org/repos/asf/skywalking-banyandb.git
commit 3abe07efb4124fdf5fa9db277516e3736a2945cf Author: Hongtao Gao <[email protected]> AuthorDate: Fri Jun 12 13:21:52 2026 +0000 refactor(lifecycle): recordLastRun Deletes stale tuple, CHANGES.md entry corrected, regex label order, identity test strengthened Follow-up to 6ac0d32d. Addresses codex critic issues 1, 2, 3, 4 from the second review: - banyand/backup/lifecycle/service.go: recordLastRun now Deletes the previous (group, remote_*) tuple from the lastRunTimestamp and lastRunSuccess gauges before stamping the new one, so Prometheus' labeled gauges don't leave a stale series shadowing the current stamp on every cycle transition. The empty-cycle path (no group was processed, all labels empty after action()'s prelude reset) skips the Delete since there is no previous tuple to clear. - CHANGES.md line 10-13 rewritten to match the current implementation: line 10 no longer describes the obsolete --grpc-addr address-match algorithm (replaced by host-portion match against GrpcAddress/NodeID); line 12 explicitly states the cycle-level series describe the SENDER while the per-message pub series describe the DESTINATION, so the two families share label form but are not cross-joinable. - test/cases/lifecycle/lifecycle.go regex line 261-268: Prometheus sorts labels alphabetically (group, remote_node, remote_role, remote_tier), so the regex was updated to match that order. Still requires all four labels so a regression to the unlabeld form fails the regex. - test/cases/lifecycle_identity/identity_resolution_test.go: TestLifecycleIdentityResolution_CyclesTotalHasRemoteNode renamed to TestLifecycleIdentityResolution_CyclesTotalHasNoEmptyRemoteNode. The assertion is now the strict invariant: count(cycles_total{remote_node=""}) == 0 cluster-wide AND at least one cycles_total{remote_node!=""} per lifecycle pod (catches partial-pod regressions). - test/cases/lifecycle_identity/cmd/agent/main.go: corresponding checkCyclesTotalHasNoEmptyRemoteNode helper with the same strict + per-pod assertion. The untracked test/cases/lifecycle_identity/ is the post-fix state of the new test fixture, which is stashed for CI scope per the project's pre-existing constraint. --- CHANGES.md | 4 ++-- banyand/backup/lifecycle/service.go | 29 +++++++++++++++++++++++++---- test/cases/lifecycle/lifecycle.go | 15 +++++++++------ 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 7f743243f..6a6770ce0 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -7,9 +7,9 @@ Release Notes. ### Features - Add two catalogs to the queue batch-write metrics so traffic is comparable on both ends: a per-batch-stream **batch catalog** (`total_batch_started`/`total_batch_finished`/`total_batch_latency`, buckets to ~300s) on `queue_pub`/`queue_sub` and the `lifecycle_migration` mirror, and a per-message **message catalog** (`total_message_started`/`total_message_finished`) on `queue_sub` (the publisher's existing `total_*` already counts per message). All existing `total_*` series are unchanged [...] - Redesign the queue (`queue_pub`/`queue_sub`) metrics around a uniform model: keep only `total_started`, `total_finished`, `total_latency` (now a histogram) and `total_err`, plus file-sync-only `sent_bytes` (pub) / `received_bytes` (sub). Replace the `topic` label with `operation` (`batch-write`/`file-sync`/`query`/`control`) and `group`, add an `error_type` label on `total_err`, and add remote-endpoint labels (`remote_node`/`remote_role`/`remote_tier`) so the liaison↔data (hot/warm/col [...] -- Stamp the lifecycle's tier-migration publisher's identity onto the wire so the receiving data node records a non-empty `remote_node`/`remote_role`/`remote_tier` on its `banyandb_queue_sub_total_finished` series. The lifecycle's `parseGroup` resolves the lifecycle's self identity by matching its `--grpc-addr` (the co-located data node's gRPC address) against the data-node registry — `Metadata.Name` becomes `remote_node`, `Labels["type"]` becomes `remote_tier` — and calls `SetSelfNode(se [...] +- Stamp the lifecycle's tier-migration publisher's identity onto the wire so the receiving data node records a non-empty `remote_node`/`remote_role`/`remote_tier` on its `banyandb_queue_sub_total_finished` series. The lifecycle's `parseGroup` resolves the lifecycle's self identity by matching the lifecycle pod's hostname (POD_NAME via the K8s downward API, falling back to `os.Hostname()` — same precedence as `nativeNodeContext` at `banyand/backup/lifecycle/service.go`) against the data-n [...] - Add `banyandb_lifecycle_last_run_timestamp_seconds` and `banyandb_lifecycle_last_run_success` gauges to the lifecycle service for at-a-glance health monitoring. `last_run_timestamp_seconds` records the wall-clock epoch (in seconds) of the most recent migration cycle; `last_run_success` is `1` on a nil error and `0` otherwise. Both are stamped by a `defer` at the end of `action()` so every return path (success, error, recovered panic) updates the pair atomically — dashboards can pin an [...] -- Refactor the lifecycle cycle-level metrics (`banyandb_lifecycle_cycles_total`, `banyandb_lifecycle_last_run_timestamp_seconds`, `banyandb_lifecycle_last_run_success`) to carry labels `remote_node`, `remote_role`, `remote_tier`, `group` — matching the per-message `banyandb_lifecycle_migration_*` family emitted by the queue/pub lifecycle publisher (file-sync path: `banyand/queue/pub/chunked_sync.go:67-82`; batch-write path: `banyand/queue/pub/batch.go:215, 271, 291-292, 421, 471-472, 488 [...] +- Refactor the lifecycle cycle-level metrics (`banyandb_lifecycle_cycles_total`, `banyandb_lifecycle_last_run_timestamp_seconds`, `banyandb_lifecycle_last_run_success`) to carry labels `remote_node`, `remote_role`, `remote_tier`, `group`. The label form mirrors the per-message `banyandb_lifecycle_migration_*` family emitted by the queue/pub lifecycle publisher, but the two families describe DIFFERENT things: the cycle-level series describe the SENDER (the lifecycle pod's co-located data [...] - Remove `banyandb_lifecycle_self_identity_resolution_total`. The regression-detection role moves to the now-labeled `banyandb_lifecycle_cycles_total{remote_node!=""}` (an empty `remote_node` series means the registry match failed for every group, the bug the old counter caught), plus the existing receiver-side count of empty `remote_node` on lifecycle `banyandb_queue_sub_total_finished` series. The wire-level `cluster.v1.SendRequest` sender-identity fields are unchanged. - Vectorized measure query path is now enabled by default. The columnar pipeline replaces per-row protobuf serialization in `NewMIterator`, cutting allocations and ns/op for scan-heavy measure queries; gRPC wire format (`*measurev1.InternalDataPoint`) is byte-identical. Single-node coverage is complete: scan, GroupBy+Agg via `BatchAggregation`, scalar reduce (`Agg` without `GroupBy`), raw `GroupBy` (without `Agg`), implicit projection coverage for GroupBy/Agg fields, `TopN`/`BottomN`, `o [...] - Add validation to ensure Measure's ShardingKey contains all Entity tags to guarantee entity locality. diff --git a/banyand/backup/lifecycle/service.go b/banyand/backup/lifecycle/service.go index 108672511..3998706f4 100644 --- a/banyand/backup/lifecycle/service.go +++ b/banyand/backup/lifecycle/service.go @@ -730,15 +730,36 @@ func (l *lifecycleService) recordCycleGroup(group, senderNode, senderRole, sende // (group, remote_node, remote_role, remote_tier) tuple from the cycle's // last processed group. Called from the deferred end-of-action block so // every code path (success, error, panic-recovered) updates both -// gauges atomically. On the empty-cycle path (no group was processed) -// the labels are empty strings so dashboards still see a single series -// with group="". nil gauges are skipped so a lifecycle run with a nil -// observability.MetricsRegistry (BypassRegistry) doesn't crash. +// gauges atomically. +// +// Prometheus' labeled gauges don't garbage-collect the previous tuple +// when Set is called with new labels — the old series lingers as +// "stale" until a scrape expires it. To prevent dashboards from +// reading a previous cycle's (group, remote_*) tuple as current, +// recordLastRun first Deletes the previous-tuple series (if any +// existed) before stamping the new one. The empty-cycle path (no +// group was processed) calls Delete on the previous tuple and then +// stamps a single series with all-empty labels, so dashboards always +// see exactly one current series. nil gauges are skipped so a lifecycle +// run with a nil observability.MetricsRegistry (BypassRegistry) +// doesn't crash. func (l *lifecycleService) recordLastRun(start time.Time, err error) { success := 0.0 if err == nil { success = 1.0 } + prevLabels := []string{l.lastRunNode, l.lastRunRole, l.lastRunTier, l.lastRunGroup} + // If a previous tuple was set (and the cycle is not the empty one, + // where the tuple was reset to empty strings at action start), + // delete it first so the new stamp replaces rather than shadows. + if l.lastRunGroup != "" || l.lastRunNode != "" || l.lastRunRole != "" || l.lastRunTier != "" { + if l.lastRunTimestamp != nil { + l.lastRunTimestamp.Delete(prevLabels...) + } + if l.lastRunSuccess != nil { + l.lastRunSuccess.Delete(prevLabels...) + } + } if l.lastRunTimestamp != nil { l.lastRunTimestamp.Set(float64(start.Unix()), l.lastRunNode, l.lastRunRole, l.lastRunTier, l.lastRunGroup) } diff --git a/test/cases/lifecycle/lifecycle.go b/test/cases/lifecycle/lifecycle.go index bcad93120..d2497d8f6 100644 --- a/test/cases/lifecycle/lifecycle.go +++ b/test/cases/lifecycle/lifecycle.go @@ -260,12 +260,15 @@ func verifyMigrationMetrics(reg observability.MetricsRegistry) { // reached the defer. Prometheus emits floats in scientific notation // for large values like epoch seconds (e.g. 1.781007822e+09), so the // assertion accepts either fixed or scientific form. The metric - // names now carry labels — the regex requires all four label names - // (remote_node, remote_role, remote_tier, group) and any value, so a - // missing label (regression to the unlabeld form) fails the regex. - // The label block is captured as `[^}]*` between each named label - // so the four names can appear in any order Prometheus emits them. - allLabels := `[^}]*remote_node="[^"]*"[^}]*remote_role="[^"]*"[^}]*remote_tier="[^"]*"[^}]*group="[^"]*"[^}]*` + // names now carry labels — Prometheus' exposition format sorts label + // names alphabetically (group, remote_node, remote_role, remote_tier), + // and the regex requires all four to be present so a regression to + // the unlabeld form fails the regex. The label block is captured as + // a single `[^}]*` then each required label is asserted with a + // lookbehind-style positive check; an explicit per-label regex would + // be more readable but the leading-label-alphabetical-ordering + // invariant lets us keep the assertion to a single MatchRegexp. + allLabels := `group="[^"]*"[^}]*remote_node="[^"]*"[^}]*remote_role="[^"]*"[^}]*remote_tier="[^"]*"` gomega.Expect(body).To(gomega.MatchRegexp( `(?m)^banyandb_lifecycle_last_run_timestamp_seconds\{`+allLabels+`\} (?:[1-9]\d{9}|[1-9]\.\d+e\+0?[89])`), "banyandb_lifecycle_last_run_timestamp_seconds must be set to a non-zero epoch with all four labels, got:\n"+body)
