[vs] Initialization of VOQ switch objects #702
Conversation
Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com> VOQ switch objects are initialized during swich create. This VOQ objects are initialized after regular switch objects are initialized.
| @@ -659,6 +659,12 @@ sai_status_t VirtualSwitchSaiInterface::create( | |||
| return SAI_STATUS_FAILURE; | |||
| } | |||
|
|
|||
| // Initialize switch for VOQ attributes | |||
| if ((ss->initialize_voq_switch_objects(attr_count, attr_list)) != SAI_STATUS_SUCCESS) | |||
There was a problem hiding this comment.
this is in the wrong place, it should be inside SwitchStateBase.cpp class in initialize objects
There was a problem hiding this comment.
As discussed in PR #701, this is unchanged and going to be called from here as it is now.
There was a problem hiding this comment.
agree, but there are places init_default_objects inside _init_switch and warm_boot_initialize_objects,
current implementation you want to execute only for default objects, and this place here is actually executing it for both, which will cause to duplicate system port existence in virtual switch for warm boot,
so i would propose to add attr_count/list to init_switch and move this initialization in line 604
There was a problem hiding this comment.
also when you will want to support warm boot scenario for this, then you will need to add warm boot case in init_switch also
There was a problem hiding this comment.
For comment 1: o.k. I'll fix proposed.
For comment 2: o.k. when we support warm boot (only when system ports become dynamic config. Not sure when), this place will be used.
There was a problem hiding this comment.
Fixed. Moved initialize_voq_switch_objects() to init_switch() as proposed.
Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com> Code review comments fix 1
| @@ -602,6 +604,15 @@ std::shared_ptr<SwitchStateBase> VirtualSwitchSaiInterface::init_switch( | |||
| SWSS_LOG_THROW("unable to init switch %s", sai_serialize_status(status).c_str()); | |||
| } | |||
|
|
|||
| // Initialize switch for VOQ attributes | |||
|
|
|||
| status = ss->initialize_voq_switch_objects(attr_count, attr_list); | |||
There was a problem hiding this comment.
ok, this looks better, but i would like you to make 1 more change, so i think it would be actually better to propagate those count/list to parameters initialize_default_objects() function, and call voq initialize inside initialize_devault_objects, this way we will have initialization part in 1 single function, what do you think?
There was a problem hiding this comment.
No issue in doing what you are proposing. However, here the voq objects are not defaults. They are configured attributes. Not sure if it will be clearer to have non default object initialization in function whose name indicates default object initialization
There was a problem hiding this comment.
I also see a comment "TODO" to move the calling "initialize_default_objects()" to constructor. If we modify to propagate attr_count/attr_list to this function, we may not be able move this to constructor. Is it still o.k, if I do the modification as proposed?
There was a problem hiding this comment.
that move to constructor cant be moved to constructor since that is virtual function, it could be moved, but it would need to be called explicitly in each constructor, and not in SwitchStateBase
There was a problem hiding this comment.
o.k. I'll modify as proposed, then.
There was a problem hiding this comment.
thanks, this is actually interesting that some of the internal objects depends on the input attributes to create_switch function, and not actual hardware profile, this is wired to me
There was a problem hiding this comment.
Modified as proposed.
|
retest vs please |
|
lgtm is stuck, so you will need to do force push or add another commit to trigger lgtm rerun |
Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com> Code review comments 2
Signed-off-by: vedganes <vedavinayagam.ganesan@nokia.com> Code review comments fix 3
VOQ switch objects are initialized during switch create. This VOQ objects are initialized after regular switch objects are initialized. This PR depends on sonic-net#701 for switch state initialization
VOQ switch objects are initialized during swich create. This VOQ objects
are initialized after regular switch objects are initialized. This PR depends on #701
for switch state initialization