BMP Module initial implementation #1130
Conversation
Signed-off-by: Sergey Matov <[email protected]>
Signed-off-by: Sergey Matov <[email protected]>
Signed-off-by: Sergey Matov <[email protected]>
Signed-off-by: Sergey Matov <[email protected]>
Signed-off-by: Sergey Matov <[email protected]>
Signed-off-by: Sergey Matov <[email protected]>
sergeymatov
requested review from
Fredi-raspall
and removed request for
a team
sergeymatov
force-pushed
the
pr/smatov/bmp-frr-config
branch
from
43e8d27 to
6044efa
Compare
config/src/internal/routing/bgp.rs
Outdated
| } | ||
|
|
||
| #[derive(Clone, Debug)] | ||
| pub enum BmpSource { |
There was a problem hiding this comment.
I'd define this in another file (bmp.rs), since the bgp.rs is becoming large and bmp is a clear cut?
(I mean the whole bmp config). In bgp.rs you would just import the BmpOptions.
| cfg += MARKER; | ||
| cfg += "address-family ipv4 unicast"; | ||
|
|
||
| /* activate neighbors in AF */ |
There was a problem hiding this comment.
Why do you remove the comments?
dataplane/src/main.rs
Outdated
| init_logging(); | ||
| let args = CmdArgs::parse(); | ||
|
|
||
| let (bmp_server_params, bmp_client_opts) = if args.bmp_enabled() { |
There was a problem hiding this comment.
You could probably put this in a function in main.rs that would return the tuple so that main() would remain small (that will help dealing with conflicts).
| CheckBytes, | ||
| )] | ||
| #[rkyv(attr(derive(PartialEq, Eq, Debug)))] | ||
| pub struct BmpConfigSection { |
There was a problem hiding this comment.
Here I have a question, that we should clarify ASAP.
For mgmt server (gRPC), we pass the ip/port & Co. from the cmd line. We have no other option probably. However, for BMP, is this what we want to do? I mean, are we sure we don't want to do that via grpc/k8s? In the latter case, the bmp server would be configured and started from the mgmt module and not by main, on demand, depending on the gRPC/k8s configuration.
@Frostman, @mvachhar any thoughts on that?
I know it may seem to be more work, but it may actually turn out to reduce and simplify the code and provide the capability to enable/disable bmp if needed at runtime.
|
|
||
| /// Spawn BMP server in a dedicated thread with its own Tokio runtime. | ||
| /// Always uses `StatusUpdateHandler` to update `DataplaneStatus`. | ||
| pub fn spawn_background( |
There was a problem hiding this comment.
Can this be renamed to be more meaningful?
| } | ||
| } | ||
|
|
||
| impl Render for BmpOptions { |
There was a problem hiding this comment.
you could also put this in a separate bmp.rs. But it's fine.
At the end of the file, there's a test for bgp that you could extend with bmp options.
It's not really a test, but a way to visually validate that the config is properly rendered.
Fredi-raspall
left a comment
Fredi-raspall
left a comment
There was a problem hiding this comment.
I like the approach and implementation!
I am going to request changes because I'd like to review this again once it is no longer a draft and some of the questions get clarified.
Thanks!
sergeymatov
force-pushed
the
pr/smatov/bmp-frr-config
branch
from
6044efa to
66e7168
Compare
Signed-off-by: Sergey Matov <[email protected]>
Signed-off-by: Sergey Matov <[email protected]>
Signed-off-by: Sergey Matov <[email protected]>
Signed-off-by: Sergey Matov <[email protected]>
Signed-off-by: Sergey Matov <[email protected]>
sergeymatov
force-pushed
the
pr/smatov/bmp-frr-config
branch
from
66e7168 to
613b990
Compare
Signed-off-by: Sergey Matov <[email protected]>
sergeymatov
force-pushed
the
pr/smatov/bmp-frr-config
branch
from
613b990 to
4ce4148
Compare
Signed-off-by: Sergey Matov <[email protected]>
sergeymatov
force-pushed
the
pr/smatov/bmp-frr-config
branch
from
4ce4148 to
eeea2b2
Compare
this is WIP, desc will be added later