Skip to content

Commit 222674b

Browse files
committed
Fix review comments
Signed-off-by: Sebastian Sch <[email protected]>
1 parent b7e600a commit 222674b

File tree

28 files changed

+327
-197
lines changed

28 files changed

+327
-197
lines changed

cmd/sriov-network-config-daemon/service.go

Lines changed: 26 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -77,25 +77,28 @@ func newServiceConfig(setupLog logr.Logger) (*ServiceConfig, error) {
7777
if err != nil {
7878
return nil, fmt.Errorf("failed to create host helpers: %v", err)
7979
}
80-
sc := &ServiceConfig{
81-
hostHelpers,
82-
nil,
83-
setupLog,
84-
nil,
85-
}
8680

87-
err = sc.readConf()
81+
// Read config first to get platform type
82+
83+
sriovConfig, err := readConf(hostHelpers, setupLog)
8884
if err != nil {
89-
return nil, sc.updateSriovResultErr(phaseArg, err)
85+
return nil, fmt.Errorf("failed to read configuration: %w", err)
9086
}
9187

9288
// init globals
93-
vars.PlatformType = sc.sriovConfig.PlatformType
94-
platformInterface, err := newPlatformFunc(hostHelpers)
89+
vars.PlatformType = sriovConfig.PlatformType
90+
platformInterface, err := newPlatformFunc(vars.PlatformType, hostHelpers)
9591
if err != nil {
96-
return nil, fmt.Errorf("failed to creeate serviceConfig: %w", err)
92+
return nil, fmt.Errorf("failed to create serviceConfig: %w", err)
93+
}
94+
95+
// Create final ServiceConfig with all fields initialized
96+
sc := &ServiceConfig{
97+
hostHelper: hostHelpers,
98+
platformInterface: platformInterface,
99+
log: setupLog,
100+
sriovConfig: sriovConfig,
97101
}
98-
sc.platformInterface = platformInterface
99102

100103
return sc, nil
101104
}
@@ -159,21 +162,20 @@ func runServiceCmd(cmd *cobra.Command, args []string) error {
159162
return sc.updateSriovResultOk(phaseArg)
160163
}
161164

162-
func (s *ServiceConfig) readConf() error {
163-
nodeStateSpec, err := s.hostHelper.ReadConfFile()
165+
func readConf(hostHelper helper.HostHelpersInterface, log logr.Logger) (*hosttypes.SriovConfig, error) {
166+
nodeStateSpec, err := hostHelper.ReadConfFile()
164167
if err != nil {
165168
if _, err := os.Stat(utils.GetHostExtensionPath(consts.SriovSystemdConfigPath)); !errors.Is(err, os.ErrNotExist) {
166-
return fmt.Errorf("failed to read the sriov configuration file in path %s: %v", utils.GetHostExtensionPath(consts.SriovSystemdConfigPath), err)
169+
return nil, fmt.Errorf("failed to read the sriov configuration file in path %s: %v", utils.GetHostExtensionPath(consts.SriovSystemdConfigPath), err)
167170
}
168-
s.log.Info("configuration file not found, use default config")
171+
log.Info("configuration file not found, use default config")
169172
nodeStateSpec = &hosttypes.SriovConfig{
170173
Spec: sriovv1.SriovNetworkNodeStateSpec{},
171174
UnsupportedNics: false,
172175
PlatformType: consts.Baremetal,
173176
}
174177
}
175-
s.sriovConfig = nodeStateSpec
176-
return nil
178+
return nodeStateSpec, nil
177179
}
178180

179181
func (s *ServiceConfig) initSupportedNics() error {
@@ -244,7 +246,7 @@ func (s *ServiceConfig) callPlugin(phase string) error {
244246
}
245247

246248
func (s *ServiceConfig) getPlugin(phase string) (plugin.VendorPlugin, error) {
247-
return s.platformInterface.SystemdGetPlugin(phase)
249+
return s.platformInterface.SystemdGetVendorPlugin(phase)
248250
}
249251

250252
func (s *ServiceConfig) getNetworkNodeState(phase string) (*sriovv1.SriovNetworkNodeState, error) {
@@ -259,9 +261,11 @@ func (s *ServiceConfig) getNetworkNodeState(phase string) (*sriovv1.SriovNetwork
259261
}
260262
if phase != consts.PhasePre && vars.ManageSoftwareBridges {
261263
// openvswitch is not available during the pre-phase
262-
bridges, err = s.platformInterface.DiscoverBridges()
263-
if err != nil {
264-
return nil, fmt.Errorf("failed to discover managed bridges on the host: %v", err)
264+
if vars.ManageSoftwareBridges {
265+
bridges, err = s.platformInterface.DiscoverBridges()
266+
if err != nil {
267+
return nil, fmt.Errorf("failed to discover managed bridges on the host: %v", err)
268+
}
265269
}
266270
}
267271

cmd/sriov-network-config-daemon/service_test.go

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ var _ = Describe("Service", func() {
124124
genericPlugin = plugins_mock.NewMockVendorPlugin(testCtrl)
125125
virtualPlugin = plugins_mock.NewMockVendorPlugin(testCtrl)
126126

127-
newPlatformFunc = func(hostHelpers helper.HostHelpersInterface) (platform.Interface, error) {
127+
newPlatformFunc = func(platformType consts.PlatformTypes, hostHelpers helper.HostHelpersInterface) (platform.Interface, error) {
128128
return platformMock, nil
129129
}
130130
newHostHelpersFunc = func() (helper.HostHelpersInterface, error) {
@@ -146,15 +146,15 @@ var _ = Describe("Service", func() {
146146
hostHelpers.EXPECT().CheckRDMAEnabled().Return(true, nil)
147147
hostHelpers.EXPECT().TryEnableTun().Return()
148148
hostHelpers.EXPECT().TryEnableVhostNet().Return()
149-
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(0), nil)
149+
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(consts.Baremetal), nil)
150150
hostHelpers.EXPECT().ReadSriovSupportedNics().Return(testSriovSupportedNicIDs, nil)
151151
hostHelpers.EXPECT().RemoveSriovResult().Return(nil)
152152
hostHelpers.EXPECT().WriteSriovResult(&hosttypes.SriovResult{SyncStatus: consts.SyncStatusInProgress})
153153

154154
genericPlugin.EXPECT().OnNodeStateChange(newNodeStateContainsDeviceMatcher("enp216s0f0np0")).Return(true, false, nil)
155155
genericPlugin.EXPECT().Apply().Return(nil)
156156

157-
platformMock.EXPECT().SystemdGetPlugin(phaseArg).Return(genericPlugin, nil)
157+
platformMock.EXPECT().SystemdGetVendorPlugin(phaseArg).Return(genericPlugin, nil)
158158
platformMock.EXPECT().DiscoverSriovDevices().Return([]sriovnetworkv1.InterfaceExt{{
159159
Name: "enp216s0f0np0",
160160
}}, nil)
@@ -168,12 +168,12 @@ var _ = Describe("Service", func() {
168168
hostHelpers.EXPECT().CheckRDMAEnabled().Return(true, nil)
169169
hostHelpers.EXPECT().TryEnableTun().Return()
170170
hostHelpers.EXPECT().TryEnableVhostNet().Return()
171-
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(1), nil)
171+
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(consts.VirtualOpenStack), nil)
172172
hostHelpers.EXPECT().ReadSriovSupportedNics().Return(testSriovSupportedNicIDs, nil)
173173
hostHelpers.EXPECT().RemoveSriovResult().Return(nil)
174174
hostHelpers.EXPECT().WriteSriovResult(&hosttypes.SriovResult{SyncStatus: consts.SyncStatusInProgress})
175175

176-
platformMock.EXPECT().SystemdGetPlugin(phaseArg).Return(virtualPlugin, nil)
176+
platformMock.EXPECT().SystemdGetVendorPlugin(phaseArg).Return(virtualPlugin, nil)
177177
platformMock.EXPECT().DiscoverSriovDevices().Return([]sriovnetworkv1.InterfaceExt{{
178178
Name: "enp216s0f0np0",
179179
}}, nil)
@@ -192,15 +192,15 @@ var _ = Describe("Service", func() {
192192
hostHelpers.EXPECT().CheckRDMAEnabled().Return(true, nil)
193193
hostHelpers.EXPECT().TryEnableTun().Return()
194194
hostHelpers.EXPECT().TryEnableVhostNet().Return()
195-
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(0), nil)
195+
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(consts.Baremetal), nil)
196196
hostHelpers.EXPECT().ReadSriovSupportedNics().Return(testSriovSupportedNicIDs, nil)
197197
hostHelpers.EXPECT().RemoveSriovResult().Return(nil)
198198
hostHelpers.EXPECT().WriteSriovResult(&hosttypes.SriovResult{SyncStatus: consts.SyncStatusFailed, LastSyncError: "pre: failed to apply configuration: test"})
199199

200200
genericPlugin.EXPECT().OnNodeStateChange(newNodeStateContainsDeviceMatcher("enp216s0f0np0")).Return(true, false, nil).AnyTimes()
201201
genericPlugin.EXPECT().Apply().Return(testError)
202202

203-
platformMock.EXPECT().SystemdGetPlugin(phaseArg).Return(genericPlugin, nil)
203+
platformMock.EXPECT().SystemdGetVendorPlugin(phaseArg).Return(genericPlugin, nil)
204204
platformMock.EXPECT().DiscoverSriovDevices().Return([]sriovnetworkv1.InterfaceExt{{
205205
Name: "enp216s0f0np0",
206206
}}, nil)
@@ -214,14 +214,14 @@ var _ = Describe("Service", func() {
214214
hostHelpers.EXPECT().TryGetInterfaceName("0000:d8:00.0").Return("enp216s0f0np0")
215215
hostHelpers.EXPECT().WaitUdevEventsProcessed(60).Return(nil)
216216
hostHelpers.EXPECT().ReadSriovResult().Return(getTestResultFileContent("InProgress", ""), nil)
217-
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(0), nil)
217+
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(consts.Baremetal), nil)
218218
hostHelpers.EXPECT().ReadSriovSupportedNics().Return(testSriovSupportedNicIDs, nil)
219219
hostHelpers.EXPECT().WriteSriovResult(&hosttypes.SriovResult{SyncStatus: consts.SyncStatusSucceeded})
220220

221221
genericPlugin.EXPECT().OnNodeStateChange(newNodeStateContainsDeviceMatcher("enp216s0f0np0")).Return(true, false, nil)
222222
genericPlugin.EXPECT().Apply().Return(nil)
223223

224-
platformMock.EXPECT().SystemdGetPlugin(phaseArg).Return(genericPlugin, nil)
224+
platformMock.EXPECT().SystemdGetVendorPlugin(phaseArg).Return(genericPlugin, nil)
225225
platformMock.EXPECT().DiscoverSriovDevices().Return([]sriovnetworkv1.InterfaceExt{{
226226
Name: "enp216s0f0np0",
227227
}}, nil)
@@ -233,15 +233,15 @@ var _ = Describe("Service", func() {
233233

234234
It("Post phase - virtual cluster", func() {
235235
phaseArg = consts.PhasePost
236-
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(1), nil)
236+
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(consts.VirtualOpenStack), nil)
237237
hostHelpers.EXPECT().ReadSriovSupportedNics().Return(testSriovSupportedNicIDs, nil)
238238
hostHelpers.EXPECT().ReadSriovResult().Return(getTestResultFileContent("InProgress", ""), nil)
239239
hostHelpers.EXPECT().WriteSriovResult(&hosttypes.SriovResult{SyncStatus: consts.SyncStatusSucceeded})
240240

241241
virtualPlugin.EXPECT().OnNodeStateChange(newNodeStateContainsDeviceMatcher("enp216s0f0np0")).Return(true, false, nil)
242242
virtualPlugin.EXPECT().Apply().Return(nil)
243243

244-
platformMock.EXPECT().SystemdGetPlugin(phaseArg).Return(virtualPlugin, nil)
244+
platformMock.EXPECT().SystemdGetVendorPlugin(phaseArg).Return(virtualPlugin, nil)
245245
platformMock.EXPECT().DiscoverSriovDevices().Return([]sriovnetworkv1.InterfaceExt{{
246246
Name: "enp216s0f0np0",
247247
}}, nil)
@@ -253,25 +253,26 @@ var _ = Describe("Service", func() {
253253

254254
It("Post phase - wrong result of the pre phase", func() {
255255
phaseArg = consts.PhasePost
256-
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(1), nil)
256+
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(consts.VirtualOpenStack), nil)
257257
hostHelpers.EXPECT().ReadSriovSupportedNics().Return(testSriovSupportedNicIDs, nil)
258258
hostHelpers.EXPECT().ReadSriovResult().Return(getTestResultFileContent("Failed", "pretest"), nil)
259259
hostHelpers.EXPECT().WriteSriovResult(&hosttypes.SriovResult{SyncStatus: consts.SyncStatusFailed, LastSyncError: "post: unexpected result of the pre phase: Failed, syncError: pretest"})
260260

261261
Expect(runServiceCmd(&cobra.Command{}, []string{})).To(HaveOccurred())
262262
})
263263
It("waitForDevicesInitialization", func() {
264-
cfg := &hosttypes.SriovConfig{Spec: sriovnetworkv1.SriovNetworkNodeStateSpec{
265-
Interfaces: []sriovnetworkv1.Interface{
266-
{Name: "name1", PciAddress: "0000:d8:00.0"},
267-
{Name: "name2", PciAddress: "0000:d8:00.1"}}}}
264+
cfg := getTestSriovInterfaceConfig(consts.Baremetal)
265+
cfg.Spec.Interfaces = []sriovnetworkv1.Interface{
266+
{Name: "name1", PciAddress: "0000:d8:00.0"},
267+
{Name: "name2", PciAddress: "0000:d8:00.1"},
268+
}
268269
hostHelpers.EXPECT().TryGetInterfaceName("0000:d8:00.0").Return("other")
269270
hostHelpers.EXPECT().TryGetInterfaceName("0000:d8:00.1").Return("")
270271
hostHelpers.EXPECT().TryGetInterfaceName("0000:d8:00.0").Return("name1")
271272
hostHelpers.EXPECT().TryGetInterfaceName("0000:d8:00.1").Return("")
272273
hostHelpers.EXPECT().TryGetInterfaceName("0000:d8:00.1").Return("name2")
273274
hostHelpers.EXPECT().WaitUdevEventsProcessed(60).Return(nil)
274-
hostHelpers.EXPECT().ReadConfFile().Return(getTestSriovInterfaceConfig(0), nil)
275+
hostHelpers.EXPECT().ReadConfFile().Return(cfg, nil)
275276
sc, err := newServiceConfig(logr.Discard())
276277
Expect(err).ToNot(HaveOccurred())
277278
sc.sriovConfig = cfg

cmd/sriov-network-config-daemon/start.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ func runStartCmd(cmd *cobra.Command, args []string) error {
250250
vars.PlatformType = pType
251251
}
252252
}
253-
setupLog.Info("Running on", "platform", vars.PlatformType.String())
253+
setupLog.Info("Running on", "platform", vars.PlatformType)
254254

255255
// create helpers
256256
hostHelpers, err := helper.NewDefaultHostHelpers()
@@ -259,7 +259,7 @@ func runStartCmd(cmd *cobra.Command, args []string) error {
259259
return err
260260
}
261261

262-
plat, err := platform.New(hostHelpers)
262+
plat, err := platform.New(vars.PlatformType, hostHelpers)
263263
if err != nil {
264264
setupLog.Error(err, "failed to create hypervisor")
265265
return err

cmd/sriov-network-operator-config-cleanup/cleanup_test.go

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -147,12 +147,7 @@ func newConfigController() *configController {
147147
}).AnyTimes()
148148

149149
// TODO: Change this to add tests for hypershift
150-
orchestrator.EXPECT().Flavor().DoAndReturn(func() consts.ClusterFlavor {
151-
if vars.ClusterType == consts.ClusterTypeOpenshift {
152-
return consts.DefaultConfigName
153-
}
154-
return consts.ClusterFlavorVanillaK8s
155-
}).AnyTimes()
150+
orchestrator.EXPECT().Flavor().Return(consts.ClusterFlavorDefault).AnyTimes()
156151

157152
err = (&controllers.SriovOperatorConfigReconciler{
158153
Client: k8sManager.GetClient(),

controllers/sriovnetworkpoolconfig_controller_test.go

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -53,12 +53,7 @@ var _ = Describe("SriovNetworkPoolConfig controller", Ordered, func() {
5353
}).AnyTimes()
5454

5555
// TODO: Change this to add tests for hypershift
56-
orchestrator.EXPECT().Flavor().DoAndReturn(func() consts.ClusterFlavor {
57-
if vars.ClusterType == consts.ClusterTypeOpenshift {
58-
return consts.DefaultConfigName
59-
}
60-
return consts.ClusterFlavorVanillaK8s
61-
}).AnyTimes()
56+
orchestrator.EXPECT().Flavor().Return(consts.ClusterFlavorDefault).AnyTimes()
6257

6358
err = (&SriovNetworkPoolConfigReconciler{
6459
Client: k8sManager.GetClient(),

controllers/sriovoperatorconfig_controller_test.go

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,7 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
7070
}).AnyTimes()
7171

7272
// TODO: Change this to add tests for hypershift
73-
orchestrator.EXPECT().Flavor().DoAndReturn(func() consts.ClusterFlavor {
74-
if vars.ClusterType == consts.ClusterTypeOpenshift {
75-
return consts.DefaultConfigName
76-
}
77-
return consts.ClusterFlavorVanillaK8s
78-
}).AnyTimes()
73+
orchestrator.EXPECT().Flavor().Return(consts.ClusterFlavorDefault).AnyTimes()
7974

8075
err = (&SriovOperatorConfigReconciler{
8176
Client: k8sManager.GetClient(),

main.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ func main() {
151151
vars.Config = restConfig
152152
vars.Scheme = mgrGlobal.GetScheme()
153153

154-
orch, err := orchestrator.New()
154+
orch, err := orchestrator.New(vars.ClusterType)
155155
if err != nil {
156156
setupLog.Error(err, "couldn't create orchestrator")
157157
os.Exit(1)

pkg/consts/platforms.go

Lines changed: 14 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,39 +1,31 @@
11
package consts
22

3-
import "fmt"
4-
3+
// ClusterType represents the type of Kubernetes cluster (e.g., OpenShift, vanilla Kubernetes)
54
type ClusterType string
65

76
const (
8-
ClusterTypeOpenshift ClusterType = "openshift"
7+
// ClusterTypeOpenshift represents an OpenShift cluster
8+
ClusterTypeOpenshift ClusterType = "openshift"
9+
// ClusterTypeKubernetes represents a vanilla Kubernetes cluster
910
ClusterTypeKubernetes ClusterType = "kubernetes"
1011
)
1112

13+
// ClusterFlavor represents the specific flavor or variant of a cluster type
1214
type ClusterFlavor string
1315

1416
const (
15-
ClusterFlavorVanillaK8s ClusterFlavor = "vanilla"
16-
ClusterFlavorOpenshift ClusterFlavor = "default"
17+
// ClusterFlavorDefault represents the standard/default flavor of any cluster type
18+
ClusterFlavorDefault ClusterFlavor = "default"
19+
// ClusterFlavorHypershift represents an OpenShift Hypershift cluster
1720
ClusterFlavorHypershift ClusterFlavor = "hypershift"
1821
)
1922

20-
// PlatformTypes
21-
type PlatformTypes int
23+
// PlatformTypes represents the type of platform the cluster is running on (baremetal, virtual, etc.)
24+
type PlatformTypes string
2225

2326
const (
24-
// Baremetal platform
25-
Baremetal PlatformTypes = iota
26-
// VirtualOpenStack platform
27-
VirtualOpenStack
27+
// Baremetal represents a bare metal platform
28+
Baremetal PlatformTypes = "Baremetal"
29+
// VirtualOpenStack represents a virtual OpenStack platform
30+
VirtualOpenStack PlatformTypes = "Virtual/Openstack"
2831
)
29-
30-
func (e PlatformTypes) String() string {
31-
switch e {
32-
case Baremetal:
33-
return "Baremetal"
34-
case VirtualOpenStack:
35-
return "Virtual/Openstack"
36-
default:
37-
return fmt.Sprintf("%d", int(e))
38-
}
39-
}

pkg/daemon/daemon_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ var _ = Describe("Daemon Controller", Ordered, func() {
152152

153153
genericPlugin, err = generic.NewGenericPlugin(hostHelper)
154154
Expect(err).ToNot(HaveOccurred())
155-
platformMock.EXPECT().GetPlugins(gomock.Any()).Return(genericPlugin, []plugin.VendorPlugin{}, nil)
155+
platformMock.EXPECT().GetVendorPlugins(gomock.Any()).Return(genericPlugin, []plugin.VendorPlugin{}, nil)
156156

157157
featureGates := featuregate.New()
158158
featureGates.Init(map[string]bool{})

pkg/daemon/plugin.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package daemon
22

33
import (
4+
"fmt"
5+
46
"sigs.k8s.io/controller-runtime/pkg/log"
57

68
sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1"
@@ -11,17 +13,17 @@ func (dn *NodeReconciler) loadPlugins(ns *sriovnetworkv1.SriovNetworkNodeState,
1113
funcLog := log.Log.WithName("loadPlugins").WithValues("platform", vars.PlatformType, "orchestrator", vars.ClusterType)
1214
funcLog.Info("loading plugins", "disabled", disabledPlugins)
1315

14-
mainPlugin, additionalPlugins, err := dn.platformInterface.GetPlugins(ns)
16+
mainPlugin, additionalPlugins, err := dn.platformInterface.GetVendorPlugins(ns)
1517
if err != nil {
1618
funcLog.Error(err, "Failed to load plugins", "platform", vars.PlatformType, "orchestrator", vars.ClusterType)
1719
return err
1820
}
1921

20-
// check for the main plugin disabled
21-
// TODO: we really want to allow the disable of the main plugin?
22-
if !isPluginDisabled(mainPlugin.Name(), disabledPlugins) {
23-
dn.mainPlugin = mainPlugin
22+
// Check if the main plugin is disabled - this is not allowed
23+
if isPluginDisabled(mainPlugin.Name(), disabledPlugins) {
24+
return fmt.Errorf("main plugin %s cannot be disabled", mainPlugin.Name())
2425
}
26+
dn.mainPlugin = mainPlugin
2527

2628
for _, plugin := range additionalPlugins {
2729
if !isPluginDisabled(plugin.Name(), disabledPlugins) {

0 commit comments

Comments
 (0)