Make ovs-vswitchd service 'other_config' option configurable by e0ne · Pull Request #823 · k8snetworkplumbingwg/sriov-network-operator

e0ne · 2024-12-30T19:55:27Z

No description provided.

github-actions · 2024-12-30T19:55:37Z

Thanks for your PR,
To run vendors CIs, Maintainers can 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, Maintainers can 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.

coveralls · 2024-12-30T23:07:25Z

Pull Request Test Coverage Report for Build 22157252900

Details

118 of 163 (72.39%) changed or added relevant lines in 10 files are covered.
10 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.2%) to 63.089%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/render/render.go	16	18	88.89%
pkg/host/internal/service/service.go	30	36	83.33%
pkg/plugins/k8s/k8s_plugin.go	16	29	55.17%
pkg/host/mock/mock_host.go	0	24	0.0%

Files with Coverage Reduction	New Missed Lines	%
controllers/helper.go	2	69.7%
pkg/plugins/k8s/k8s_plugin.go	2	74.78%
controllers/drain_controller.go	3	77.17%
controllers/sriovnetworknodepolicy_controller.go	3	70.61%

Totals
Change from base Build 22144740276:	0.2%
Covered Lines:	9411
Relevant Lines:	14917

💛 - Coveralls

api/v1/sriovnetworknodestate_types.go

api/v1/sriovnetworkpoolconfig_types.go

adrianchiris · 2025-01-02T17:26:33Z

So, here are my thoughts on this one:

we should use same API as we have in OVSBridgeConfig e.g have OVSGlobalConfig in the pool config object
for now we can allow to set other_config keys only.

in addition we should support clearing any global configurations which were set but are not relevant anymore.
this can be done by checking the last applied status we cache or by saving which keys were set by the operator in a special key part of openvswitch table external_ids e.g external_ids:sriov-operator-owned-keys

the ExecPre we got for the systemd service of openvswitch would need to be a bit more complex to achieve that.

i wonder if we want a design doc for this one WDYT ?

github-actions · 2025-04-30T09:50:22Z

Thanks for your PR,
To run vendors CIs, Maintainers can 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, Maintainers can 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.

e0ne · 2025-07-21T09:35:20Z

/test-all-nvidia

e0ne · 2025-07-28T13:30:24Z

CI failures aren't related to this change

github-actions · 2025-10-16T13:12:26Z

Thanks for your PR,
To run vendors CIs, Maintainers can 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, Maintainers can 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.

github-actions · 2025-12-23T14:25:18Z

Thanks for your PR,
To run vendors CIs, Maintainers can 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, Maintainers can 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.

e0ne · 2026-01-20T12:09:04Z

@SchSeba could you please take a look on that?

Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>

SchSeba · 2026-02-23T13:18:27Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces configurability for the ovs-vswitchd service's other_config option. The changes span API types, CRDs, controllers, and host-level service management to apply these configurations. While the overall approach is sound, I've found a critical issue in the systemd service unit command that includes a typo and has flawed logic for cleaning up old configurations. I've also noted some areas for improvement regarding API consistency, test code structure, and plugin logic. Please see the detailed comments for suggestions.

gemini-code-assist · 2026-02-23T13:23:56Z

bindata/manifests/switchdev-config/ovs-units/ovs-vswitchd.service.yaml

  contents: |
    [Service]
-    ExecStartPre=/bin/ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true
+    ExecStartPre=/bin/ovs-vsctl -t 5 set Open_vSwitch . external_ids:sriov-operator-owned-keys='{{ .ExternalIds }}' && /bin/ovs-vsctl/ovs-vsctl -t 5 get Open_vSwitch . external_ids:sriov-operator-owned-keys | xargs /bin/ovs-vsctl --no-wait remove Open_vSwitch . other_config && /bin/ovs-vsctl --no-wait set Open_vSwitch . {{ .OtherOvsConfig }}


The ExecStartPre command has a critical typo and flawed logic for managing other_config.

Critical Typo: The path /bin/ovs-vsctl/ovs-vsctl is incorrect and will cause the command to fail. It should be /bin/ovs-vsctl.

Flawed Cleanup Logic: The command attempts to clean up old other_config entries. However, it first updates external_ids:sriov-operator-owned-keys with the new set of keys, then reads them back to perform the cleanup. This means if a key is removed from the configuration, it will not be in the list of keys to remove and will remain stale in other_config.

Incorrect xargs Usage: ovs-vsctl get returns a single quoted string (e.g., "key1 key2"). xargs will pass this as a single argument to ovs-vsctl remove, which will fail because it expects separate keys.

To fix this, you should first get the old keys, then remove them, then set the new keys. A simple fix for the typo is provided, but the logic should be revisited for robustness.

ExecStartPre=/bin/ovs-vsctl -t 5 set Open_vSwitch . external_ids:sriov-operator-owned-keys='{{ .ExternalIds }}' && /bin/ovs-vsctl -t 5 get Open_vSwitch . external_ids:sriov-operator-owned-keys | xargs /bin/ovs-vsctl --no-wait remove Open_vSwitch . other_config && /bin/ovs-vsctl --no-wait set Open_vSwitch . {{ .OtherOvsConfig }}

gemini-code-assist · 2026-02-23T13:23:56Z

api/v1/sriovnetworkpoolconfig_types.go

 	Name string `json:"name,omitempty"`
+	// OVS config. It will be provided for ovs-vswitchd service as other_config option
+	// +kubebuilder:default:={hw-offload: "true"}
+	OvsConfig map[string]string `json:"otherConfig,omitempty"`


The JSON tag otherConfig for the OvsConfig field is inconsistent. In sriovnetworknodestate_types.go, the OvsConfig field uses the ovsConfig JSON tag. For consistency across the API, it would be better to use ovsConfig here as well.

Suggested change

OvsConfig map[string]string `json:"otherConfig,omitempty"`

OvsConfig map[string]string `json:"ovsConfig,omitempty"`

gemini-code-assist · 2026-02-23T13:23:56Z

pkg/daemon/daemon_test.go

+	Context("Config Daemon generic flow", func() {
+		BeforeEach(func() {
+			// k8s plugin for k8s cluster type
+			if vars.ClusterType == constants.ClusterTypeKubernetes {
+				hostHelper.EXPECT().ReadServiceManifestFile(gomock.Any()).Return(&hostTypes.Service{Name: "test"}, nil).AnyTimes()
+				hostHelper.EXPECT().ReadServiceInjectionManifestFile(gomock.Any(), gomock.Any()).Return(&hostTypes.Service{Name: "test"}, nil).AnyTimes()
+			}
+		})
+	})


There appears to be a duplicate Context("Config Daemon generic flow", ...) block. The BeforeEach in this first block should likely be merged into the subsequent Context block to avoid confusion and keep related test setup together.

gemini-code-assist · 2026-02-23T13:23:56Z

pkg/plugins/k8s/k8s_plugin.go

 	if p.isOVSHwOffloadingEnabled() {
 		p.updateTarget.openVSwitch.SetNeedUpdate()
-	} else {
-		p.updateTarget.openVSwitch.SetNeedReboot()
 	}


The logic if p.isOVSHwOffloadingEnabled() { p.updateTarget.openVSwitch.SetNeedUpdate() } seems redundant. The preceding if/else block already sets NeedUpdate if the systemd service requires an update. The isSystemDServiceNeedUpdate check should be sufficient to determine if the service file needs to be updated. This additional check is confusing because it marks the service for an update even if it's already enabled, which is likely not the intention. Consider removing this block for clarity.

e0ne requested review from SchSeba, adrianchiris and ykulazhenkov December 30, 2024 19:55

e0ne force-pushed the ovs-config branch 3 times, most recently from 2dc7153 to bc320cd Compare December 30, 2024 23:00

ykulazhenkov reviewed Dec 31, 2024

View reviewed changes

api/v1/sriovnetworknodestate_types.go Outdated Show resolved Hide resolved

api/v1/sriovnetworkpoolconfig_types.go Outdated Show resolved Hide resolved

e0ne force-pushed the ovs-config branch from bc320cd to 7fe2a0e Compare December 31, 2024 12:32

e0ne force-pushed the ovs-config branch 4 times, most recently from a26b54a to 027353b Compare January 13, 2025 19:20

e0ne requested a review from ykulazhenkov January 13, 2025 19:20

e0ne force-pushed the ovs-config branch 4 times, most recently from c451160 to 13d1f90 Compare March 6, 2025 12:32

e0ne force-pushed the ovs-config branch from 13d1f90 to 4676995 Compare March 24, 2025 12:22

e0ne force-pushed the ovs-config branch 9 times, most recently from 5f41adf to 9d3add7 Compare April 2, 2025 15:39

e0ne closed this Apr 30, 2025

e0ne reopened this Apr 30, 2025

e0ne force-pushed the ovs-config branch 2 times, most recently from f64a6b8 to f73543a Compare May 5, 2025 08:51

e0ne added the hold label May 13, 2025

e0ne force-pushed the ovs-config branch from f73543a to 846f9d4 Compare July 14, 2025 09:15

e0ne removed the hold label Jul 14, 2025

e0ne force-pushed the ovs-config branch 3 times, most recently from 466ee14 to dd06c5b Compare July 17, 2025 09:39

e0ne closed this Oct 16, 2025

e0ne reopened this Oct 16, 2025

github-actions bot added the tests label Oct 16, 2025

e0ne force-pushed the ovs-config branch 3 times, most recently from 1a25f3d to 4c7f92e Compare November 21, 2025 14:19

e0ne requested a review from SchSeba December 16, 2025 10:20

e0ne closed this Dec 23, 2025

e0ne reopened this Dec 23, 2025

e0ne added 2 commits February 18, 2026 22:54

Add support to render template from string

418ab3a

Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>

Make ovs-vswitchd service 'other_config' option configurable

1477097

Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>

e0ne force-pushed the ovs-config branch from 4c7f92e to 1477097 Compare February 18, 2026 20:55

gemini-code-assist bot reviewed Feb 23, 2026

View reviewed changes

	OvsConfig map[string]string `json:"otherConfig,omitempty"`
	OvsConfig map[string]string `json:"ovsConfig,omitempty"`

Comments

Conversation

e0ne commented Dec 30, 2024

Uh oh!

github-actions bot commented Dec 30, 2024

Uh oh!

coveralls commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 22157252900

Details

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

adrianchiris commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 30, 2025

Uh oh!

e0ne commented Jul 21, 2025

Uh oh!

e0ne commented Jul 28, 2025

Uh oh!

github-actions bot commented Oct 16, 2025

Uh oh!

github-actions bot commented Dec 23, 2025

Uh oh!

e0ne commented Jan 20, 2026

Uh oh!

SchSeba commented Feb 23, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Dec 30, 2024 •

edited

Loading

adrianchiris commented Jan 2, 2025 •

edited

Loading