Skip to content

Make ovs-vswitchd service 'other_config' option configurable#58

Merged
e0ne merged 2 commits intoMellanox:network-operator-25.4.xfrom
e0ne:ovs-config
Apr 16, 2025
Merged

Make ovs-vswitchd service 'other_config' option configurable#58
e0ne merged 2 commits intoMellanox:network-operator-25.4.xfrom
e0ne:ovs-config

Conversation

@e0ne
Copy link
Collaborator

@e0ne e0ne commented Apr 14, 2025

This is a backport k8snetworkplumbingwg#823. We need this one for an internal release until community approves a PR

e0ne added 2 commits April 14, 2025 11:56
Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>
Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>
@github-actions
Copy link

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 e0ne changed the title Ovs config Make ovs-vswitchd service 'other_config' option configurable Apr 14, 2025
@e0ne e0ne requested a review from almaslennikov April 14, 2025 11:14
//RDMA subsystem. Allowed value "shared", "exclusive".
RdmaMode string `json:"rdmaMode,omitempty"`
// OVS config. It will be provided for ovs-vswitchd service as other_config option
// +kubebuilder:default:={hw-offload: "true"}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want such a default at the API level?
If I understand correctly, this means that if a user sets ovsConfig = {"foo":"bar"}, then the default will not apply.
This may be confusing. Should we instead always add {hw-offload: "true"} to the ovsConfig value when it doesn't contain an explicit value for this field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep default behaviour as is now. Always add {hw-offload: "true"} could be non-obvious for users, so I prefer user add this option if needed

@ykulazhenkov
Copy link

Overall, the code looks good. Are the CI failures expected?

@e0ne
Copy link
Collaborator Author

e0ne commented Apr 16, 2025

Overall, the code looks good. Are the CI failures expected?

I re-run test-coverage job - it should pass. Other failures are expected in this fork repo

@coveralls
Copy link

Pull Request Test Coverage Report for Build 14443961186

Details

  • 93 of 159 (58.49%) changed or added relevant lines in 9 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.05%) to 61.632%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/render/render.go 16 18 88.89%
pkg/plugins/k8s/k8s_plugin.go 16 29 55.17%
pkg/host/mock/mock_host.go 0 24 0.0%
pkg/host/internal/service/service.go 14 41 34.15%
Files with Coverage Reduction New Missed Lines %
pkg/plugins/k8s/k8s_plugin.go 2 76.11%
pkg/daemon/daemon.go 3 44.69%
Totals Coverage Status
Change from base Build 14392662641: 0.05%
Covered Lines: 8594
Relevant Lines: 13944

💛 - Coveralls

@e0ne e0ne merged commit fa04774 into Mellanox:network-operator-25.4.x Apr 16, 2025
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants