ateomnet: create the actor veth peer inside the actor netns - #850
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the actor network setup by creating the veth pair with its peer already in the target namespace under its final name, significantly reducing setup latency by avoiding costly netlink rename and move operations. It also adds a comprehensive set of unit tests in net_linux_test.go. The review feedback highlights a compilation error in the tests due to the use of a non-existent errors.AsType function, and suggests using errors.As for more robust error checking in both the tests and the main network configuration code.
| if _, ok := errors.AsType[netlink.LinkNotFoundError](err); ok { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The function errors.AsType does not exist in the Go standard library errors package. This will cause a compilation failure. Use the standard errors.As function instead.
| if _, ok := errors.AsType[netlink.LinkNotFoundError](err); ok { | |
| return nil | |
| } | |
| var lnf netlink.LinkNotFoundError | |
| if errors.As(err, &lnf) { | |
| return nil | |
| } |
There was a problem hiding this comment.
7abdbb7 to
025b94b
Compare
|
Do we have any issues with the fact that the test may require privileged to run? |
I e2e tested it with both uVM and gvisor ateoms, gvisor ateom is not full I don't think so? |
SetupActorNetwork built the veth pair in the worker pod netns with the peer under a temporary name, moved the peer into the actor netns with LinkSetNsFd, and renamed it to eth0 on the far side. The temporary name existed only because the pod netns has an eth0 of its own for the peer to collide with. Those two operations were 13.8ms of the 15.0ms of veth work: each drives a netdev unregister/register with an RCU grace period taken under the global RTNL lock. Neither parallelizes, so the fix is to not do them. Creating the pair with PeerName plus PeerNamespace has the peer born in the actor netns already called eth0, which costs neither: the name is resolved in the peer's namespace, so it never sees the pod's eth0. Measured on the real function, 200 activations on a 6.8 kernel: before SetupActorNetwork mean=18.07ms p50=17.83ms p99=26.81ms after SetupActorNetwork mean=2.96ms p50=2.69ms p99=5.72ms That is ~15ms off every actor resume, against a 100ms p95 activation target, since RestoreWorkload calls this before runsc restore. CleanupActorNetwork is unchanged at ~16ms; that cost is the veth delete and it lands on suspend. The resulting namespace is identical, which is what the new tests assert: they pin the end state gVisor and the micro-VM guest read -- which links exist, in which namespace, with which addresses and routes -- rather than the sequence of netlink calls that produced it. They also cover repeated activation on a reused worker, cleanup idempotency, and the two micro-VM options (the fixed host veth MAC that keeps a restored guest's frozen ARP entry valid, and the interior link sweep). They run in throwaway anonymous namespaces, so they leave nothing behind on the machine, and are root-gated via internal/roottest like the other privileged tests. ActorVethTempName goes away with its last user. The two LinkNotFoundError checks in CleanupActorNetwork move from a bare type assertion to errors.AsType at the same time, so the rewritten cleanup does not match the same error type two different ways.
025b94b to
89d089f
Compare
421b7ac
into
agent-substrate:main
2 participants
SetupActorNetworkbuilt the veth pair in the worker pod netns with the peer under a temporary name, moved the peer into the actor netns with LinkSetNsFd, and renamed it to eth0 on the far side. The temporary name existed only because the pod netns has an eth0 of its own for the peer to collide with.Those two operations were 13.8ms of the 15.0ms of veth work: each drives a netdev unregister/register with an RCU grace period taken under the global RTNL lock. Neither parallelizes, so the fix is to not do them. Creating the pair with PeerName plus PeerNamespace has the peer born in the actor netns already called eth0, which costs neither: the name is resolved in the peer's namespace, so it never sees the pod's eth0.
Measured on the real function, 200 activations on a 6.8 kernel:
before:
SetupActorNetwork mean=18.07ms p50=17.83ms p99=26.81msafter:
SetupActorNetwork mean=2.96ms p50=2.69ms p99=5.72msThat is ~15ms off every actor resume, against a 100ms p95 activation target, since RestoreWorkload calls this before runsc restore. CleanupActorNetwork is unchanged at ~16ms; that cost is the veth delete and it lands on suspend.
The resulting namespace is identical, which is what the new tests assert: they pin the end state gVisor and the micro-VM guest read -- which links exist, in which namespace, with which addresses and routes -- rather than the sequence of netlink calls that produced it. They also cover repeated activation on a reused worker, cleanup idempotency, and the two micro-VM options (the fixed host veth MAC that keeps a restored guest's frozen ARP entry valid, and the interior link sweep). They run in throwaway anonymous namespaces, so they leave nothing behind on the machine, and are root-gated via internal/roottest like the other privileged tests.
ActorVethTempNamegoes away with its last user.