-
Notifications
You must be signed in to change notification settings - Fork 539
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 switch_id of fabric switch from config_db #3102
Conversation
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.
LGTM, @saksarav-nokia f.y.i
} | ||
else | ||
{ | ||
SWSS_LOG_ERROR("Fabric switch id is not 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.
Seems like its a breaking change. You are exiting orchagent if switch_id is not present in db. Is this intenational. @judyjoseph , @arlakshm
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.
@prsunny We have the same condition for linecard asics. In latest SAI, switch_id
was made mandatory on switch create of fabric asic opencomputeproject/SAI#1793. So, we need this change to pass the correct switch-id. The configuration is expected to have switch_id for both linecard asic and fabric asic.
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.
ok, please approve the PR
/Azp run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -579,6 +579,28 @@ int main(int argc, char **argv) | |||
attr.value.u32 = SAI_SWITCH_TYPE_FABRIC; | |||
attrs.push_back(attr); | |||
|
|||
//Read switch_id from config_db. | |||
Table cfgDeviceMetaDataTable(&config_db, CFG_DEVICE_METADATA_TABLE_NAME); |
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.
Can you check if VS tests are adopted for this change for Voq?
/Azwp run Azure.sonic-swss |
/Azpw run Azure.sonic-swss |
/Azp run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
@arlakshm , looks like its a genuine VS failure.. please check my comment |
99e732e
to
e52eea9
Compare
a9e746a
to
c25aa1d
Compare
Can you please fix coverage? |
c25aa1d
to
47dc4db
Compare
Hi @prsunny updated PR with unit test. |
/Azp run Azure.sonic-swss |
Commenter does not have sufficient privileges for PR 3102 in repo sonic-net/sonic-swss |
/azpw run Azure.sonic-swss |
/AzurePipelines run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
/Azp run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
/azpw run Azure.sonic-swss |
/AzurePipelines run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
4f62b62
to
066fc9a
Compare
3073b2f
to
94a9cba
Compare
This check will work even if process is auto-restarted after crash.
/azpw run Azure.sonic-swss |
/AzurePipelines run Azure.sonic-swss |
Azure Pipelines successfully started running 1 pipeline(s). |
What I did
Read switch_id of fabric switch from config_db
Why I did it
Fix sonic-net/sonic-buildimage#18558
How I verified it
Details if related