add design doc for switchdev mode refactoring #559

ykulazhenkov · 2023-12-14T14:03:59Z

We need to refactor the implementation used for NICs in switchdev mode and align its behavior with the systemd
mode of the operator. The refactoring is required to simplify the development of the new switchdev-related
features for the sriov-network-operator.

github-actions · 2023-12-14T14:04:17Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

ykulazhenkov · 2023-12-14T14:05:31Z

@adrianchiris @SchSeba @e0ne please, take a look

coveralls · 2023-12-14T14:09:50Z

Pull Request Test Coverage Report for Build 7475965250

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.05%) to 23.306%

Totals
Change from base Build 7473924622:	0.05%
Covered Lines:	2298
Relevant Lines:	9860

💛 - Coveralls

adrianchiris

looking good @ykulazhenkov, added a few comments/questions.

doc/design/switchdev-refactoring.md

github-actions · 2024-01-02T09:08:00Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

adrianchiris

lgtm! great wok @ykulazhenkov

doc/design/switchdev-refactoring.md

zeeke · 2024-01-03T09:25:28Z

Just dropping an idea that could simplify here:

At the moment, the field SriovOperatorConfig.configurationMode defines whether the host configuration is applied by the config-daemon pod or the systemd service.

We can make the pod apply the configuration despite the field value, which only enables the systemd service. The service does the same logic as the pod, but it does that early in the boot process.
Then, when the config-daemon pod starts, it doesn't make any changes to the host because everything is already in place (unless there are bugs)

This could remove some if/then/else statements and simplify some workflows. It can be addressed and discussed in a different PR/design_doc if deemed out of scope.

ykulazhenkov · 2024-01-04T07:30:53Z

Just dropping an idea that could simplify here:

At the moment, the field SriovOperatorConfig.configurationMode defines whether the host configuration is applied by the config-daemon pod or the systemd service.

We can make the pod apply the configuration despite the field value, which only enables the systemd service. The service does the same logic as the pod, but it does that early in the boot process. Then, when the config-daemon pod starts, it doesn't make any changes to the host because everything is already in place (unless there are bugs)

This could remove some if/then/else statements and simplify some workflows. It can be addressed and discussed in a different PR/design_doc if deemed out of scope.

I like the idea. This means that configurationMode daemon will mean "daemon only" and configurationMode systemd means "daemon+systemd service for early configuration." I believe this can certainly simplify the code, and the main reason for this is that it looks like we will not need to propagate the result of the execution of the systemd service to the daemon (currently, this is required to update the status in the API) because the daemon will always recheck the configuration of the host.

This refactoring can be easily implemented on top of the changes that are proposed in this PR, but I prefer to discuss them separately to reduce the scope a bit.

e0ne · 2024-01-04T08:31:58Z

Just dropping an idea that could simplify here:

At the moment, the field SriovOperatorConfig.configurationMode defines whether the host configuration is applied by the config-daemon pod or the systemd service.

We can make the pod apply the configuration despite the field value, which only enables the systemd service. The service does the same logic as the pod, but it does that early in the boot process. Then, when the config-daemon pod starts, it doesn't make any changes to the host because everything is already in place (unless there are bugs)

This could remove some if/then/else statements and simplify some workflows. It can be addressed and discussed in a different PR/design_doc if deemed out of scope.

I like this idea. We can add some note in this design doc as a further improvement action item but this is out of scope of propesed idea

adrianchiris · 2024-01-04T08:54:46Z

+1 on having systemd service "assisting" config daemon, in case systemd failed we shoud take care to report that (e.g via event on srionetworknodestate) and allow config daemon to align configuration.

that said, i do not think this is related to switchdev refactoring directly and can be added on top.
@zeeke if you agree, mind opening a separate issue for this one ?

zeeke · 2024-01-04T10:12:33Z

Thanks for the quick feedback. I'll take care of starting a discussion on this

mlguerrero12 · 2024-01-04T13:49:50Z

@ykulazhenkov, I'm not familiar with the switchdev related code but I've seen that the k8s plugin has some config for that. This plugin is not mentioned in the proposal. Will this be affected? What's the impact?

ykulazhenkov · 2024-01-04T17:45:44Z

@ykulazhenkov, I'm not familiar with the switchdev related code but I've seen that the k8s plugin has some config for that. This plugin is not mentioned in the proposal. Will this be affected? What's the impact?

@mlguerrero12
k8s plugin is responsible for creation of bash based swithcdev systemd services
"Drop existing bash-based implementation which is used for NICs with switchdev configurationt" assumes that we will clean up this code + Openshift specific code in SriovNetworkPoolConfig controller which creates these services for Openshift

mlguerrero12 · 2024-01-05T10:38:37Z

cool, thanks for the clarification

SchSeba

I like the idea!
but one think we need to cover is when we apply a policy and we are in systemd mode we must reboot the host because the user may need to vfs on boot .
also with your solution it me be a bit slower as we will apply the config (create the vfs with the daemon) then reboot the node systemd will configure it again and then daemon will just pass because everything is in place

SchSeba · 2024-01-07T17:43:43Z

doc/design/switchdev-refactoring.md

+Both services run bash scripts. The script from the first service is responsible for VFs creation and for
+switching a NIC to the switchdev eSwitch mode. The script from the second service binds VFs to the required drivers.
+
+If a NIC has _**switchdev**_ configuration, then `configurationMode` of the operator does not affect it.


Hi think that is because the switchdev was only to support HWoffload right? we didn't had any other support in the operator because we don't have any CNI for it

Right. But now we want to propose support for additional use cases for NICs in switchdev mode(with separate discussion and design docs). That is why we want to align switched configs with configurationMode and systemd.

SchSeba · 2024-01-07T17:50:41Z

doc/design/switchdev-refactoring.md

+The operator should behave for these configurations the same way as it does now.
+
+Users using NICs with _**switchdev**_ configurations will need to explicitly set operator's 
+`configurationMode` to `systemd` if they expect the configuration of the NIC to happen


please add a comment here "to continue and support the hwoffload use-case"
because after we implement this design we should be able and configure switchdev even in daemon mode

@SchSeba can you elaborate what is hwoffload use-case ?

as i see it, for now its switching nic to switchdev and configuring sriov which after this enhancement can be done in either daemon or systemd mode,

for Openshift current use-case the latter will satisfy he requirement doesnt it ?

discussed in community meeting. my question is answered.

hwoffload == setting up systemd service to enable hw-offload config in ovs.

SchSeba · 2024-01-07T17:51:52Z

doc/design/switchdev-refactoring.md

+Change in the operator's behavior: `configurationMode` option now have effect
+on NICs with _**switchdev**_ configurations.
+
+### Implementation Details/Notes/Constraints


can we also consider to have a special controller to handle the case of HWoffload?
meaning switch the daemon to systemd mode?

can we also consider to have a special controller to handle the case of HWoffload? meaning switch the daemon to systemd mode?

Sorry, I didn't get the point. If you want to configure NIC to switchdev on boot, you should simply configure the sriov operator to systemd mode(by changing the OperatorConfig CR, the same way as it works now). The generic plugin will be extended to do the required actions for HW offload (tc offload enabling, vdpa device creation, etc)

as discussed in community meeting: its best to reduce complexity and have users explicitly setting the daemon to systemd.

users that use hwoffload (we dont know if/how-many there are) can:

scale down controller

update operator config to move to systemd mode

upgrade operator

scaleup opeator deployment

github-actions · 2024-01-08T08:00:35Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

github-actions · 2024-01-10T13:58:05Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

ykulazhenkov · 2024-01-10T13:59:43Z

while I was coding POC I discovered that we will need a minor change in the API - add vdpaType field to the interface status. I updated the design doc to reflect that this change is needed. @e0ne @adrianchiris please, re-review the PR

adrianchiris

LGTM

will re-approve the latest version

e0ne

LGTM

ykulazhenkov · 2024-01-16T08:58:40Z

@zeeke @SchSeba can we merge this one?

zeeke

LGTM

adrianchiris reviewed Dec 31, 2023

View reviewed changes

ykulazhenkov force-pushed the switchdev-refactoring-design branch from 1a27f69 to 0c6163a Compare January 2, 2024 09:07

ykulazhenkov requested a review from adrianchiris January 2, 2024 09:13

adrianchiris approved these changes Jan 2, 2024

View reviewed changes

zeeke reviewed Jan 3, 2024

View reviewed changes

doc/design/switchdev-refactoring.md Show resolved Hide resolved

e0ne previously approved these changes Jan 4, 2024

View reviewed changes

SchSeba reviewed Jan 7, 2024

View reviewed changes

ykulazhenkov force-pushed the switchdev-refactoring-design branch from 0c6163a to 0d22d52 Compare January 8, 2024 08:00

ykulazhenkov mentioned this pull request Jan 10, 2024

[switchdev 1/x] Add additional methods to host/kernel.go #582

Merged

add design doc for switchdev mode refactoring

6508c69

Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

ykulazhenkov force-pushed the switchdev-refactoring-design branch from 0d22d52 to 6508c69 Compare January 10, 2024 13:57

ykulazhenkov requested review from e0ne and adrianchiris January 10, 2024 13:59

This was referenced Jan 10, 2024

[switchdev 2/x] API: Add vdpaType field to the node SriovNetworkNodeState CRD #583

Merged

[switchdev 3/x] Add host.VdpaInterface whch provided helpers to work with VDPA devices #585

Closed

adrianchiris approved these changes Jan 15, 2024

View reviewed changes

e0ne approved these changes Jan 16, 2024

View reviewed changes

zeeke approved these changes Jan 16, 2024

View reviewed changes

zeeke merged commit 6e8a861 into k8snetworkplumbingwg:master Jan 16, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add design doc for switchdev mode refactoring #559

add design doc for switchdev mode refactoring #559

ykulazhenkov commented Dec 14, 2023

github-actions bot commented Dec 14, 2023

ykulazhenkov commented Dec 14, 2023

coveralls commented Dec 14, 2023 •

edited

Loading

adrianchiris left a comment

github-actions bot commented Jan 2, 2024

adrianchiris left a comment

zeeke commented Jan 3, 2024

ykulazhenkov commented Jan 4, 2024

e0ne commented Jan 4, 2024

adrianchiris commented Jan 4, 2024

zeeke commented Jan 4, 2024

mlguerrero12 commented Jan 4, 2024

ykulazhenkov commented Jan 4, 2024

mlguerrero12 commented Jan 5, 2024

SchSeba left a comment

SchSeba Jan 7, 2024

ykulazhenkov Jan 8, 2024

SchSeba Jan 7, 2024

ykulazhenkov Jan 8, 2024

adrianchiris Jan 8, 2024

adrianchiris Jan 9, 2024

SchSeba Jan 7, 2024

ykulazhenkov Jan 8, 2024

adrianchiris Jan 9, 2024 •

edited

Loading

github-actions bot commented Jan 8, 2024

github-actions bot commented Jan 10, 2024

ykulazhenkov commented Jan 10, 2024

adrianchiris left a comment

e0ne left a comment

ykulazhenkov commented Jan 16, 2024

zeeke left a comment

add design doc for switchdev mode refactoring #559

add design doc for switchdev mode refactoring #559

Conversation

ykulazhenkov commented Dec 14, 2023

github-actions bot commented Dec 14, 2023

ykulazhenkov commented Dec 14, 2023

coveralls commented Dec 14, 2023 • edited Loading

Pull Request Test Coverage Report for Build 7475965250

💛 - Coveralls

adrianchiris left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 2, 2024

adrianchiris left a comment

Choose a reason for hiding this comment

zeeke commented Jan 3, 2024

ykulazhenkov commented Jan 4, 2024

e0ne commented Jan 4, 2024

adrianchiris commented Jan 4, 2024

zeeke commented Jan 4, 2024

mlguerrero12 commented Jan 4, 2024

ykulazhenkov commented Jan 4, 2024

mlguerrero12 commented Jan 5, 2024

SchSeba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 8, 2024

github-actions bot commented Jan 10, 2024

ykulazhenkov commented Jan 10, 2024

adrianchiris left a comment

Choose a reason for hiding this comment

e0ne left a comment

Choose a reason for hiding this comment

ykulazhenkov commented Jan 16, 2024

zeeke left a comment

Choose a reason for hiding this comment

coveralls commented Dec 14, 2023 •

edited

Loading

adrianchiris Jan 9, 2024 •

edited

Loading