Repository navigation
cmd/microcloud: Issue each peer's join tokens just before its join - #1613
jrtorsella wants to merge 1 commit into
Conversation
This PR prevents an issue where every service join token was issued before the first join such that if there were many joins to do the tokens for the later joiners could expire before their joins had a chance to start (or finish). This changes the behavior to issue tokens prior to each join rather than all at once at the beginning. Fixes canonical#1565 Signed-off-by: Joseph Robert Torsella <joey.torsella@canonical.com>
There was a problem hiding this comment.
🟢 Approval recommended
The localized change preserves sequential joining and adds focused regression coverage, with no blocking issues identified.
0 open findings
What changed in this PR
This PR prevents join tokens from expiring while peers wait for earlier joins to finish.
Changes:
- Issues each peer’s service tokens immediately before its sequential join.
- Adds a regression test checking token timing and successful peer joins.
| File | Description |
|---|---|
cmd/microcloud/main_init.go |
Moves token issuance into the per-peer join sequence. |
cmd/microcloud/main_init_test.go |
Tests token issuance timing across multiple peers. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
roosterfish
left a comment
roosterfish
left a comment
There was a problem hiding this comment.
Nice approach, a few smaller comments from my side.
| @@ -480,7 +480,7 @@ func (c *initConfig) addPeers(sh *service.Handler) (revert.Hook, error) { | |||
| } | |||
|
|
|||
| // Concurrently issue a token for each joiner. | |||
There was a problem hiding this comment.
Does the comment require a slight update as you now invoke issueTokens for every member?
| // Peers join one at a time, so issue each peer's tokens right before its join. | ||
| // Issuing them all up front lets the tokens of the last peers expire on large clusters. | ||
| // If the local node needs to join an existing cluster, do it first so we can proceed as normal. | ||
| err := issueTokens(sh.Name) |
There was a problem hiding this comment.
Now as this is a function call, I wonder if we should rather pull out the if clusteredSystem.ServerInfo.Name == sh.Name check so that we don't even try generating a token for the local node if it doesn't need to join an existing cluster?
| return nil, err | ||
| } | ||
|
|
||
| cfg := joinConfig[peer] |
There was a problem hiding this comment.
Can we return the tokens from the call to issueTokens instead of populating joinConfig?
This PR prevents an issue where every service join token was issued before the first join such that if there were many joins to do the tokens for the later joiners could expire before their joins had a chance to start (or finish). This changes the behavior to issue tokens prior to each join rather than all at once at the beginning.
Fixes #1565
https://warthogs.atlassian.net/browse/LXD-5259?atlOrigin=eyJpIjoiZTUyMzQ1NzU0NmM4NDIyNWE0NTg0MjE3NmU0ZDBmZTIiLCJwIjoiaiJ9
Checklist