Skip to content

Run initial recompute exactly once in State.Start - #636

Merged
justinsb merged 1 commit into
gke-labs:mainfrom
codebot-robot:issue_632
Oct 7, 2026
Merged

justinsb merged 1 commit into
gke-labs:mainfrom
codebot-robot:issue_632

Conversation

@codebot-robot

@codebot-robot codebot-robot commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #632

@justinsb

justinsb commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator

Thanks! The approach is right: SetSynced only sets the flag and Start runs one explicit Recompute(). A few things before merging:

  1. Rebase carefully onto Incremental state: follow-ups from #629 review #631. This branch predates Incremental state: follow-ups from #629 review #631, which is why it conflicts. Main already removed recomputeAsyncLocked and added the dirty flag and IsDirty().
    • The helper you renamed now does s.dirty = true and then notifies the loop. The rebased version must keep that line, and Recompute() must still clear dirty.
    • Because the helper both marks the state dirty and notifies, please call it markDirtyLocked rather than notifyRecomputeLocked.
    • Main's SetSynced has an if synced && running { s.Recompute() } guard. It should end up only setting the flag, as in this PR.
  2. Don't add Recomputes() to the public State API just for tests. The tests are in package state, so they can read an unexported counter under s.mu. A small unexported helper is fine.
  3. Replace the fixed time.Sleep(50 * time.Millisecond) waits with polling. Fixed sleeps are flaky on a loaded CI runner.
    • Poll until the recompute count is at least 1, with a deadline. Then check it stays at exactly 1 over a short window.
    • Likewise, poll for the second recompute after UpsertNamespace.
  4. Minor: the countingProxyUpdater assertion is a good addition. Please add a one-line comment explaining what it checks: only one proxy push reaches the data plane at startup. (A no-op second recompute would not push anyway, so the recompute counter is the main assertion.)

@justinsb

justinsb commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Please rebase onto main, which now includes #631 and #624, and address the earlier review. In short:

  1. Keep Incremental state: follow-ups from #629 review #631's dirty flag. Rename triggerRecomputeLocked to markDirtyLocked, which sets dirty and notifies. Fast-path provisioning: per-Gateway LoadBalancer Services and per-Gateway ports #624 added callers in SetGatewayReadiness and SetGatewayProvisioningError, so update those too.
  2. Remove the public Recomputes(). The tests are in package state, so they can read an unexported counter.
  3. Replace the time.Sleep(50ms) waits with polling against a deadline.

And please replace the PR body with just Fixes #632.

- SetSynced only sets the synced flag without running Recompute
- State.Start explicitly triggers Recompute once after informers sync
- Rename triggerRecomputeLocked to markDirtyLocked, preserving the dirty flag lifecycle
- Add counting unit tests with polling for single startup recompute and proxy update

Fixes gke-labs#632

@justinsb justinsb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@justinsb
justinsb merged commit 132bd13 into gke-labs:main Oct 7, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

State.Start runs the initial recompute twice

2 participants