Improve apply link reconciliation - #3260
Conversation
|
Dead code left behind |
| } | ||
|
|
||
| if err := deployable.Deploy(ctx); err != nil { | ||
| if err := clablinks.DeployEndpoint(ctx, ep); err != nil { |
There was a problem hiding this comment.
Why use a Function instead of the Method that was there?
But im my view you should even just call link.Deploy() ... and the link should figure out what needs to be done...
There was a problem hiding this comment.
this func indeed calls link.Deploy(ctx, ep). It is a helper method that adds a couple of nil checks prior to calling link.Deploy(ctx, ep)
There was a problem hiding this comment.
I see that... but if you have an endpoint, there should by contract be a Link associated... and I don't see why you would have a nil endpoint...
So if you then simply call Deploy on endpoint, the links is implicitely being deployed ...
There was a problem hiding this comment.
by now there is just way to many functions floting around somewhere that magically do things that should be covered as an integrated part of the standards workflow.
// activateEndpoint brings the endpoint's interface up in its current namespace.
func activateEndpoint(ctx context.Context, e Endpoint) error {
return e.GetNode().ExecFunction(ctx, func(_ ns.NetNS) error {
link, err := netlink.LinkByName(e.GetIfaceName())
if err != nil {
return err
}
return netlink.LinkSetUp(link)
})
}
why is this not implemented as Activate function on the GenericEndpoint... but referenced in every Endpoint and called specifically... things like that... the cohesion of this stuff is quite low and therefore unfortunately the complexity is high.
move endpoint. the same
func moveEndpoint(ctx context.Context, e Endpoint, dst Node) error {
There was a problem hiding this comment.
I'll put move and activate on the generic endpoint, good call.
There was a problem hiding this comment.
the move func turned to be dependent on the concrete endpoint type, so promoting it to generic endpoint did not work. But I removed the interface MoveTo method that was redundant eb5578f
…nd improve clarity.
hellt
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3260 +/- ##
==========================================
+ Coverage 56.76% 56.90% +0.14%
==========================================
Files 248 248
Lines 25838 25848 +10
==========================================
+ Hits 14667 14709 +42
+ Misses 9496 9469 -27
+ Partials 1675 1670 -5
🚀 New features to boost your workflow:
|
|
lets have it in and soak it through user testing |
Re-integrate the veth-stitch link with the new deploy/apply orchestration (#3274 unify deploy, #3260 apply reconciliation): - move the tc stitch into LinkVEthStitched.PostDeploy (upstream calls link.PostDeploy generically, replacing the old type-cast post-deploy pass); node workers deploy the two segments, so the composite Deploy "join" and its Remove mutex are retired. - implement GetRuntimeEndpoints (the two node-side endpoints) in place of the removed ApplyRuntimeEndpoints type-switch case, preserving apply park/restore. - keep the apply-remove veth-stitch netns GC (now after deployLinks). - drop EndpointOwner (folded into the Node interface upstream); keep NetemTarget. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 participants
Summary
links.Linkcontract withGetRuntimeEndpoints()linkspackage and update apply/deploy callersValidation
go test ./links ./core ./cmdSplit out from #3256 so that PR can stay focused on agent skills only.