feat(launchpad): make /start restore the baseline instead of restarting flagd - #396
Open
aepfli wants to merge 1 commit into
Open
feat(launchpad): make /start restore the baseline instead of restarting flagd#396aepfli wants to merge 1 commit into
aepfli wants to merge 1 commit into
Conversation
…ng flagd A conformance client that wants per-scenario isolation has only /start to call -- the control API marks /reset optional (open-feature/spec#423) and this testbed does not implement it -- so it pays a full flagd stop-and-start before every scenario, for a configuration that has not changed. Post-#394 that is ~140-190ms each, over the testbed's ~325 executed scenario instances. The process is not what carries a scenario's leftovers; the flag definitions are. /start now reuses the running flagd when the requested configuration is the one already running, the process is alive, and no delayed restart is pending, and restores the baseline flag state in place instead. Restoring needs no pristine copy of the definitions, which is just as well, because the image does not carry one: /change overwrites its own source, rawflags/changing-flag.json, in the container's writable layer. But /change is a two-state toggle on a single flag, so the shipped baseline is recoverable by reading the current variant and flipping it back when it reads "bar". Reading it rather than remembering it is deliberate. A launchpad that restarts inside a container whose writable layer already holds "bar" would believe a remembered flag, and serve "bar" while reporting a restored baseline. That read is only sound because writes are now honest. /change used to wait for our own file watcher to regenerate the merged file and then return; measured against v0.16.0 in the built image, flagd serves the new variant about a second later, because it watches with the fileinfo watcher whose default poll interval is 1000ms. So /change reported success while flagd still served the old value, and a restore could not tell a file reading "foo" apart from a flagd that had not caught up with one. Both writes now wait for flagd to serve what was written, which is what makes the file content readable as the state flagd is in. Measured in the built image: | case | before | after | | --------------------------------------------- | ----------- | ------- | | /start, same config, nothing changed | ~140-190ms | ~9-10ms | | /start, same config, after a /change | ~140-190ms | ~1.0s | | /start, other config, or after /stop, or in a | | | | /restart window | ~140-190ms | ~125ms | | /change | ~0ms, wrong | ~1.0s | Two scenarios call /change, so a suite of a few hundred scenarios pays the second row about three times and the first row for everything else. Also fixes a leaked flagdLock: StartFlagd returned without unlocking when stopFlagDWithoutLock failed. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
|
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:
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
POST /startrestartsflagdunconditionally, and does not restore the flag definitions the container shipped with. This makes it reuse the running process when the requested configuration is the one already running, and restore the baseline in place instead.Stacked on #394 — it reuses that PR's OFREP probe, and its measurements are relative to it. Closes #395.
Why not a
POST /resetThat is what #395 originally asked for, and it is the wrong shape.
/resetis optional in the control API contract (spec#423) withPOST /start?config=defaultas the mandatory fallback, so every client implements the/startpath regardless. A new endpoint would reach only the clients that opt in and would leave a second branch in the contract to document and test. Making/startcheap gives every adoption the same saving with no client change and no new surface.Restoring without a pristine copy
The image does not carry one.
/changewrites back torawflags/changing-flag.json— its own source, and the input toCombineJSONFiles— so after one call the shippeddefaultVariantis gone from the container for good, and regenerating the merged file reproduces the mutated value.No snapshot is needed, though, because the baseline that has to be recoverable is not the whole directory. It is the one value that can be mutated, and
/changeis a two-state toggle on a single flag: reading the current variant and flipping it back when it reads"bar"restores the shipped"foo". That is a named constant rather than a build-time copy and a new directory in the image.Reading the variant rather than remembering it is deliberate. A launchpad that restarts inside a container whose writable layer already holds
"bar"would believe a remembered boolean and serve"bar"while reporting a restored baseline. It is also what makes the common case free: most scenarios never call/change, so most restores write nothing and wait for nothing./changewas not honest, and the restore depends on it being honestReading the file as the state flagd is in only works if the two cannot drift. They could:
ToggleChangingFlagwaited for our file watcher to regenerate the merged file and then returned, which says nothing about flagd having re-read it. Two watchers, one observed.Measured in the built image against
v0.16.0, that gap is about a second —/changereturns, andchanging-flagstill resolves to the old variant for ~500-600ms:flagd picks the
fileinfowatcher here, whose default poll interval is 1000ms (core/pkg/sync/file/fileinfo_watcher.go:38), and it is not configurable per source.Left alone, this made the restore probe meaningless: after
/changethe store still held"foo", so polling for"foo"succeeded instantly against a value that was about to become"bar". Both writes now wait for flagd to serve what was written, which fixes/changeand makes the file content readable as the state flagd is in.Cost
Measured in the built image, timing the HTTP calls:
/start, same config, nothing changed/start, same config, after a/change/start, different config/startafter/stop, or during a/restartwindow/changeTwo scenarios call
/change(gherkin/events.feature:37andgherkin/rpc-caching.feature:28, three calls between them), so a suite of a few hundred scenarios pays the second row about three times and the first row for everything else. Against the testbed's ~325 executed scenario instances that is roughly 54s of restarts traded for roughly 6s.Guards
The fast path is taken only when the process is alive, the requested configuration equals the running one, and no delayed restart is pending. Verified in the image:
/startafter/stopand/startinside a/restartwindow both fall through to a real start, as does a configuration switch.A configuration that does not read the merged flag file —
metadata.json— cannot servechanging-flagat all, so the restore skips the probe for it rather than burning the whole budget before failing. Covered by a unit test.Behavioural notes for reviewers
/change. It fires a realPROVIDER_CONFIGURATION_CHANGEDto anything already connected, indistinguishable from a genuine one./changestays a single two-state toggle on a single flag. That is true today and structurally enforced — the handler takes no parameters — andChangingFlagFilecarries a comment saying so. A future/change?flag=x&variant=ywould need the snapshot after all.flagdLock:StartFlagdreturned without unlocking whenstopFlagDWithoutLockfailed.On open-feature/flagd#2050
That PR makes flagd's
/readyzwait for the evaluator to be updated, which is the upstream half of #394. It does not overlap with this one: the fast path's problem is not readiness but a flagd that is already ready and serving the previous scenario's state, so the value probe is needed either way.Testing
go build ./...,go vet ./...,gofmt -lclean on the touched files,go test -count=1 ./...— new table tests forfileSourcesandservesCombinedFlags/change→restore cycles, configuration switch,/startafter/stop,/startinside a/restartwindow. Container logs confirm the fast path does not restartflagd(11 reuses, no accompanying stop).🤖 Generated with Claude Code