improvements to initial member handling - #10176
Conversation
👷 Deploy Preview for chef-habitat processing.
|
There was a problem hiding this comment.
Pull request overview
This PR modifies the Habitat Supervisor's peer discovery mechanism to continuously ping initial members from the peer watch file, rather than stopping after reaching the minimum threshold. The change enables dynamic peer discovery when new initial members are added to the watch file during runtime.
Changes:
- Removed the early return check that prevented peer seeding when members existed
- Added a signaling mechanism to notify the outbound thread when new initial members are added
- Modified the outbound loop to re-evaluate and ping initial members when new ones are detected
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| components/sup/src/manager.rs | Removed early return check and added signal call when initial members are updated from watch file |
| components/butterfly/src/server/outbound.rs | Added logic to detect new initial members signal, skip pinging already-alive members, and respect min_to_start threshold |
| components/butterfly/src/server.rs | Added new_initial_members atomic flag and signal_new_initial_members() method; removed unused need_peer_seeding_mlr() function |
Signed-off-by: Matt Wrock <matt@mattwrock.com>
Signed-off-by: Matt Wrock <matt@mattwrock.com>
Signed-off-by: Matt Wrock <matt@mattwrock.com>
… review Signed-off-by: Matt Wrock <matt@mattwrock.com>
2b8eb7d to
4d21841
Compare
Signed-off-by: Matt Wrock <matt@mattwrock.com>
| /// Signal that new initial members have been added and should be pinged on the next outbound | ||
| /// cycle | ||
| pub fn signal_new_initial_members(&self) { | ||
| self.new_initial_members.store(true, Ordering::Release); |
There was a problem hiding this comment.
Missing import for Ordering. The atomic::Ordering type is used here but not imported in the visible portion of this file. Ensure use std::sync::atomic::Ordering; is added to the imports at the top of the file.
| @@ -16,9 +16,6 @@ services: | |||
| - run | |||
| - --listen-ctl=0.0.0.0:9632 | |||
| - --peer-watch-file=/hab/PEERS | |||
There was a problem hiding this comment.
The volume mount for the PEERS file has been removed, but the supervisor is still configured to watch /hab/PEERS via --peer-watch-file. Without the volume mount, the file won't exist or be accessible for dynamic updates during the test. This will cause the peer watcher test to fail.
| - --peer-watch-file=/hab/PEERS | |
| - --peer-watch-file=/hab/PEERS | |
| volumes: | |
| - ./testcases/peer_watcher/PEERS:/hab/PEERS |
| let new_initial_members_added = server.new_initial_members.load(Ordering::Acquire); | ||
| if new_initial_members_added { | ||
| // Reset the flag and force re-evaluation of initial member pinging | ||
| server.new_initial_members.store(false, Ordering::Relaxed); |
There was a problem hiding this comment.
The flag is loaded with Ordering::Acquire but stored with Ordering::Relaxed. For proper synchronization, this should use Ordering::Release to ensure the store is visible to other threads that may be loading it.
| server.new_initial_members.store(false, Ordering::Relaxed); | |
| server.new_initial_members.store(false, Ordering::Release); |
Signed-off-by: Matt Wrock <matt@mattwrock.com>
Signed-off-by: Matt Wrock <matt@mattwrock.com>
| - run | ||
| - --listen-ctl=0.0.0.0:9632 | ||
| - --peer-watch-file=/hab/PEERS | ||
| volumes: | ||
| - ./testcases/peer_watcher/PEERS:/hab/PEERS | ||
|
|
||
|
|
||
| tester: |
There was a problem hiding this comment.
The peer watch file path is configured on the bastion supervisor (--peer-watch-file=/hab/PEERS), but the bind mount for ./testcases/peer_watcher/PEERS is now only on the tester service. This means edits made by the test may not be visible inside the bastion container, so the supervisor won’t detect changes. Fix by mounting the same host file into bastion as well (or use a shared named volume mounted into both containers at /hab/PEERS).
| volumes: | ||
| - source: ./testcases/peer_watcher/PEERS | ||
| target: /hab/PEERS | ||
| type: bind |
There was a problem hiding this comment.
The peer watch file path is configured on the bastion supervisor (--peer-watch-file=/hab/PEERS), but the bind mount for ./testcases/peer_watcher/PEERS is now only on the tester service. This means edits made by the test may not be visible inside the bastion container, so the supervisor won’t detect changes. Fix by mounting the same host file into bastion as well (or use a shared named volume mounted into both containers at /hab/PEERS).
| let new_initial_members_added = server.new_initial_members.load(Ordering::Acquire); | ||
| if new_initial_members_added { | ||
| // Reset the flag and force re-evaluation of initial member pinging | ||
| server.new_initial_members.store(false, Ordering::Relaxed); |
There was a problem hiding this comment.
Resetting new_initial_members with a separate load() then store(false, ...) can lose signals: if another thread sets the flag to true between the load and the store, this store will overwrite it back to false, and the outbound loop may miss the update. Use an atomic read-modify-write (e.g., swap(false, Ordering::AcqRel) or compare_exchange loop) to clear the flag without dropping concurrent updates, and use a non-Relaxed ordering consistent with the producer’s Release store.
| let new_initial_members_added = server.new_initial_members.load(Ordering::Acquire); | |
| if new_initial_members_added { | |
| // Reset the flag and force re-evaluation of initial member pinging | |
| server.new_initial_members.store(false, Ordering::Relaxed); | |
| let new_initial_members_added = | |
| server.new_initial_members.swap(false, Ordering::AcqRel); | |
| if new_initial_members_added { | |
| // Reset the flag and force re-evaluation of initial member pinging |
|
|
||
| It "adds beta to the peer watch file and finds it as a peer" { | ||
| Add-Content -Path "/hab/PEERS" -Value "beta.habitat.dev" | ||
| Start-Sleep -Seconds 5 # give butterfly some time to detect the change and update the census |
There was a problem hiding this comment.
The fixed Start-Sleep makes the e2e test timing-dependent and more prone to flakes under load/CI variance. Prefer polling with the existing Wait-True helper until the census reflects the expected last_membership_counter (with a reasonable timeout and error message), instead of sleeping a fixed duration.
| Start-Sleep -Seconds 5 # give butterfly some time to detect the change and update the census | |
| $testScript = { (Invoke-WebRequest "http://bastion.habitat.dev:9631/census" | ConvertFrom-Json).last_membership_counter -eq 3 } | |
| $timeoutScript = { Write-Error "Timed out waiting 45 seconds for beta to be detected and the census to reach last_membership_counter 3" } | |
| Wait-True -TestScript $testScript -TimeoutScript $timeoutScript -Timeout 45 |
Signed-off-by: Matt Wrock <matt@mattwrock.com>
No description provided.