bundle/direct/dstate: read-only feature-flag gate for direct deployment state#5660
Conversation
Integration test reportCommit: 6f1292a
8 interesting tests: 4 SKIP, 2 RECOVERED, 2 flaky
Top 10 slowest tests (at least 2 minutes):
|
663ad08 to
a1b0613
Compare
| for name := range db.Features { | ||
| names = append(names, name) | ||
| } | ||
| slices.Sort(names) |
There was a problem hiding this comment.
makes tests with multiple features deterministic.
a1b0613 to
a550105
Compare
| // RecordFeature marks the state as depending on the named feature, stamping the | ||
| // minimum CLI version required to read it (from featureMinCLIVersion). It must be | ||
| // called before the WAL is started (UpgradeToWrite) so the change is persisted. | ||
| func (db *DeploymentState) RecordFeature(name string) { |
There was a problem hiding this comment.
Get rid of this function - make this decision at open.
| { | ||
| "state_version": 3, | ||
| "features": { | ||
| "dummy": "1.2.0" |
There was a problem hiding this comment.
talk to Julia, if aligned get rid of versions from state and consider hosting every feature on our docs page.
a550105 to
2d62964
Compare
2d62964 to
e559fca
Compare
e559fca to
c62c873
Compare
c62c873 to
1a7c392
Compare
| if db.StateVersion > currentStateVersion { | ||
| return fmt.Errorf("state version %d is newer than supported version %d; upgrade the CLI", db.StateVersion, currentStateVersion) | ||
| if db.StateVersion > featureStateVersion { | ||
| return fmt.Errorf("state version %d is newer than supported version %d; upgrade the CLI", db.StateVersion, featureStateVersion) |
There was a problem hiding this comment.
should we allocate constant supportedStateVersion? Currently it's 3, the same as featureStateVersion but in the future the constant will remain but it'll be equal to currentStateVersion most likely unless we execute another 2-phase bump like this one.
There was a problem hiding this comment.
We would just cleanup featureStateVersion constant in the future and be left again with just the current version. I'm not sure I followed exactly what the recommendation was here.
I added supportedStateVersion in any case.
|
|
||
| Exit code: 1 | ||
|
|
||
| === a version-3 state with an empty features map is accepted (treated as version 2) |
There was a problem hiding this comment.
We should have another test with non-empty deploy to ensure that state is not bumped.
| // CLI lacks, so refuse it and tell the user to upgrade. | ||
| if db.StateVersion == featureStateVersion { | ||
| if len(db.Features) == 0 { | ||
| db.StateVersion = currentStateVersion |
There was a problem hiding this comment.
This will flip flop the version between 2/3. That's fine though because that only happens when there are no features. If that happens then a newer client will upgrade it back to 3.
There was a problem hiding this comment.
We should not flip the version on disk; can we keep at 3? just skip the migrations below.
There was a problem hiding this comment.
Agree, we can return here. No other checks needed.
c724a74 to
6f1292a
Compare
| if db.StateVersion > currentStateVersion { | ||
| return fmt.Errorf("state version %d is newer than supported version %d; upgrade the CLI", db.StateVersion, currentStateVersion) | ||
| if db.StateVersion > featureStateVersion { | ||
| return fmt.Errorf("state version %d is newer than supported version %d; upgrade the CLI", db.StateVersion, featureStateVersion) |
| // CLI lacks, so refuse it and tell the user to upgrade. | ||
| if db.StateVersion == featureStateVersion { | ||
| if len(db.Features) == 0 { | ||
| db.StateVersion = currentStateVersion |
There was a problem hiding this comment.
Agree, we can return here. No other checks needed.
…nt state Add forward-compat scaffolding so this CLI reads a future "feature flags" deployment state correctly. featureStateVersion (3) is the version a future CLI will write once it records Header.Features; this CLI writes no features and always stamps currentStateVersion (2). On read: currentStateVersion passes; featureStateVersion passes iff its features list is empty (normalized to version 2); a featureStateVersion state recording any feature is refused with the offending feature keys and a link to the docs, telling the user to upgrade. The v3-empty == v2 equivalence is special-cased to version 3 only and pinned by a forcing-function unit test so it is removed when the baseline is really bumped to 3. Co-authored-by: Isaac
Address review feedback (Denis, Pieter): the "state too new" check compared against featureStateVersion, conflating "the version a future CLI writes" with "the highest version this CLI can read". Introduce supportedStateVersion for the read ceiling. It equals featureStateVersion during this two-phase bump and tracks currentStateVersion the rest of the time; the check now reads against it. Co-authored-by: Isaac
Address review feedback (Denis, Pieter): a featureStateVersion state with no features is accepted by returning early from migrateState without running the downstream migrations, leaving the on-disk version at featureStateVersion. The previous code flipped it down to currentStateVersion, so a deploy rewrote a v3 state as v2. Acceptance test now deploys and asserts state_version stays 3. Co-authored-by: Isaac
9efcb33 to
be41023
Compare
Why
Forward-compat scaffolding so this CLI reads a future "feature flags" deployment state correctly, without writing one yet. A later release can add a real feature and flip the baseline; until then this change bakes in the read behavior with minimal friction.
currentStateVersion(2) is what this CLI writes.featureStateVersion(3) is the version a future CLI will write once it recordsHeader.Features.supportedStateVersionis the highest version this CLI can read. It equalsfeatureStateVersionduring this two-phase bump and trackscurrentStateVersionotherwise.What
On read (
migrateState):migrateStatereturns early, leaving the on-disk version at 3 (it is not flipped down to 2, so a subsequent deploy keeps writing version 3);supportedStateVersionis rejected as too new.The version-3 special case is scaffolding, pinned by a forcing-function unit test (
TestEmptyFeatureStateAcceptedWithoutFlippingVersion) so it is removed when the baseline is really bumped to 3. Thefeature_flagsacceptance test deploys an empty-features v3 state and assertsstate_versionstays 3 on disk.This pull request and its description were written by Isaac.