Load swbusd configuration from config_db #47
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| .status() | ||
| .unwrap(); | ||
| self.proc.wait().unwrap(); | ||
| // Command::new("kill") |
There was a problem hiding this comment.
is this some pending work item todo?
There was a problem hiding this comment.
ran some test but forgot to turn it back on.
| struct ConfigDBDPUEntry { | ||
| dpu_type: DpuType, | ||
| state: Option<String>, | ||
| slot_id: u8, |
There was a problem hiding this comment.
no need to restrict here, we can simply use u32 or i32
| const CONFIG_DB: &str = "CONFIG_DB"; | ||
| const SWBUSD_PORT: u16 = 51000; | ||
| #[derive(Debug, Clone, Deserialize, PartialEq, Eq)] | ||
| pub struct SwbusdConfig { |
There was a problem hiding this comment.
this file is not just route config, maybe split the file will be better.
| let file = File::open(yaml_file)?; | ||
| let reader = BufReader::new(file); | ||
|
|
||
| // Parse the YAML data | ||
| let routes_config: RoutesConfig = serde_yaml::from_reader(reader)?; | ||
| // let routes_config = RoutesConfig::from_routes_config_serde(&route_config_serde); | ||
| let routes_config: SwbusdConfig = serde_yaml::from_reader(reader)?; |
| Ok(routes_config) | ||
| } | ||
|
|
||
| pub fn load_from_configdb(slot_id: u8) -> Result<SwbusdConfig, Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
will be great to add instrument
| use swss_common::{DbConnector, Table}; | ||
| use swss_serde::from_table; | ||
| use tracing::*; | ||
| const CONFIG_DB: &str = "CONFIG_DB"; |
There was a problem hiding this comment.
will be better to move all the db loading logic into swbusd instead of core. in this way, it will be much easier for testing, because there is no swss and redis dependency at all.
| #[serde(rename_all = "lowercase")] | ||
| enum DpuType { | ||
| Local, | ||
| Cluster, |
There was a problem hiding this comment.
will be better to add some comments here
| } | ||
| impl RouteConfig { | ||
| #[instrument] | ||
| pub(crate) fn from_dpu_table( |
| impl RouteConfig { | ||
| #[instrument] | ||
| pub(crate) fn from_dpu_table( | ||
| dpu_table: &ConfigDBDPUEntry, |
There was a problem hiding this comment.
dpu_entry or something similar might be better
| dpu_table: &ConfigDBDPUEntry, | ||
| region: &str, | ||
| cluster: &str, | ||
| ) -> Result<Vec<RouteConfig>, String> { |
There was a problem hiding this comment.
will be better to actually use a real error.
The practice in rust is:
- the lib crate will have its own error defined
- outside lib, in exe, we can use anyhow or something similar.
if we can move these db loading code out into swbusd, then we can simply use anyhow, otherwise, it will be better to use thiserror crate and having explicit error defined.
| } | ||
|
|
||
| impl SwbusdConfig { | ||
| pub fn load_from_yaml(yaml_file: String) -> Result<SwbusdConfig, Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
Box dyn error is not a good practice. either we can use thiserror to define it, or move these code into the swbusd and use anyhow
| Ok(routes_config) | ||
| } | ||
|
|
||
| pub fn load_from_configdb(slot_id: u8) -> Result<SwbusdConfig, Box<dyn std::error::Error>> { | ||
| let db = DbConnector::new_named(CONFIG_DB, false, 0)?; |
There was a problem hiding this comment.
since there are multiple config db load here, it will be better to split this function into multiple ones. the code will be more readable.
why
We need to derive swbusd configuration from dpu table in config_db.
What this PR does
Otherwise, it will read configuration from the specified yaml file and bind to the specified address. The second option
is for development where we don't have full sonic environment.