snapshotoffload: wire the scheduler into the server runtime - #1224
snapshotoffload: wire the scheduler into the server runtime#1224bootjp wants to merge 3 commits into
Conversation
Completes M2: the scheduler now actually runs. Opt-in via --snapshotOffloadBucket (S3) or --snapshotOffloadLocalDir, with the §7 configuration surface for region, endpoint, profile, path style, server-side encryption, schedule, jitter, concurrency, spool dir and source-cluster identity. Each local Raft group contributes its own data dir plus both leadership callbacks. Both read the engine through snapshotEngine(): the scheduler outlives startup and races Close(), so a direct field read would be a data race, and a runtime whose engine has been cleared reports "not leader" rather than publishing. A configured-but-unbuildable offload fails startup instead of logging and continuing. An operator who set a backup destination and silently received no backups is worse off than one whose node refused to start. Bucket and local dir are mutually exclusive for the same reason: ambiguity about which destination holds the artifacts is only discovered when a restore is attempted. Adds the scheduler's Prometheus metrics — published/skipped/failed counters, last-published-index gauge, publish-duration and payload-size histograms. The failure counter deliberately carries no error label: messages are unbounded and one recurring failure would explode the metric's cardinality. Skip reasons are normalized into the scheduler's closed set. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
@codex review |
|
@claude review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @bootjp's task in 3m 42s —— View job Claude finished reviewing this PR
SummaryThe wiring itself is careful and well-tested: Finding: design-doc update overstates what's implemented
This replaced the previous, accurate line: "The runtime scheduler and retention/GC remain pending." I couldn't find any retention/GC implementation in the repo:
This PR's own diff ( It's also internally inconsistent with the rest of the same doc, which this PR leaves unchanged:
So the doc now simultaneously says retention/GC is done (new prose) and that it's part of the still- Everything else
|
Closes the M3 operator-documentation item: enabling offload, verifying
that backups are actually being produced, retention semantics, restore,
and failure modes.
Two things the runbook makes explicit because they are the ways an
operator gets silently burned:
- a group whose last_published_index never advances has no backups
even though nothing is failing, so staleness needs its own alert;
- a versioned bucket without a noncurrent-version lifecycle rule
grows without bound while retention reports success, because a
keyed delete only writes a delete marker.
Every flag, metric name and skip reason in the runbook was
cross-checked against the source rather than written from memory.
Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b10815988
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if *snapshotOffloadJitter > 0 { | ||
| opts = append(opts, snapshotoffload.WithSchedulerJitter(*snapshotOffloadJitter)) |
There was a problem hiding this comment.
When --snapshotOffloadInterval is customized and jitter remains at its advertised default of zero, this branch supplies no jitter option, so NewScheduler retains DefaultSchedulerInterval / 4 (3m45s) rather than one quarter of the configured interval. For example, a 1-minute interval unexpectedly gets up to 3m45s of additional delay and per-group staggering instead of 15s; pass the derived quarter-interval jitter explicitly when this flag is zero.
Useful? React with 👍 / 👎.
| `--snapshotOffloadBucket` (or `--snapshotOffloadLocalDir`). Retention/GC | ||
| is implemented per §5. Restore drills and corruption tests are in place; |
There was a problem hiding this comment.
This states that retention/GC is implemented, but a repository-wide search finds no snapshot-offload retention or garbage-collection implementation or runtime hook, and the same document still marks M3 as pending. With the newly wired scheduler, successive manifests and payloads therefore remain indefinitely; operators relying on this claim may omit an external bucket lifecycle policy and incur unbounded storage growth, so keep this capability marked pending until it is implemented and wired.
Useful? React with 👍 / 👎.
| snapshotoffload.WithSchedulerInterval(*snapshotOffloadInterval), | ||
| snapshotoffload.WithSchedulerConcurrency(*snapshotOffloadConcurrency), |
There was a problem hiding this comment.
When an enabled node is started with a non-positive --snapshotOffloadInterval or --snapshotOffloadConcurrency, these options silently ignore the supplied values and retain the scheduler defaults instead of producing the promised startup configuration error. The startup log nevertheless reports the raw invalid values, so an operator can see concurrency zero while uploads still run at concurrency one, or see a negative interval while scans run every 15 minutes; validate these flag values before constructing the options.
Useful? React with 👍 / 👎.
| } | ||
| return store, nil | ||
| } | ||
| store, err := snapshotoffload.NewS3Store(ctx, snapshotoffload.S3StoreConfig{ |
There was a problem hiding this comment.
When --snapshotOffloadEndpoint targets an S3-compatible service that rejects AWS checksum headers, the runtime cannot use the compatibility mode already supported by S3StoreConfig and exposed by the operator CLI as --s3-disable-checksum-headers: this constructor never sets DisableChecksumHeaders, so it is always false. Such a destination can work with manual publish/restore commands but all scheduled runtime publications fail; add an equivalent runtime flag and forward it here.
Useful? React with 👍 / 👎.
| snapshotOffloadSSE = flag.String("snapshotOffloadServerSideEncryption", "", | ||
| "server-side encryption mode for snapshot objects (AES256 or aws:kms)") |
There was a problem hiding this comment.
When an operator supplies the documented bucket opt-in and source-cluster flags but does not also set this flag, its empty default is forwarded to NewS3Store, whose validation rejects an empty server-side-encryption mode and aborts node startup. The standalone snapshot CLI defaults the same setting to AES256, so the runtime's nominally minimal S3 configuration unexpectedly cannot start; use the same safe default or explicitly document and label this flag as mandatory.
Useful? React with 👍 / 👎.
1bda3aa to
50d6113
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50d6113647
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # 2. Restore into a fresh directory. | ||
| elastickv-snapshot-offload restore \ | ||
| --store=s3 --s3-bucket=my-backup-bucket --s3-region=ap-northeast-1 \ |
There was a problem hiding this comment.
When following the earlier enablement example, snapshots are written with aws:kms, but this restore command omits --s3-sse=aws:kms and --s3-kms-key-id, so the CLI defaults to AES256; S3Store.GetObject then rejects the KMS-encrypted manifest because its encryption metadata does not match the configured mode. Include the same KMS settings in the restore command so the documented end-to-end workflow succeeds.
Useful? React with 👍 / 👎.
| # 1. Find the generation to restore. | ||
| elastickv-snapshot-offload publish --help # same store flags as below |
There was a problem hiding this comment.
In a recovery where the manifest key is not already known, this command cannot find a generation: the CLI only implements publish and restore, and publish --help neither connects to nor lists the object store (its flag output is also redirected to io.Discard). The runbook therefore stops before the required --manifest-key can be obtained; provide an actual bucket-listing command or another supported discovery procedure.
Useful? React with 👍 / 👎.
|
|
||
| # 2. Restore into a fresh directory. | ||
| elastickv-snapshot-offload restore \ | ||
| --store=s3 --s3-bucket=my-backup-bucket --s3-region=ap-northeast-1 \ |
There was a problem hiding this comment.
When following the earlier enablement example, snapshots are written with aws:kms, but this restore command omits --s3-sse=aws:kms and --s3-kms-key-id, so the CLI defaults to AES256; S3Store.GetObject validates the stored encryption mode against that configured mode and therefore rejects these KMS-encrypted manifests and payloads as integrity failures.
Useful? React with 👍 / 👎.
| # 1. Find the generation to restore. | ||
| elastickv-snapshot-offload publish --help # same store flags as below |
There was a problem hiding this comment.
This command cannot find a generation: the CLI only implements publish and restore, publish --help performs no object-store listing, and its FlagSet output is discarded, so the documented recovery procedure procedure stops before an operator can obtain the required manifest key. Replace this with an actual S3/local-store listing command or concrete object-listing instructions.
Useful? React with 👍 / 👎.
| label := snapshotOffloadGroupLabel(groupID) | ||
| m.published.WithLabelValues(label).Inc() | ||
| m.lastPublishIndex.WithLabelValues(label).Set(float64(index)) |
There was a problem hiding this comment.
For a group that has never published successfully—such as a new group whose scans keep returning no_persisted_snapshot, or a destination that fails from the first attempt—this gauge is never instantiated because it is touched only on the success path. Consequently the runbook's last_published_index staleness query returns no series rather than a frozen zero, so the recommended freshness alert can silently miss the exact “no backups have ever been produced” case; initialize a zero-valued series for each scheduled group before scans begin.
Useful? React with 👍 / 👎.
| **A misconfigured offload refuses to start the node.** That is deliberate: an | ||
| operator who configured a backup destination and silently received no backups | ||
| is worse off than one whose node failed loudly. |
There was a problem hiding this comment.
When the bucket does not exist, credentials are rejected, or the endpoint is unreachable, startup still succeeds: NewS3Store only validates local fields and constructs an SDK client, while the first remote request is deferred until the scheduler's initial interval elapses. This absolute guarantee can make operators treat a running node as evidence that its backup destination works; document that only locally detectable configuration errors fail startup, or add an explicit remote preflight if fail-fast behavior is intended.
Useful? React with 👍 / 👎.
| Exit codes: `0` success, `1` invalid invocation, `2` missing or invalid | ||
| snapshot data. Automation should distinguish these. |
There was a problem hiding this comment.
The CLI's classifyError returns code 1 for every error not matching the small set of data-error sentinels, including S3 authorization failures, network outages, and other runtime/store errors, not just invalid invocations. Automation following this table can therefore misclassify a retryable restore outage as a bad command; either distinguish operational failures in the implementation or document code 1 as the catch-all non-data error.
Useful? React with 👍 / 👎.
| snapshotOffloadPrefix = flag.String("snapshotOffloadPrefix", "", | ||
| "key prefix below which snapshot artifacts are written") |
There was a problem hiding this comment.
When two clusters use the same backup bucket and prefix—which the empty default and generic documented prefix permit—their manifest keys collide because the key contains only group, index, and term while sourceCluster is merely stored in the manifest body. Once both clusters reach the same group/index/term, the immutable write sees different manifest contents and one cluster's offload fails repeatedly; derive a cluster-specific namespace from snapshotOffloadSourceCluster, or require and document a unique prefix per cluster.
Useful? React with 👍 / 👎.
| --manifest-key='elastickv/v1/groups/1/snapshots/00000000000000004211-00000000000000000007.json' \ | ||
| --data-dir=/var/lib/elastickv/n1 \ | ||
| --peers='n1=10.0.0.1:50051,n2=10.0.0.2:50051,n3=10.0.0.3:50051' |
There was a problem hiding this comment.
For a multi-group node, this target path is not where the server looks for group 1: groupDataDir resolves it as /var/lib/elastickv/n1/group-1 (and every other group needs its own restored directory), so the subsequent normal startup ignores the snapshot placed at /var/lib/elastickv/n1. In a multi-group disaster-recovery run this procedure therefore fails to restore the node's complete state; document restoring every manifest into its exact per-group path, while retaining the current root path only for an explicitly single-group example.
Useful? React with 👍 / 👎.
1 participant
What
Wires the M2 scheduler into the server runtime, so physical snapshot offload actually runs. This is what turns #1220 from a library into a feature.
Opt-in via
--snapshotOffloadBucket(S3) or--snapshotOffloadLocalDir, plus the §7 configuration surface: region, endpoint, profile, path style, server-side encryption + KMS key, interval, jitter, concurrency, spool dir, and source-cluster identity.Also adds the scheduler's Prometheus metrics (published / skipped / failed counters, last-published-index gauge, publish-duration and payload-size histograms), which M2 listed and #1220 had no home for.
Decisions worth reviewing
Leadership callbacks go through
snapshotEngine(), not the field. The scheduler outlives startup and racesClose(), so a directrt.engineread would be a data race — the same reason the keyviz publisher uses that accessor. A runtime whose engine has been cleared reports not leader, so shutdown fails closed rather than publishing.Configured-but-unbuildable fails startup. An operator who set a backup destination and silently got no backups is worse off than one whose node refused to boot. Same reasoning makes bucket and local dir mutually exclusive: ambiguity about which destination holds the artifacts surfaces only when someone attempts a restore.
Per-group data dirs come from
groupDataDir. Publishing a group's snapshot from another group's directory would ship the wrong state under the right manifest identity — pinned by a test, and revert-checked.The failure counter has no error label. Error text is unbounded; one recurring failure would explode the metric's cardinality. Diagnosis comes from the scheduler's log line. Skip reasons are normalized into the scheduler's closed set for the same reason.
Behavior change / risk
Nothing changes for a node that does not set an offload flag —
snapshotOffloadEnabled()is checked before any other offload config is even validated, so an unconfigured node cannot fail startup on offload settings. Pinned byTestSnapshotOffloadIsOptIn.When enabled, the scheduler runs in the existing errgroup and returns only on context cancellation; a failing group is retried next tick rather than tearing the process down, since an object-store outage must not stop serving.
Test evidence
go test . ./monitoring/ ./internal/snapshotoffload/ -race -count=1— all passgolangci-lint run(full repo) — 0 issues, no//nolintaddeddiff -q):TestSnapshotOffloadRejectsAmbiguousDestinationFAILsTestSnapshotOffloadGroupsCarryPerGroupDataDirsFAILsTestSnapshotOffloadLeadershipFailsClosedOnAClosedEngineFAILs10 new tests: 6 on the wiring (opt-in, ambiguity, local store, per-group dirs, closed-engine fail-closed, incomplete config) and 4 on the metrics (outcomes, reason-label bounding, no error label, nil-receiver).
Self-review (five passes)
snapshotEngine()accessor that exists for exactly this race; the scheduler's own single-flight and shared upload bound come from snapshotoffload: add the M2 leader-only publish scheduler #1220. Race-clean.https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE