-
Notifications
You must be signed in to change notification settings - Fork 51
fix(buildernet): make --rbuilder flag work #420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,8 @@ func (b *BuilderNetRecipe) Apply(ctx *ExContext) *Component { | |
| // Apply beacon service overrides for buildernet. | ||
| // We need these for letting the builder connect to the beacon node. | ||
| // Basically, the beacon node can never be healthy until the builder | ||
| // connects. | ||
| // connects, so we also drop its healthmon and downgrade any consumer's | ||
| // dependency on beacon from healthy to running. | ||
| if beacon := component.FindService("beacon"); beacon != nil { | ||
| beacon.ReplaceArgs(map[string]string{ | ||
| "--target-peers": "1", | ||
|
|
@@ -54,10 +55,15 @@ func (b *BuilderNetRecipe) Apply(ctx *ExContext) *Component { | |
| if mevBoostRelay := component.FindService("mev-boost-relay"); mevBoostRelay != nil { | ||
| mevBoostRelay.DependsOnNone() | ||
| } | ||
| // Remove beacon healthmon - doesn't work with --target-peers=1 which is required for builder VM | ||
| component.RemoveService("beacon_healthmon") | ||
|
|
||
| component.RunContenderIfEnabled(ctx) | ||
| // Beacon never reaches healthy in buildernet, so any consumer that asked | ||
| // for healthy must accept running instead. | ||
| component.WalkServices(func(svc *Service) { | ||
| svc.ReplaceDependency("beacon", DependsOnConditionRunning) | ||
| }) | ||
|
Comment on lines
+59
to
+63
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| // Note: contender is already added by L1Recipe.Apply, so we do not call | ||
| // RunContenderIfEnabled here. | ||
|
|
||
| return component | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new
RemoveServicesemantics (always cleans up dangling sidecar labels) and the newWalkServices/ReplaceDependencyhelpers are not covered bymanifest_test.go— only the originalRemoveServicecases are. A few small unit tests would lock in the contract being introduced here (in particular, thatRemoveService("X")clears anyhealth-check-sidecar=Xlabel across the tree, including in nested components), so a future caller doesn't accidentally regress this back to the dangling-pointer state this PR is fixing.Minor:
clearSidecarLabelalways walks the entire tree even whenremoveServiceRecursivereturnedfalse(i.e. nothing was removed). Harmless, but you could skip it in that case.