Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/v1/sriovnetworknodestate_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ type System struct {
// +kubebuilder:validation:Enum=shared;exclusive
//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"}
OvsConfig map[string]string `json:"ovsConfig,omitempty"`
}

// SriovNetworkNodeStateStatus defines the observed state of SriovNetworkNodeState
Expand Down
3 changes: 3 additions & 0 deletions api/v1/sriovnetworkpoolconfig_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ type OvsHardwareOffloadConfig struct {
// On OpenShift:
// Name is the name of MachineConfigPool to be enabled with OVS hardware offload
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"`

Choose a reason for hiding this comment

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

medium

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"`

}

// SriovNetworkPoolConfigStatus defines the observed state of SriovNetworkPoolConfig
Expand Down
20 changes: 17 additions & 3 deletions api/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ dropins:
- name: 10-hw-offload.conf
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 }}

Choose a reason for hiding this comment

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

critical

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

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

  2. 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.

  3. 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 }}

Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,14 @@ spec:
type: array
system:
properties:
ovsConfig:
additionalProperties:
type: string
default:
hw-offload: "true"
description: OVS config. It will be provided for ovs-vswitchd
service as other_config option
type: object
rdmaMode:
description: RDMA subsystem. Allowed value "shared", "exclusive".
enum:
Expand Down Expand Up @@ -368,6 +376,14 @@ spec:
type: string
system:
properties:
ovsConfig:
additionalProperties:
type: string
default:
hw-offload: "true"
description: OVS config. It will be provided for ovs-vswitchd
service as other_config option
type: object
rdmaMode:
description: RDMA subsystem. Allowed value "shared", "exclusive".
enum:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,14 @@ spec:
On OpenShift:
Name is the name of MachineConfigPool to be enabled with OVS hardware offload
type: string
otherConfig:
additionalProperties:
type: string
default:
hw-offload: "true"
description: OVS config. It will be provided for ovs-vswitchd
service as other_config option
type: object
type: object
rdmaMode:
description: RDMA subsystem. Allowed value "shared", "exclusive".
Expand Down
6 changes: 6 additions & 0 deletions controllers/sriovnetworknodepolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,12 @@ func (r *SriovNetworkNodePolicyReconciler) syncAllSriovNetworkNodeStates(ctx con
}
if netPoolConfig != nil {
ns.Spec.System.RdmaMode = netPoolConfig.Spec.RdmaMode
if netPoolConfig.Spec.OvsHardwareOffloadConfig.OvsConfig != nil {
ns.Spec.System.OvsConfig = make(map[string]string, len(netPoolConfig.Spec.OvsHardwareOffloadConfig.OvsConfig))
for k, v := range netPoolConfig.Spec.OvsHardwareOffloadConfig.OvsConfig {
ns.Spec.System.OvsConfig[k] = v
}
}
}
j, _ := json.Marshal(ns)
logger.V(2).Info("SriovNetworkNodeState CR", "content", j)
Expand Down
4 changes: 4 additions & 0 deletions controllers/sriovnetworkpoolconfig_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
constants "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/orchestrator"
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/render"
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/utils"
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars"
)

Expand Down Expand Up @@ -160,6 +161,9 @@ func (r *SriovNetworkPoolConfigReconciler) syncOvsHardwareOffloadMachineConfigs(
}

data := render.MakeRenderData()
externalIds, otherOvsConfig := utils.RenderOtherOvsConfigOption(nc.Spec.OvsHardwareOffloadConfig.OvsConfig)
data.Data["OtherOvsConfig"] = otherOvsConfig
data.Data["ExternalIds"] = externalIds
mc, err := render.GenerateMachineConfig("bindata/manifests/switchdev-config", mcName, mcpName, true, &data)
if err != nil {
return err
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,14 @@ spec:
type: array
system:
properties:
ovsConfig:
additionalProperties:
type: string
default:
hw-offload: "true"
description: OVS config. It will be provided for ovs-vswitchd
service as other_config option
type: object
rdmaMode:
description: RDMA subsystem. Allowed value "shared", "exclusive".
enum:
Expand Down Expand Up @@ -368,6 +376,14 @@ spec:
type: string
system:
properties:
ovsConfig:
additionalProperties:
type: string
default:
hw-offload: "true"
description: OVS config. It will be provided for ovs-vswitchd
service as other_config option
type: object
rdmaMode:
description: RDMA subsystem. Allowed value "shared", "exclusive".
enum:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,14 @@ spec:
On OpenShift:
Name is the name of MachineConfigPool to be enabled with OVS hardware offload
type: string
otherConfig:
additionalProperties:
type: string
default:
hw-offload: "true"
description: OVS config. It will be provided for ovs-vswitchd
service as other_config option
type: object
type: object
rdmaMode:
description: RDMA subsystem. Allowed value "shared", "exclusive".
Expand Down
12 changes: 11 additions & 1 deletion pkg/daemon/daemon_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ var _ = Describe("Daemon Controller", Ordered, 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()).Return(&hostTypes.Service{Name: "test"}, nil).AnyTimes()
hostHelper.EXPECT().ReadServiceInjectionManifestFile(gomock.Any(), gomock.Any()).Return(&hostTypes.Service{Name: "test"}, nil).AnyTimes()
}

platformMock.EXPECT().Init().Return(nil)
Expand Down Expand Up @@ -253,6 +253,16 @@ var _ = Describe("Daemon Controller", Ordered, func() {
}, waitTime, retryTime).Should(Succeed())
})

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()
}
})
})
Comment on lines +256 to +264

Choose a reason for hiding this comment

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

medium

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.


Context("Config Daemon generic flow", func() {
It("Should expose nodeState Status section", func(ctx context.Context) {
originalInterface := sriovnetworkv1.InterfaceExt{
Expand Down
36 changes: 32 additions & 4 deletions pkg/helper/mock/mock_helper.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 45 additions & 2 deletions pkg/host/internal/service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/host/types"
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/render"
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/utils"
)

Expand Down Expand Up @@ -105,6 +106,34 @@ func (s *service) EnableService(service *types.Service) error {
return err
}

// ReloadService reloads a systemd unit files on the host
func (s *service) ReloadService() error {
// Change root dir
exit, err := s.utilsHelper.Chroot(consts.Chroot)
if err != nil {
return err
}
defer exit()

// Restart the service
_, _, err = s.utilsHelper.RunCommand("systemctl", "daemon-reload")
return err
}

// RestartService restarts systemd service with systemctl restart
func (s *service) RestartService(service *types.Service) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add unit-test for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

// Change root dir
exit, err := s.utilsHelper.Chroot(consts.Chroot)
if err != nil {
return err
}
defer exit()

// Restart the service
_, _, err = s.utilsHelper.RunCommand("systemctl", "restart", service.Name)
return err
}

// CompareServices returns true if serviceA needs update(doesn't contain all fields from service B)
func (s *service) CompareServices(serviceA, serviceB *types.Service) (bool, error) {
optsA, err := unit.DeserializeOptions(strings.NewReader(serviceA.Content))
Expand All @@ -130,8 +159,12 @@ OUTER:
return false, nil
}

func (s *service) renderOtherOvsConfigOption(ovsConfig map[string]string) (string, string) {
return utils.RenderOtherOvsConfigOption(ovsConfig)
}

// ReadServiceInjectionManifestFile reads service injection file
func (s *service) ReadServiceInjectionManifestFile(path string) (*types.Service, error) {
func (s *service) ReadServiceInjectionManifestFile(path string, ovsConfig map[string]string) (*types.Service, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add unit-test for this functionality

data, err := os.ReadFile(path)
if err != nil {
return nil, err
Expand All @@ -142,10 +175,20 @@ func (s *service) ReadServiceInjectionManifestFile(path string) (*types.Service,
return nil, err
}

d := render.MakeRenderData()
externalIds, otherOvsConfig := s.renderOtherOvsConfigOption(ovsConfig)
d.Data["ExternalIds"] = externalIds
d.Data["OtherOvsConfig"] = otherOvsConfig

srv, err := render.RenderTemplate(serviceContent.Dropins[0].Contents, &d)
if err != nil {
return nil, err
}

return &types.Service{
Name: serviceContent.Name,
Path: systemdDir + serviceContent.Name,
Content: serviceContent.Dropins[0].Contents,
Content: srv.String(),
}, nil
}

Expand Down
21 changes: 21 additions & 0 deletions pkg/host/internal/service/service_suite_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package service

import (
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

"go.uber.org/zap/zapcore"
"sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
)

func TestSystemd(t *testing.T) {
log.SetLogger(zap.New(
zap.WriteTo(GinkgoWriter),
zap.Level(zapcore.Level(-2)),
zap.UseDevMode(true)))
RegisterFailHandler(Fail)
RunSpecs(t, "Package Service Suite")
}
Loading
Loading