-
Notifications
You must be signed in to change notification settings - Fork 22
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
Read parameters for vpp host interface creation from config file #1027
Conversation
3c987a2
to
f43ee92
Compare
0e43999
to
2a24fdd
Compare
Signed-off-by: Laszlo Kiraly <[email protected]>
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.
Conceptually looks good. Added a few comments mostly related to style
internal/vppinit/apiparams/const.go
Outdated
package apiparams | ||
|
||
const ( | ||
confFilename = "/var/lib/networkservicemesh/vppapi-hostint-args.yaml" |
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.
Is it possible to move this parameter to the NSM config?
I think it will be easier to see that vppinit can be configured
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.
Yes, eventually that was my initial approach. I can define this parameter as NSM config parameter and
1/ Pass it to init (Execute function) either changing argument list or using options pattern - each will change the function prototype and made reading/understanding the code harder. For example in main.go the call of this init function is complex already:
vppinit.Must(cfg.VppInit.Execute(ctx, vppConn, cfg.TunnelIP)),
Or
2/ Get the parameter value by os.Getenv()
where I want to use the path name in apiparams.go: getConfig(...)
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.
@glazychev-art Going forward with 2nd alternative, detailed in comment above.
- move to vppinit module Signed-off-by: Laszlo Kiraly <[email protected]>
- do not dump default configuration Signed-off-by: Laszlo Kiraly <[email protected]>
- rename the configuration struct Signed-off-by: Laszlo Kiraly <[email protected]>
- fix linter errors Signed-off-by: Laszlo Kiraly <[email protected]>
- read the path of the configuration file from an environment variable Signed-off-by: Laszlo Kiraly <[email protected]>
…d-forwarder-vpp@main PR link: networkservicemesh/cmd-forwarder-vpp#1027 Commit: 665dbbc Author: Laszlo Kiraly Date: 2024-03-20 11:16:26 +0100 Message: - Read parameters for vpp host interface creation from config file (#1027) * Parameters for vpp's host interface creation read from config file Signed-off-by: Laszlo Kiraly <[email protected]> * Parameters for vpp's host interface creation read from config file - move to vppinit module Signed-off-by: Laszlo Kiraly <[email protected]> * Parameters for vpp's host interface creation read from config file - do not dump default configuration Signed-off-by: Laszlo Kiraly <[email protected]> * Parameters for vpp's host interface creation read from config file - rename the configuration struct Signed-off-by: Laszlo Kiraly <[email protected]> * Parameters for vpp's host interface creation read from config file - fix linter errors Signed-off-by: Laszlo Kiraly <[email protected]> * Parameters for vpp's host interface creation read from config file - read the path of the configuration file from an environment variable Signed-off-by: Laszlo Kiraly <[email protected]> --------- Signed-off-by: NSMBot <[email protected]>
Improvement details
Parameters of vpp init functions for host interface creation should be exposed.
If a configuration file is present, the parameter values should be read from this config.
If there is no configuration file the current parameter values should be used.
The parameter values of the api functions should be written into the configuration file.
Notes
Parameters and default values:
Configuration file path:
/var/lib/networkservicemesh/vppapi-hostint-args.yaml