diff --git a/internal/constants/constants.go b/internal/constants/constants.go index 4ac6fe8f8..cda063216 100644 --- a/internal/constants/constants.go +++ b/internal/constants/constants.go @@ -11,9 +11,9 @@ const ( DaemonSetRole = "kmm.node.kubernetes.io/role" NamespaceLabelKey = "kmm.node.k8s.io/contains-modules" - WorkerPodVersionLabelPrefix = "beta.kmm.node.kubernetes.io/version-worker-pod" - DevicePluginVersionLabelPrefix = "beta.kmm.node.kubernetes.io/version-device-plugin" - ModuleVersionLabelPrefix = "kmm.node.kubernetes.io/version-module" + WorkerPodVersionLabelPrefix = "beta.kmm.node.kubernetes.io/version-worker-pod" + SchedulePluginVersionLabelPrefix = "beta.kmm.node.kubernetes.io/version-schedule-plugin" + ModuleVersionLabelPrefix = "kmm.node.kubernetes.io/version-module" GCDelayFinalizer = "kmm.node.kubernetes.io/gc-delay" ModuleFinalizer = "kmm.node.kubernetes.io/module-finalizer" diff --git a/internal/controllers/device_plugin_reconciler.go b/internal/controllers/device_plugin_reconciler.go index 86a324ce3..973c1a543 100644 --- a/internal/controllers/device_plugin_reconciler.go +++ b/internal/controllers/device_plugin_reconciler.go @@ -500,7 +500,7 @@ func generateDevicePluginLabelsAndSelector(mod *kmmv1beta1.Module) (map[string]s } if mod.Spec.ModuleLoader != nil && mod.Spec.ModuleLoader.Container.Version != "" { - versionLabel := utils.GetDevicePluginVersionLabelName(mod.Namespace, mod.Name) + versionLabel := utils.GetSchedulePluginVersionLabelName(mod.Namespace, mod.Name) labels[versionLabel] = mod.Spec.ModuleLoader.Container.Version nodeSelector[versionLabel] = mod.Spec.ModuleLoader.Container.Version } else if mod.Spec.ModuleLoader == nil { @@ -536,7 +536,7 @@ func getExistingDSFromVersion(existingDS []appsv1.DaemonSet, version = moduleLoader.Container.Version } - versionLabel := utils.GetDevicePluginVersionLabelName(moduleNamespace, moduleName) + versionLabel := utils.GetSchedulePluginVersionLabelName(moduleNamespace, moduleName) for _, ds := range existingDS { dsModuleVersion := ds.GetLabels()[versionLabel] if dsModuleVersion == version { @@ -549,7 +549,7 @@ func getExistingDSFromVersion(existingDS []appsv1.DaemonSet, func isOlderVersionUnusedDevicePluginDaemonset(ds *appsv1.DaemonSet, moduleVersion string) bool { moduleName := ds.Labels[constants.ModuleNameLabel] moduleNamespace := ds.Namespace - versionLabel := utils.GetDevicePluginVersionLabelName(moduleNamespace, moduleName) + versionLabel := utils.GetSchedulePluginVersionLabelName(moduleNamespace, moduleName) return ds.Labels[versionLabel] != moduleVersion && ds.Status.DesiredNumberScheduled == 0 } diff --git a/internal/controllers/device_plugin_reconciler_test.go b/internal/controllers/device_plugin_reconciler_test.go index 15861ca16..b3e20588a 100644 --- a/internal/controllers/device_plugin_reconciler_test.go +++ b/internal/controllers/device_plugin_reconciler_test.go @@ -316,7 +316,7 @@ var _ = Describe("DevicePluginReconciler_garbageCollect", func() { }, }, } - devicePluginVersionLabel := utils.GetDevicePluginVersionLabelName(mod.Namespace, mod.Name) + schedulePluginVersionLabel := utils.GetSchedulePluginVersionLabelName(mod.Namespace, mod.Name) DescribeTable("device-plugin GC", func(devicePluginFormerLabel bool, devicePluginFormerDesired int) { devicePluginDS := appsv1.DaemonSet{ @@ -324,8 +324,8 @@ var _ = Describe("DevicePluginReconciler_garbageCollect", func() { Name: "devicePlugin", Namespace: "namespace", Labels: map[string]string{ - devicePluginVersionLabel: currentModuleVersion, - constants.ModuleNameLabel: mod.Name, + schedulePluginVersionLabel: currentModuleVersion, + constants.ModuleNameLabel: mod.Name, }, }, } @@ -335,7 +335,7 @@ var _ = Describe("DevicePluginReconciler_garbageCollect", func() { if devicePluginFormerLabel { devicePluginFormerVersionDS = devicePluginDS.DeepCopy() devicePluginFormerVersionDS.SetName("devicePluginFormer") - devicePluginFormerVersionDS.Labels[devicePluginVersionLabel] = "former label" + devicePluginFormerVersionDS.Labels[schedulePluginVersionLabel] = "former label" devicePluginFormerVersionDS.Status.DesiredNumberScheduled = int32(devicePluginFormerDesired) existingDS = append(existingDS, *devicePluginFormerVersionDS) } @@ -357,7 +357,7 @@ var _ = Describe("DevicePluginReconciler_garbageCollect", func() { ObjectMeta: metav1.ObjectMeta{ Name: "devicePlugin", Namespace: "namespace", - Labels: map[string]string{constants.ModuleNameLabel: mod.Name, devicePluginVersionLabel: "formerVersion"}, + Labels: map[string]string{constants.ModuleNameLabel: mod.Name, schedulePluginVersionLabel: "formerVersion"}, }, } clnt.EXPECT().Delete(context.Background(), &deleteDS).Return(fmt.Errorf("some error")) @@ -375,7 +375,7 @@ var _ = Describe("DevicePluginReconciler_garbageCollect", func() { ObjectMeta: metav1.ObjectMeta{ Name: "devicePlugin", Namespace: "namespace", - Labels: map[string]string{constants.ModuleNameLabel: mod.Name, devicePluginVersionLabel: "formerVersion"}, + Labels: map[string]string{constants.ModuleNameLabel: mod.Name, schedulePluginVersionLabel: "formerVersion"}, }, } @@ -738,7 +738,7 @@ var _ = Describe("DevicePluginReconciler_setDevicePluginAsDesired", func() { err := dsc.setDevicePluginAsDesired(context.Background(), &ds, &mod) Expect(err).NotTo(HaveOccurred()) - versionLabel := utils.GetDevicePluginVersionLabelName(namespace, moduleName) + versionLabel := utils.GetSchedulePluginVersionLabelName(namespace, moduleName) Expect(ds.GetLabels()).Should(HaveKeyWithValue(versionLabel, "some version")) }) @@ -968,7 +968,7 @@ var _ = Describe("DevicePluginReconciler_getExistingDSFromVersion", func() { ) devicePluginLabels := map[string]string{ - utils.GetDevicePluginVersionLabelName(moduleNamespace, moduleName): moduleVersion, + utils.GetSchedulePluginVersionLabelName(moduleNamespace, moduleName): moduleVersion, } ds := appsv1.DaemonSet{ diff --git a/internal/controllers/dra_reconciler.go b/internal/controllers/dra_reconciler.go index 06530b100..9cd4d43b7 100644 --- a/internal/controllers/dra_reconciler.go +++ b/internal/controllers/dra_reconciler.go @@ -113,6 +113,11 @@ func (r *DRAReconciler) Reconcile(ctx context.Context, mod *kmmv1beta1.Module) ( return res, fmt.Errorf("could not handle DRA: %v", err) } + err = r.reconHelperAPI.garbageCollectDRADaemonSets(ctx, mod, existingDRADS) + if err != nil { + return res, fmt.Errorf("failed to run DRA garbage collection: %v", err) + } + err = r.reconHelperAPI.handleDeviceClasses(ctx, mod, existingDCs) if err != nil { return res, fmt.Errorf("could not handle DeviceClasses: %v", err) @@ -133,6 +138,7 @@ func (r *DRAReconciler) Reconcile(ctx context.Context, mod *kmmv1beta1.Module) ( type draReconcilerHelperAPI interface { getModuleDRADaemonSets(ctx context.Context, name, namespace string) ([]appsv1.DaemonSet, error) handleDRA(ctx context.Context, mod *kmmv1beta1.Module, existingDRADS []appsv1.DaemonSet) error + garbageCollectDRADaemonSets(ctx context.Context, mod *kmmv1beta1.Module, existingDS []appsv1.DaemonSet) error deleteDRAResources(ctx context.Context, moduleName, moduleNamespace string) error moduleUpdateDRAStatus(ctx context.Context, mod *kmmv1beta1.Module, existingDRADS []appsv1.DaemonSet) error clearDRAStatus(ctx context.Context, mod *kmmv1beta1.Module) error @@ -181,11 +187,9 @@ func (drh *draReconcilerHelper) handleDRA(ctx context.Context, mod *kmmv1beta1.M logger := log.FromContext(ctx) - var ds *appsv1.DaemonSet - if len(existingDRADS) > 0 { - ds = &existingDRADS[0] - } else { - logger.Info("creating new DRA DaemonSet") + ds, version := getExistingDRADSFromVersion(existingDRADS, mod.Namespace, mod.Name, mod.Spec.ModuleLoader) + if ds == nil { + logger.Info("creating new DRA DaemonSet", "version", version) ds = &appsv1.DaemonSet{ ObjectMeta: metav1.ObjectMeta{Namespace: mod.Namespace, GenerateName: mod.Name + "-dra-"}, } @@ -202,6 +206,51 @@ func (drh *draReconcilerHelper) handleDRA(ctx context.Context, mod *kmmv1beta1.M return err } +func (drh *draReconcilerHelper) garbageCollectDRADaemonSets(ctx context.Context, mod *kmmv1beta1.Module, existingDS []appsv1.DaemonSet) error { + if mod.Spec.ModuleLoader == nil { + return nil + } + + logger := log.FromContext(ctx) + deleted := make([]string, 0) + for _, ds := range existingDS { + if isOlderVersionUnusedDRADaemonSet(&ds, mod.Namespace, mod.Spec.ModuleLoader.Container.Version) { + deleted = append(deleted, ds.Name) + if err := drh.client.Delete(ctx, &ds); err != nil { + return fmt.Errorf("could not delete DRA DaemonSet %s: %v", ds.Name, err) + } + } + } + + logger.Info("garbage-collected DRA DaemonSets", "names", deleted) + return nil +} + +func getExistingDRADSFromVersion(existingDS []appsv1.DaemonSet, + moduleNamespace string, + moduleName string, + moduleLoader *kmmv1beta1.ModuleLoaderSpec) (*appsv1.DaemonSet, string) { + version := "" + if moduleLoader != nil { + version = moduleLoader.Container.Version + } + + versionLabel := utils.GetSchedulePluginVersionLabelName(moduleNamespace, moduleName) + for _, ds := range existingDS { + dsModuleVersion := ds.GetLabels()[versionLabel] + if dsModuleVersion == version { + return &ds, version + } + } + return nil, version +} + +func isOlderVersionUnusedDRADaemonSet(ds *appsv1.DaemonSet, moduleNamespace, moduleVersion string) bool { + moduleName := ds.Labels[constants.ModuleNameLabel] + versionLabel := utils.GetSchedulePluginVersionLabelName(moduleNamespace, moduleName) + return ds.Labels[versionLabel] != moduleVersion && ds.Status.DesiredNumberScheduled == 0 +} + // deleteDRAResources deletes all DRA-owned DaemonSets and DeviceClasses using label-based bulk deletion. func (drh *draReconcilerHelper) deleteDRAResources(ctx context.Context, moduleName, moduleNamespace string) error { var errs []error @@ -403,6 +452,12 @@ func (dsci *draDaemonSetCreatorImpl) setDRAAsDesired( utils.GetKernelModuleReadyNodeLabel(mod.Namespace, mod.Name): "", } + if mod.Spec.ModuleLoader != nil && mod.Spec.ModuleLoader.Container.Version != "" { + versionLabel := utils.GetSchedulePluginVersionLabelName(mod.Namespace, mod.Name) + standardLabels[versionLabel] = mod.Spec.ModuleLoader.Container.Version + nodeSelector[versionLabel] = mod.Spec.ModuleLoader.Container.Version + } + ds.SetLabels( overrideLabels(ds.GetLabels(), standardLabels), ) diff --git a/internal/controllers/dra_reconciler_test.go b/internal/controllers/dra_reconciler_test.go index 31402f523..1d767c962 100644 --- a/internal/controllers/dra_reconciler_test.go +++ b/internal/controllers/dra_reconciler_test.go @@ -69,7 +69,7 @@ var _ = Describe("DRAReconciler_Reconcile", func() { ctx := context.Background() - DescribeTable("check error flows", func(getDSError, getDCError, handleDRAError, handleDCError bool) { + DescribeTable("check error flows", func(getDSError, getDCError, handleDRAError, gcError, handleDCError bool) { draDS := []appsv1.DaemonSet{{}} returnedError := fmt.Errorf("some error") if getDSError { @@ -87,6 +87,11 @@ var _ = Describe("DRAReconciler_Reconcile", func() { goto executeTestFunction } mockReconHelper.EXPECT().handleDRA(ctx, mod, draDS).Return(nil) + if gcError { + mockReconHelper.EXPECT().garbageCollectDRADaemonSets(ctx, mod, draDS).Return(returnedError) + goto executeTestFunction + } + mockReconHelper.EXPECT().garbageCollectDRADaemonSets(ctx, mod, draDS).Return(nil) if handleDCError { mockReconHelper.EXPECT().handleDeviceClasses(ctx, mod, []resourcev1.DeviceClass(nil)).Return(returnedError) goto executeTestFunction @@ -101,11 +106,12 @@ var _ = Describe("DRAReconciler_Reconcile", func() { Expect(err).To(HaveOccurred()) }, - Entry("getModuleDRADaemonSets failed", true, false, false, false), - Entry("getModuleDeviceClasses failed", false, true, false, false), - Entry("handleDRA failed", false, false, true, false), - Entry("handleDeviceClasses failed", false, false, false, true), - Entry("moduleUpdateDRAStatus failed", false, false, false, false), + Entry("getModuleDRADaemonSets failed", true, false, false, false, false), + Entry("getModuleDeviceClasses failed", false, true, false, false, false), + Entry("handleDRA failed", false, false, true, false, false), + Entry("garbageCollectDRADaemonSets failed", false, false, false, true, false), + Entry("handleDeviceClasses failed", false, false, false, false, true), + Entry("moduleUpdateDRAStatus failed", false, false, false, false, false), ) It("Good flow", func() { @@ -114,6 +120,7 @@ var _ = Describe("DRAReconciler_Reconcile", func() { mockReconHelper.EXPECT().getModuleDRADaemonSets(ctx, mod.Name, mod.Namespace).Return(draDS, nil), mockReconHelper.EXPECT().getModuleDeviceClasses(ctx, mod.Name, mod.Namespace).Return(nil, nil), mockReconHelper.EXPECT().handleDRA(ctx, mod, draDS).Return(nil), + mockReconHelper.EXPECT().garbageCollectDRADaemonSets(ctx, mod, draDS).Return(nil), mockReconHelper.EXPECT().handleDeviceClasses(ctx, mod, []resourcev1.DeviceClass(nil)).Return(nil), mockReconHelper.EXPECT().moduleUpdateDRAStatus(ctx, mod, draDS).Return(nil), ) @@ -699,6 +706,149 @@ var _ = Describe("DRAReconciler_setDRAAsDesired", func() { Entry("without init container", false), Entry("with init container", true), ) + + It("should include the version-dra label in the DaemonSet labels and node selector when version is set", func() { + mod := kmmv1beta1.Module{ + TypeMeta: metav1.TypeMeta{ + APIVersion: kmmv1beta1.GroupVersion.String(), + Kind: "Module", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: draModuleName, + Namespace: namespace, + }, + Spec: kmmv1beta1.ModuleSpec{ + ModuleLoader: &kmmv1beta1.ModuleLoaderSpec{ + Container: kmmv1beta1.ModuleLoaderContainerSpec{ + Version: "1", + Modprobe: kmmv1beta1.ModprobeSpec{ + ModuleName: "test-mod", + }, + KernelMappings: []kmmv1beta1.KernelMapping{ + {Regexp: "^.+$", ContainerImage: "some-image"}, + }, + }, + }, + DRA: &kmmv1beta1.DRASpec{ + Container: kmmv1beta1.CommonContainerSpec{ + Image: draImage, + }, + DriverName: "test.driver", + }, + }, + } + + ds := appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ds", + Namespace: namespace, + }, + } + + err := dsc.setDRAAsDesired(context.Background(), &ds, &mod) + Expect(err).NotTo(HaveOccurred()) + + versionLabel := utils.GetSchedulePluginVersionLabelName(namespace, draModuleName) + + Expect(ds.Labels).To(HaveKeyWithValue(versionLabel, "1")) + Expect(ds.Spec.Template.Spec.NodeSelector).To(HaveKeyWithValue(versionLabel, "1")) + Expect(ds.Spec.Template.Spec.NodeSelector).To(HaveKeyWithValue( + utils.GetKernelModuleReadyNodeLabel(namespace, draModuleName), "", + )) + }) +}) + +var _ = Describe("DRAReconciler_garbageCollectDRADaemonSets", func() { + const currentModuleVersion = "current" + + var ( + ctrl *gomock.Controller + clnt *client.MockClient + drh draReconcilerHelper + ) + + BeforeEach(func() { + ctrl = gomock.NewController(GinkgoT()) + clnt = client.NewMockClient(ctrl) + drh = draReconcilerHelper{client: clnt} + }) + + mod := &kmmv1beta1.Module{ + ObjectMeta: metav1.ObjectMeta{ + Name: "moduleName", + Namespace: "namespace", + }, + Spec: kmmv1beta1.ModuleSpec{ + ModuleLoader: &kmmv1beta1.ModuleLoaderSpec{ + Container: kmmv1beta1.ModuleLoaderContainerSpec{ + Version: currentModuleVersion, + }, + }, + }, + } + schedulePluginVersionLabel := utils.GetSchedulePluginVersionLabelName(mod.Namespace, mod.Name) + + DescribeTable("DRA GC", func(formerDSExists bool, formerDesired int) { + currentDS := appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dra-current", + Namespace: "namespace", + Labels: map[string]string{ + schedulePluginVersionLabel: currentModuleVersion, + constants.ModuleNameLabel: mod.Name, + }, + }, + } + formerDS := &appsv1.DaemonSet{} + + existingDS := []appsv1.DaemonSet{currentDS} + if formerDSExists { + formerDS = currentDS.DeepCopy() + formerDS.SetName("dra-former") + formerDS.Labels[schedulePluginVersionLabel] = "former" + formerDS.Status.DesiredNumberScheduled = int32(formerDesired) + existingDS = append(existingDS, *formerDS) + } + if formerDSExists && formerDesired == 0 { + clnt.EXPECT().Delete(context.Background(), formerDS).Return(nil) + } + + err := drh.garbageCollectDRADaemonSets(context.Background(), mod, existingDS) + Expect(err).NotTo(HaveOccurred()) + }, + Entry("no former DS to delete", false, 0), + Entry("former DS with zero desired — deleted", true, 0), + Entry("former DS still has desired pods — kept", true, 1), + ) + + It("should return an error if a deletion failed", func() { + deleteDS := appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dra-old", + Namespace: "namespace", + Labels: map[string]string{constants.ModuleNameLabel: mod.Name, schedulePluginVersionLabel: "formerVersion"}, + }, + } + clnt.EXPECT().Delete(context.Background(), &deleteDS).Return(fmt.Errorf("some error")) + + err := drh.garbageCollectDRADaemonSets(context.Background(), mod, []appsv1.DaemonSet{deleteDS}) + Expect(err).To(HaveOccurred()) + }) + + It("should pass if moduleLoader is not defined", func() { + modWithoutModuleLoader := mod.DeepCopy() + modWithoutModuleLoader.Spec.ModuleLoader = nil + oldDS := appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dra-old", + Namespace: "namespace", + Labels: map[string]string{constants.ModuleNameLabel: mod.Name, schedulePluginVersionLabel: "formerVersion"}, + }, + } + + err := drh.garbageCollectDRADaemonSets(context.Background(), modWithoutModuleLoader, []appsv1.DaemonSet{oldDS}) + Expect(err).ToNot(HaveOccurred()) + }) }) var _ = Describe("DRAReconciler_getModuleDRADaemonSets", func() { diff --git a/internal/controllers/mock_dra_reconciler.go b/internal/controllers/mock_dra_reconciler.go index 4d50c6e28..2bdeb235f 100644 --- a/internal/controllers/mock_dra_reconciler.go +++ b/internal/controllers/mock_dra_reconciler.go @@ -69,6 +69,20 @@ func (mr *MockdraReconcilerHelperAPIMockRecorder) deleteDRAResources(ctx, module return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "deleteDRAResources", reflect.TypeOf((*MockdraReconcilerHelperAPI)(nil).deleteDRAResources), ctx, moduleName, moduleNamespace) } +// garbageCollectDRADaemonSets mocks base method. +func (m *MockdraReconcilerHelperAPI) garbageCollectDRADaemonSets(ctx context.Context, mod *v1beta1.Module, existingDS []v1.DaemonSet) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "garbageCollectDRADaemonSets", ctx, mod, existingDS) + ret0, _ := ret[0].(error) + return ret0 +} + +// garbageCollectDRADaemonSets indicates an expected call of garbageCollectDRADaemonSets. +func (mr *MockdraReconcilerHelperAPIMockRecorder) garbageCollectDRADaemonSets(ctx, mod, existingDS any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "garbageCollectDRADaemonSets", reflect.TypeOf((*MockdraReconcilerHelperAPI)(nil).garbageCollectDRADaemonSets), ctx, mod, existingDS) +} + // getModuleDRADaemonSets mocks base method. func (m *MockdraReconcilerHelperAPI) getModuleDRADaemonSets(ctx context.Context, name, namespace string) ([]v1.DaemonSet, error) { m.ctrl.T.Helper() diff --git a/internal/controllers/mock_node_label_module_version_reconciler.go b/internal/controllers/mock_node_label_module_version_reconciler.go index 475785125..2c30a3112 100644 --- a/internal/controllers/mock_node_label_module_version_reconciler.go +++ b/internal/controllers/mock_node_label_module_version_reconciler.go @@ -40,21 +40,6 @@ func (m *MocknodeLabelModuleVersionHelperAPI) EXPECT() *MocknodeLabelModuleVersi return m.recorder } -// getDevicePluginPods mocks base method. -func (m *MocknodeLabelModuleVersionHelperAPI) getDevicePluginPods(ctx context.Context, nodeName string) ([]v1.Pod, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "getDevicePluginPods", ctx, nodeName) - ret0, _ := ret[0].([]v1.Pod) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// getDevicePluginPods indicates an expected call of getDevicePluginPods. -func (mr *MocknodeLabelModuleVersionHelperAPIMockRecorder) getDevicePluginPods(ctx, nodeName any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "getDevicePluginPods", reflect.TypeOf((*MocknodeLabelModuleVersionHelperAPI)(nil).getDevicePluginPods), ctx, nodeName) -} - // getLabelsPerModules mocks base method. func (m *MocknodeLabelModuleVersionHelperAPI) getLabelsPerModules(ctx context.Context, nodeLabels map[string]string) map[string]*modulesVersionLabels { m.ctrl.T.Helper() @@ -83,18 +68,33 @@ func (mr *MocknodeLabelModuleVersionHelperAPIMockRecorder) getLoadedKernelModule return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "getLoadedKernelModules", reflect.TypeOf((*MocknodeLabelModuleVersionHelperAPI)(nil).getLoadedKernelModules), labels) } +// getSchedulePluginPods mocks base method. +func (m *MocknodeLabelModuleVersionHelperAPI) getSchedulePluginPods(ctx context.Context, nodeName string) ([]v1.Pod, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "getSchedulePluginPods", ctx, nodeName) + ret0, _ := ret[0].([]v1.Pod) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// getSchedulePluginPods indicates an expected call of getSchedulePluginPods. +func (mr *MocknodeLabelModuleVersionHelperAPIMockRecorder) getSchedulePluginPods(ctx, nodeName any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "getSchedulePluginPods", reflect.TypeOf((*MocknodeLabelModuleVersionHelperAPI)(nil).getSchedulePluginPods), ctx, nodeName) +} + // reconcileLabels mocks base method. -func (m *MocknodeLabelModuleVersionHelperAPI) reconcileLabels(modulesLabels map[string]*modulesVersionLabels, devicePluginPods []v1.Pod, kernelModuleReadyLabels []types.NamespacedName) *reconcileLabelsResult { +func (m *MocknodeLabelModuleVersionHelperAPI) reconcileLabels(modulesLabels map[string]*modulesVersionLabels, schedulePluginPods []v1.Pod, kernelModuleReadyLabels []types.NamespacedName) *reconcileLabelsResult { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "reconcileLabels", modulesLabels, devicePluginPods, kernelModuleReadyLabels) + ret := m.ctrl.Call(m, "reconcileLabels", modulesLabels, schedulePluginPods, kernelModuleReadyLabels) ret0, _ := ret[0].(*reconcileLabelsResult) return ret0 } // reconcileLabels indicates an expected call of reconcileLabels. -func (mr *MocknodeLabelModuleVersionHelperAPIMockRecorder) reconcileLabels(modulesLabels, devicePluginPods, kernelModuleReadyLabels any) *gomock.Call { +func (mr *MocknodeLabelModuleVersionHelperAPIMockRecorder) reconcileLabels(modulesLabels, schedulePluginPods, kernelModuleReadyLabels any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "reconcileLabels", reflect.TypeOf((*MocknodeLabelModuleVersionHelperAPI)(nil).reconcileLabels), modulesLabels, devicePluginPods, kernelModuleReadyLabels) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "reconcileLabels", reflect.TypeOf((*MocknodeLabelModuleVersionHelperAPI)(nil).reconcileLabels), modulesLabels, schedulePluginPods, kernelModuleReadyLabels) } // updateNodeLabels mocks base method. diff --git a/internal/controllers/module_version_label_action_table.go b/internal/controllers/module_version_label_action_table.go index 875ba9fa4..3f1ca5d37 100644 --- a/internal/controllers/module_version_label_action_table.go +++ b/internal/controllers/module_version_label_action_table.go @@ -13,9 +13,9 @@ const ( ) type labelActionKey struct { - module string - workerPod string - devicePlugin string + module string + workerPod string + schedulePlugin string } type labelAction struct { @@ -24,43 +24,43 @@ type labelAction struct { } var labelActionTable = map[labelActionKey]labelAction{ - labelActionKey{ - module: labelMissing, - workerPod: labelMissing, - devicePlugin: labelMissing}: labelAction{getLabelName: nil, action: noneAction}, + { + module: labelMissing, + workerPod: labelMissing, + schedulePlugin: labelMissing}: {getLabelName: nil, action: noneAction}, - labelActionKey{ - module: labelMissing, - workerPod: labelPresent, - devicePlugin: labelPresent}: labelAction{getLabelName: utils.GetDevicePluginVersionLabelName, action: deleteAction}, + { + module: labelMissing, + workerPod: labelPresent, + schedulePlugin: labelPresent}: {getLabelName: utils.GetSchedulePluginVersionLabelName, action: deleteAction}, - labelActionKey{ - module: labelMissing, - workerPod: labelPresent, - devicePlugin: labelMissing}: labelAction{getLabelName: utils.GetWorkerPodVersionLabelName, action: deleteAction}, + { + module: labelMissing, + workerPod: labelPresent, + schedulePlugin: labelMissing}: {getLabelName: utils.GetWorkerPodVersionLabelName, action: deleteAction}, - labelActionKey{ - module: labelPresent, - workerPod: labelMissing, - devicePlugin: labelMissing}: labelAction{getLabelName: utils.GetWorkerPodVersionLabelName, action: addAction}, + { + module: labelPresent, + workerPod: labelMissing, + schedulePlugin: labelMissing}: {getLabelName: utils.GetWorkerPodVersionLabelName, action: addAction}, - labelActionKey{ - module: labelPresent, - workerPod: labelPresent, - devicePlugin: labelMissing}: labelAction{getLabelName: utils.GetDevicePluginVersionLabelName, action: addAction}, + { + module: labelPresent, + workerPod: labelPresent, + schedulePlugin: labelMissing}: {getLabelName: utils.GetSchedulePluginVersionLabelName, action: addAction}, - labelActionKey{ - module: labelPresent, - workerPod: labelPresent, - devicePlugin: labelPresent}: labelAction{getLabelName: nil, action: noneAction}, + { + module: labelPresent, + workerPod: labelPresent, + schedulePlugin: labelPresent}: {getLabelName: nil, action: noneAction}, - labelActionKey{ - module: labelPresent, - workerPod: labelDifferent, - devicePlugin: labelDifferent}: labelAction{getLabelName: utils.GetDevicePluginVersionLabelName, action: deleteAction}, + { + module: labelPresent, + workerPod: labelDifferent, + schedulePlugin: labelDifferent}: {getLabelName: utils.GetSchedulePluginVersionLabelName, action: deleteAction}, - labelActionKey{ - module: labelPresent, - workerPod: labelDifferent, - devicePlugin: labelMissing}: labelAction{getLabelName: utils.GetWorkerPodVersionLabelName, action: deleteAction}, + { + module: labelPresent, + workerPod: labelDifferent, + schedulePlugin: labelMissing}: {getLabelName: utils.GetWorkerPodVersionLabelName, action: deleteAction}, } diff --git a/internal/controllers/node_label_module_version_reconciler.go b/internal/controllers/node_label_module_version_reconciler.go index c766d2c5a..72c13c6d1 100644 --- a/internal/controllers/node_label_module_version_reconciler.go +++ b/internal/controllers/node_label_module_version_reconciler.go @@ -9,6 +9,8 @@ import ( "github.com/kubernetes-sigs/kernel-module-management/internal/filter" "github.com/kubernetes-sigs/kernel-module-management/internal/utils" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -16,13 +18,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -// this struct contains all the version labels related to a specific Module type modulesVersionLabels struct { - name string - namespace string - moduleVersionLabel string - workerPodVersionLabel string - devicePluginVersionLabel string + name string + namespace string + moduleVersionLabel string + workerPodVersionLabel string + schedulePluginVersionLabel string } const ( @@ -50,14 +51,14 @@ func NewNodeLabelModuleVersionReconciler(client client.Client) *NodeLabelModuleV func (nlmvr *NodeLabelModuleVersionReconciler) Reconcile(ctx context.Context, node *v1.Node) (ctrl.Result, error) { modulesVersionLabels := nlmvr.helperAPI.getLabelsPerModules(ctx, node.Labels) - devicePluginPods, err := nlmvr.helperAPI.getDevicePluginPods(ctx, node.Name) + schedulePluginPods, err := nlmvr.helperAPI.getSchedulePluginPods(ctx, node.Name) if err != nil { - return ctrl.Result{}, fmt.Errorf("could get device plugin pods for the node %s: %v", node.Name, err) + return ctrl.Result{}, fmt.Errorf("could not get schedule plugin pods for the node %s: %v", node.Name, err) } loadedKernelModules := nlmvr.helperAPI.getLoadedKernelModules(node.GetLabels()) - reconLabelsRes := nlmvr.helperAPI.reconcileLabels(modulesVersionLabels, devicePluginPods, loadedKernelModules) + reconLabelsRes := nlmvr.helperAPI.reconcileLabels(modulesVersionLabels, schedulePluginPods, loadedKernelModules) logger := log.FromContext(ctx).WithValues("node name", node.Name) logLabelsUpdateData(logger, reconLabelsRes) @@ -74,9 +75,9 @@ func (nlmvr *NodeLabelModuleVersionReconciler) Reconcile(ctx context.Context, no type nodeLabelModuleVersionHelperAPI interface { getLabelsPerModules(ctx context.Context, nodeLabels map[string]string) map[string]*modulesVersionLabels - getDevicePluginPods(ctx context.Context, nodeName string) ([]v1.Pod, error) + getSchedulePluginPods(ctx context.Context, nodeName string) ([]v1.Pod, error) getLoadedKernelModules(labels map[string]string) []types.NamespacedName - reconcileLabels(modulesLabels map[string]*modulesVersionLabels, devicePluginPods []v1.Pod, kernelModuleReadyLabels []types.NamespacedName) *reconcileLabelsResult + reconcileLabels(modulesLabels map[string]*modulesVersionLabels, schedulePluginPods []v1.Pod, kernelModuleReadyLabels []types.NamespacedName) *reconcileLabelsResult updateNodeLabels(ctx context.Context, nodeName string, reconLabelsRes *reconcileLabelsResult) error } @@ -109,8 +110,8 @@ func (nlmvha *nodeLabelModuleVersionHelper) getLabelsPerModules(ctx context.Cont labelsPerModule[mapKey].moduleVersionLabel = value case utils.IsWorkerPodVersionLabel(key): labelsPerModule[mapKey].workerPodVersionLabel = value - case utils.IsDevicePluginVersionLabel(key): - labelsPerModule[mapKey].devicePluginVersionLabel = value + case utils.IsSchedulePluginVersionLabel(key): + labelsPerModule[mapKey].schedulePluginVersionLabel = value } } } @@ -118,25 +119,26 @@ func (nlmvha *nodeLabelModuleVersionHelper) getLabelsPerModules(ctx context.Cont return labelsPerModule } -func (nlmvha *nodeLabelModuleVersionHelper) getDevicePluginPods(ctx context.Context, nodeName string) ([]v1.Pod, error) { - var kmmPodsList v1.PodList - fieldSelector := client.MatchingFields{"spec.nodeName": nodeName} - labelSelector := client.HasLabels{constants.ModuleNameLabel} - err := nlmvha.client.List(ctx, &kmmPodsList, labelSelector, fieldSelector) +func (nlmvha *nodeLabelModuleVersionHelper) getSchedulePluginPods(ctx context.Context, nodeName string) ([]v1.Pod, error) { + req, err := labels.NewRequirement( + constants.DaemonSetRole, + selection.In, + []string{constants.DevicePluginRoleLabelValue, constants.DRARoleLabelValue}, + ) if err != nil { - return nil, fmt.Errorf("failed to get list of all device plugin pods for on node %s: %v", nodeName, err) + return nil, fmt.Errorf("failed to create label requirement: %v", err) } - devicePluginPods := make([]v1.Pod, 0, len(kmmPodsList.Items)) - for _, pod := range kmmPodsList.Items { - for _, ownerReference := range pod.ObjectMeta.OwnerReferences { - if ownerReference.Kind == "DaemonSet" { - devicePluginPods = append(devicePluginPods, pod) - break - } - } + var podsList v1.PodList + err = nlmvha.client.List(ctx, &podsList, + client.HasLabels{constants.ModuleNameLabel}, + client.MatchingFields{"spec.nodeName": nodeName}, + client.MatchingLabelsSelector{Selector: labels.NewSelector().Add(*req)}, + ) + if err != nil { + return nil, fmt.Errorf("failed to get list of schedule plugin pods on node %s: %v", nodeName, err) } - return devicePluginPods, nil + return podsList.Items, nil } func (nlmvha *nodeLabelModuleVersionHelper) getLoadedKernelModules(nodeLabels map[string]string) []types.NamespacedName { @@ -174,7 +176,7 @@ func (nlmvha *nodeLabelModuleVersionHelper) updateNodeLabels(ctx context.Context } func (nlmvha *nodeLabelModuleVersionHelper) reconcileLabels(modulesLabels map[string]*modulesVersionLabels, - devicePluginPods []v1.Pod, + schedulePluginPods []v1.Pod, loadedKernelModules []types.NamespacedName) *reconcileLabelsResult { reconRes := reconcileLabelsResult{ @@ -185,7 +187,7 @@ func (nlmvha *nodeLabelModuleVersionHelper) reconcileLabels(modulesLabels map[st label, labelValue, action := getLabelAndAction(moduleLabels) switch action { case deleteAction: - if utils.IsWorkerPodVersionLabel(label) && !verifyLabelDeleteValidity(moduleLabels.name, moduleLabels.namespace, devicePluginPods) { + if utils.IsWorkerPodVersionLabel(label) && !verifyLabelDeleteValidity(moduleLabels.name, moduleLabels.namespace, schedulePluginPods) { reconRes.requeue = true } else { reconRes.labelsToDelete = append(reconRes.labelsToDelete, label) @@ -202,11 +204,11 @@ func (nlmvha *nodeLabelModuleVersionHelper) reconcileLabels(modulesLabels map[st return &reconRes } -// validity is checked by verifying that devicePlugin pod defined -// by name, namespace,role is missing. In case the pod is present, no matter in -// what state, then the action is invalid -func verifyLabelDeleteValidity(name, namespace string, devicePluginPods []v1.Pod) bool { - for _, pod := range devicePluginPods { +// verifyLabelDeleteValidity checks that no schedule plugin pod (device plugin +// or DRA) matching the module name and namespace is present. If a matching pod +// exists, the delete action is invalid regardless of pod state. +func verifyLabelDeleteValidity(name, namespace string, pods []v1.Pod) bool { + for _, pod := range pods { podLabels := pod.GetLabels() if pod.Namespace == namespace && podLabels[constants.ModuleNameLabel] == name { return false @@ -215,8 +217,6 @@ func verifyLabelDeleteValidity(name, namespace string, devicePluginPods []v1.Pod return true } -// validity is checked by verifying that kernel module defined -// by name and namespace is not loaded. In case the kernel module is loaded, then the action is invalid func verifyLabelAddValidity(name, namespace string, loadedKernelModules []types.NamespacedName) bool { nsn := types.NamespacedName{Name: name, Namespace: namespace} for _, kernelModule := range loadedKernelModules { @@ -227,7 +227,6 @@ func verifyLabelAddValidity(name, namespace string, loadedKernelModules []types. return true } -// returns the label, value of the label and action to execute on label (add or delete) func getLabelAndAction(moduleLabels *modulesVersionLabels) (string, string, string) { labelActionKey := getModuleVersionLabelsState(moduleLabels) labelAction, ok := labelActionTable[labelActionKey] @@ -246,9 +245,9 @@ func getLabelAndAction(moduleLabels *modulesVersionLabels) (string, string, stri func getModuleVersionLabelsState(moduleLabels *modulesVersionLabels) labelActionKey { key := labelActionKey{ - module: labelPresent, - workerPod: labelPresent, - devicePlugin: labelPresent, + module: labelPresent, + workerPod: labelPresent, + schedulePlugin: labelPresent, } if moduleLabels.moduleVersionLabel == "" { key.module = labelMissing @@ -258,10 +257,10 @@ func getModuleVersionLabelsState(moduleLabels *modulesVersionLabels) labelAction } else if moduleLabels.moduleVersionLabel != "" && moduleLabels.workerPodVersionLabel != moduleLabels.moduleVersionLabel { key.workerPod = labelDifferent } - if moduleLabels.devicePluginVersionLabel == "" { - key.devicePlugin = labelMissing - } else if moduleLabels.moduleVersionLabel != "" && moduleLabels.devicePluginVersionLabel != moduleLabels.moduleVersionLabel { - key.devicePlugin = labelDifferent + if moduleLabels.schedulePluginVersionLabel == "" { + key.schedulePlugin = labelMissing + } else if moduleLabels.moduleVersionLabel != "" && moduleLabels.schedulePluginVersionLabel != moduleLabels.moduleVersionLabel { + key.schedulePlugin = labelDifferent } return key } diff --git a/internal/controllers/node_label_module_version_reconciler_test.go b/internal/controllers/node_label_module_version_reconciler_test.go index 73dbb3a81..82d7517c9 100644 --- a/internal/controllers/node_label_module_version_reconciler_test.go +++ b/internal/controllers/node_label_module_version_reconciler_test.go @@ -41,22 +41,22 @@ var _ = Describe("Reconcile", func() { ctx := context.Background() nodeLabels := map[string]string{"some label": "some label value"} - DescribeTable("reconciler flow", func(getMLPodsError, upDateNodeLabelsErrors, requeue bool) { + DescribeTable("reconciler flow", func(getSchedulePluginPodsError, upDateNodeLabelsErrors, requeue bool) { labelsPerModules := map[string]*modulesVersionLabels{ - "moduleNameNamespace": &modulesVersionLabels{name: "name", namespace: "namespace"}, + "moduleNameNamespace": {name: "name", namespace: "namespace"}, } - devicePluginPods := []v1.Pod{v1.Pod{}} + schedulePluginPods := []v1.Pod{{}} loadedKernelModules := []types.NamespacedName{{Name: "some name", Namespace: "some namespace"}} reconcileLabelsResult := &reconcileLabelsResult{requeue: requeue} expectedRes := ctrl.Result{Requeue: requeue} mockHelper.EXPECT().getLabelsPerModules(ctx, nodeLabels).Return(labelsPerModules) - if getMLPodsError { - mockHelper.EXPECT().getDevicePluginPods(ctx, nodeName).Return(nil, fmt.Errorf("some error")) + if getSchedulePluginPodsError { + mockHelper.EXPECT().getSchedulePluginPods(ctx, nodeName).Return(nil, fmt.Errorf("some error")) goto executeTestFunction } - mockHelper.EXPECT().getDevicePluginPods(ctx, nodeName).Return(devicePluginPods, nil) + mockHelper.EXPECT().getSchedulePluginPods(ctx, nodeName).Return(schedulePluginPods, nil) mockHelper.EXPECT().getLoadedKernelModules(nodeLabels).Return(loadedKernelModules) - mockHelper.EXPECT().reconcileLabels(labelsPerModules, devicePluginPods, loadedKernelModules).Return(reconcileLabelsResult) + mockHelper.EXPECT().reconcileLabels(labelsPerModules, schedulePluginPods, loadedKernelModules).Return(reconcileLabelsResult) if upDateNodeLabelsErrors { mockHelper.EXPECT().updateNodeLabels(ctx, nodeName, reconcileLabelsResult).Return(fmt.Errorf("some error")) goto executeTestFunction @@ -71,7 +71,7 @@ var _ = Describe("Reconcile", func() { } res, err := nlmvr.Reconcile(ctx, &node) - if upDateNodeLabelsErrors || getMLPodsError { + if upDateNodeLabelsErrors || getSchedulePluginPodsError { Expect(err).To(HaveOccurred()) } else { Expect(err).ToNot(HaveOccurred()) @@ -80,7 +80,7 @@ var _ = Describe("Reconcile", func() { }, Entry("good flow, no requeue", false, false, false), Entry("good flow, with requeue", false, false, true), - Entry("get module loader pods failed", true, false, false), + Entry("get schedule plugin pods failed", true, false, false), Entry("update node labels failed", false, true, false), ) }) @@ -97,36 +97,46 @@ var _ = Describe("getLabelsPerModules", func() { nodeLabels := map[string]string{ "some label 1": "some value1", "some label 2": "", - "beta.kmm.node.kubernetes.io/version-worker-pod.namespace1.module1": "1", - "beta.kmm.node.kubernetes.io/version-device-plugin.namespace1.module1": "1", - "kmm.node.kubernetes.io/version-module.namespace1.module1": "1", - "beta.kmm.node.kubernetes.io/version-worker-pod.namespace2.module2": "3", - "kmm.node.kubernetes.io/version-module.namespace2.module2": "3", - "beta.kmm.node.kubernetes.io/version-device-plugin.namespace3.module3": "4", - "kmm.node.kubernetes.io/version-module.namespace5.module5": "10", + "beta.kmm.node.kubernetes.io/version-worker-pod.namespace1.module1": "1", + "beta.kmm.node.kubernetes.io/version-schedule-plugin.namespace1.module1": "1", + "kmm.node.kubernetes.io/version-module.namespace1.module1": "1", + "beta.kmm.node.kubernetes.io/version-worker-pod.namespace2.module2": "3", + "kmm.node.kubernetes.io/version-module.namespace2.module2": "3", + "beta.kmm.node.kubernetes.io/version-schedule-plugin.namespace3.module3": "4", + "kmm.node.kubernetes.io/version-module.namespace5.module5": "10", + "beta.kmm.node.kubernetes.io/version-schedule-plugin.namespace4.module4": "5", + "beta.kmm.node.kubernetes.io/version-worker-pod.namespace4.module4": "5", + "kmm.node.kubernetes.io/version-module.namespace4.module4": "5", } It("normal flow", func() { expectedRes := map[string]*modulesVersionLabels{ - "namespace1-module1": &modulesVersionLabels{ - name: "module1", - namespace: "namespace1", - moduleVersionLabel: "1", - workerPodVersionLabel: "1", - devicePluginVersionLabel: "1", + "namespace1-module1": { + name: "module1", + namespace: "namespace1", + moduleVersionLabel: "1", + workerPodVersionLabel: "1", + schedulePluginVersionLabel: "1", }, - "namespace2-module2": &modulesVersionLabels{ + "namespace2-module2": { name: "module2", namespace: "namespace2", moduleVersionLabel: "3", workerPodVersionLabel: "3", }, - "namespace3-module3": &modulesVersionLabels{ - name: "module3", - namespace: "namespace3", - devicePluginVersionLabel: "4", + "namespace3-module3": { + name: "module3", + namespace: "namespace3", + schedulePluginVersionLabel: "4", }, - "namespace5-module5": &modulesVersionLabels{ + "namespace4-module4": { + name: "module4", + namespace: "namespace4", + moduleVersionLabel: "5", + workerPodVersionLabel: "5", + schedulePluginVersionLabel: "5", + }, + "namespace5-module5": { name: "module5", namespace: "namespace5", moduleVersionLabel: "10", @@ -141,7 +151,7 @@ var _ = Describe("getLabelsPerModules", func() { }) }) -var _ = Describe("getDevicePluginPods", func() { +var _ = Describe("getSchedulePluginPods", func() { const ( nodeName = "node-name" ) @@ -162,45 +172,30 @@ var _ = Describe("getDevicePluginPods", func() { It("good flow", func() { pod1 := v1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{constants.ModuleNameLabel: "module name"}, - OwnerReferences: []metav1.OwnerReference{ - { - Kind: "some kind", - }, - { - Kind: "DaemonSet", - }, + Labels: map[string]string{ + constants.ModuleNameLabel: "module name", + constants.DaemonSetRole: constants.DevicePluginRoleLabelValue, }, }, } - pod2 := v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{constants.ModuleNameLabel: "other module name"}, - }, - } - fieldSelector := client.MatchingFields{"spec.nodeName": nodeName} - labelSelector := client.HasLabels{constants.ModuleNameLabel} - kubeClient.EXPECT().List(ctx, gomock.Any(), labelSelector, fieldSelector).DoAndReturn( + kubeClient.EXPECT().List(ctx, gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( func(_ interface{}, list *v1.PodList, _ ...interface{}) error { list.Items = append(list.Items, pod1) - list.Items = append(list.Items, pod2) return nil }, ) - res, err := helper.getDevicePluginPods(ctx, nodeName) + res, err := helper.getSchedulePluginPods(ctx, nodeName) Expect(err).ToNot(HaveOccurred()) Expect(len(res)).To(Equal(1)) Expect(res[0]).To(Equal(pod1)) }) It("error flow", func() { - fieldSelector := client.MatchingFields{"spec.nodeName": nodeName} - labelSelector := client.HasLabels{constants.ModuleNameLabel} - kubeClient.EXPECT().List(ctx, gomock.Any(), labelSelector, fieldSelector).Return(fmt.Errorf("some error")) + kubeClient.EXPECT().List(ctx, gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error")) - res, err := helper.getDevicePluginPods(ctx, nodeName) + res, err := helper.getSchedulePluginPods(ctx, nodeName) Expect(err).To(HaveOccurred()) Expect(res).To(BeNil()) }) @@ -208,14 +203,14 @@ var _ = Describe("getDevicePluginPods", func() { }) var _ = Describe("getLabelAndAction", func() { - DescribeTable("reconciler flow", func(moduleVersionValue, workerPodVersionValue, devicePluginVersionValue string, + DescribeTable("should return correct label and action", func(moduleVersionValue, workerPodVersionValue, schedulePluginVersionValue string, expectedLabelFunc func(string, string) string, expectedLabelValue, expectedAction string) { moduleLabels := &modulesVersionLabels{ - name: "moduleName", - namespace: "moduleNamespace", - moduleVersionLabel: moduleVersionValue, - workerPodVersionLabel: workerPodVersionValue, - devicePluginVersionLabel: devicePluginVersionValue, + name: "moduleName", + namespace: "moduleNamespace", + moduleVersionLabel: moduleVersionValue, + workerPodVersionLabel: workerPodVersionValue, + schedulePluginVersionLabel: schedulePluginVersionValue, } expectedLabel := "" if expectedLabelFunc != nil { @@ -229,13 +224,13 @@ var _ = Describe("getLabelAndAction", func() { Expect(action).To(Equal(expectedAction)) }, Entry("all labels present with the same label", "1", "1", "1", nil, "", noneAction), - Entry("module version missing, worker pod present, device plugin present", "", "1", "1", utils.GetDevicePluginVersionLabelName, "", deleteAction), - Entry("module version missing, worker pod present, device plugin missing", "", "1", "", utils.GetWorkerPodVersionLabelName, "", deleteAction), + Entry("module version missing, worker pod present, schedule plugin present", "", "1", "1", utils.GetSchedulePluginVersionLabelName, "", deleteAction), + Entry("module version missing, worker pod present, schedule plugin missing", "", "1", "", utils.GetWorkerPodVersionLabelName, "", deleteAction), Entry("all labels missing", "", "", "", nil, "", noneAction), - Entry("module version present, worker pod missing, device plugin missing", "1", "", "", utils.GetWorkerPodVersionLabelName, "1", addAction), - Entry("module version present, worker pod present, device plugin missing", "1", "1", "", utils.GetDevicePluginVersionLabelName, "1", addAction), - Entry("module version present, worker pod different, device plugin different", "2", "1", "1", utils.GetDevicePluginVersionLabelName, "", deleteAction), - Entry("module version present, worker pod different, device plugin missing", "2", "1", "", utils.GetWorkerPodVersionLabelName, "", deleteAction), + Entry("module version present, worker pod missing, schedule plugin missing", "1", "", "", utils.GetWorkerPodVersionLabelName, "1", addAction), + Entry("module version present, worker pod present, schedule plugin missing", "1", "1", "", utils.GetSchedulePluginVersionLabelName, "1", addAction), + Entry("module version present, worker pod different, schedule plugin different", "2", "1", "1", utils.GetSchedulePluginVersionLabelName, "", deleteAction), + Entry("module version present, worker pod different, schedule plugin missing", "2", "1", "", utils.GetWorkerPodVersionLabelName, "", deleteAction), ) }) @@ -252,29 +247,29 @@ var _ = Describe("reconcileLabels", func() { ObjectMeta: metav1.ObjectMeta{Namespace: "moduleNamespace"}, } - It("delete device-plugin label, device plugin pod not present", func() { + It("delete schedule plugin label, schedule plugin pod not present", func() { moduleLabels := &modulesVersionLabels{ - name: "moduleName", - namespace: "moduleNamespace", - moduleVersionLabel: "", - workerPodVersionLabel: "1", - devicePluginVersionLabel: "1", + name: "moduleName", + namespace: "moduleNamespace", + moduleVersionLabel: "", + workerPodVersionLabel: "1", + schedulePluginVersionLabel: "1", } res := helper.reconcileLabels(map[string]*modulesVersionLabels{"key": moduleLabels}, []v1.Pod{pod}, nil) Expect(res.requeue).To(BeFalse()) Expect(len(res.labelsToAdd)).To(Equal(0)) - Expect(res.labelsToDelete).To(Equal([]string{utils.GetDevicePluginVersionLabelName("moduleNamespace", "moduleName")})) + Expect(res.labelsToDelete).To(Equal([]string{utils.GetSchedulePluginVersionLabelName("moduleNamespace", "moduleName")})) }) - It("delete worker pod label, device plugin pod not present", func() { + It("delete worker pod label, schedule plugin pod not present", func() { moduleLabels := &modulesVersionLabels{ - name: "moduleName", - namespace: "moduleNamespace", - moduleVersionLabel: "", - workerPodVersionLabel: "1", - devicePluginVersionLabel: "", + name: "moduleName", + namespace: "moduleNamespace", + moduleVersionLabel: "", + workerPodVersionLabel: "1", + schedulePluginVersionLabel: "", } res := helper.reconcileLabels(map[string]*modulesVersionLabels{"key": moduleLabels}, []v1.Pod{pod}, nil) @@ -284,30 +279,30 @@ var _ = Describe("reconcileLabels", func() { Expect(res.labelsToDelete).To(Equal([]string{utils.GetWorkerPodVersionLabelName("moduleNamespace", "moduleName")})) }) - It("delete device-plugin label, device plugin pod present", func() { + It("delete schedule plugin label, schedule plugin pod present", func() { moduleLabels := &modulesVersionLabels{ - name: "moduleName", - namespace: "moduleNamespace", - moduleVersionLabel: "", - workerPodVersionLabel: "1", - devicePluginVersionLabel: "1", + name: "moduleName", + namespace: "moduleNamespace", + moduleVersionLabel: "", + workerPodVersionLabel: "1", + schedulePluginVersionLabel: "1", } pod.SetLabels(map[string]string{constants.ModuleNameLabel: "moduleName"}) res := helper.reconcileLabels(map[string]*modulesVersionLabels{"key": moduleLabels}, []v1.Pod{pod}, nil) Expect(res.requeue).To(BeFalse()) - Expect(res.labelsToDelete).To(Equal([]string{utils.GetDevicePluginVersionLabelName("moduleNamespace", "moduleName")})) + Expect(res.labelsToDelete).To(Equal([]string{utils.GetSchedulePluginVersionLabelName("moduleNamespace", "moduleName")})) Expect(len(res.labelsToAdd)).To(Equal(0)) }) - It("delete worker pod label,device plugin pod present", func() { + It("delete worker pod label, schedule plugin pod present", func() { moduleLabels := &modulesVersionLabels{ - name: "moduleName", - namespace: "moduleNamespace", - moduleVersionLabel: "", - workerPodVersionLabel: "1", - devicePluginVersionLabel: "", + name: "moduleName", + namespace: "moduleNamespace", + moduleVersionLabel: "", + workerPodVersionLabel: "1", + schedulePluginVersionLabel: "", } pod.SetLabels(map[string]string{constants.ModuleNameLabel: "moduleName"}) @@ -320,11 +315,11 @@ var _ = Describe("reconcileLabels", func() { It("add worker pod label, kernel module is not loaded", func() { moduleLabels := &modulesVersionLabels{ - name: "moduleName", - namespace: "moduleNamespace", - moduleVersionLabel: "1", - workerPodVersionLabel: "", - devicePluginVersionLabel: "", + name: "moduleName", + namespace: "moduleNamespace", + moduleVersionLabel: "1", + workerPodVersionLabel: "", + schedulePluginVersionLabel: "", } res := helper.reconcileLabels(map[string]*modulesVersionLabels{"key": moduleLabels}, []v1.Pod{pod}, nil) @@ -336,11 +331,11 @@ var _ = Describe("reconcileLabels", func() { It("add worker pod label, kernel module is loaded", func() { moduleLabels := &modulesVersionLabels{ - name: "moduleName", - namespace: "moduleNamespace", - moduleVersionLabel: "1", - workerPodVersionLabel: "", - devicePluginVersionLabel: "", + name: "moduleName", + namespace: "moduleNamespace", + moduleVersionLabel: "1", + workerPodVersionLabel: "", + schedulePluginVersionLabel: "", } loadedKernelModules := []types.NamespacedName{{Name: "moduleName", Namespace: "moduleNamespace"}} @@ -352,29 +347,29 @@ var _ = Describe("reconcileLabels", func() { Expect(len(res.labelsToAdd)).To(Equal(0)) }) - It("add device-plugin label, kernel module is not loaded", func() { + It("add schedule plugin label, kernel module is not loaded", func() { moduleLabels := &modulesVersionLabels{ - name: "moduleName", - namespace: "moduleNamespace", - moduleVersionLabel: "1", - workerPodVersionLabel: "1", - devicePluginVersionLabel: "", + name: "moduleName", + namespace: "moduleNamespace", + moduleVersionLabel: "1", + workerPodVersionLabel: "1", + schedulePluginVersionLabel: "", } res := helper.reconcileLabels(map[string]*modulesVersionLabels{"key": moduleLabels}, []v1.Pod{pod}, nil) Expect(res.requeue).To(BeFalse()) - Expect(res.labelsToAdd).To(Equal(map[string]string{utils.GetDevicePluginVersionLabelName("moduleNamespace", "moduleName"): "1"})) + Expect(res.labelsToAdd).To(Equal(map[string]string{utils.GetSchedulePluginVersionLabelName("moduleNamespace", "moduleName"): "1"})) Expect(len(res.labelsToDelete)).To(Equal(0)) }) - It("add device-plugin label, kernel module is loaded", func() { + It("add schedule plugin label, kernel module is loaded", func() { moduleLabels := &modulesVersionLabels{ - name: "moduleName", - namespace: "moduleNamespace", - moduleVersionLabel: "1", - workerPodVersionLabel: "1", - devicePluginVersionLabel: "", + name: "moduleName", + namespace: "moduleNamespace", + moduleVersionLabel: "1", + workerPodVersionLabel: "1", + schedulePluginVersionLabel: "", } loadedKernelModules := []types.NamespacedName{{Name: "moduleName", Namespace: "moduleNamespace"}} @@ -382,17 +377,17 @@ var _ = Describe("reconcileLabels", func() { res := helper.reconcileLabels(map[string]*modulesVersionLabels{"key": moduleLabels}, []v1.Pod{pod}, loadedKernelModules) Expect(res.requeue).To(BeFalse()) - Expect(res.labelsToAdd).To(Equal(map[string]string{utils.GetDevicePluginVersionLabelName("moduleNamespace", "moduleName"): "1"})) + Expect(res.labelsToAdd).To(Equal(map[string]string{utils.GetSchedulePluginVersionLabelName("moduleNamespace", "moduleName"): "1"})) Expect(len(res.labelsToDelete)).To(Equal(0)) }) It("no label needs to be added due to none action", func() { moduleLabels := &modulesVersionLabels{ - name: "moduleName", - namespace: "moduleNamespace", - moduleVersionLabel: "1", - workerPodVersionLabel: "1", - devicePluginVersionLabel: "1", + name: "moduleName", + namespace: "moduleNamespace", + moduleVersionLabel: "1", + workerPodVersionLabel: "1", + schedulePluginVersionLabel: "1", } pod := v1.Pod{} pod.Namespace = "moduleNamespace" @@ -406,7 +401,7 @@ var _ = Describe("reconcileLabels", func() { }) }) -var _ = Describe("reconcileLabels", func() { +var _ = Describe("updateNodeLabels", func() { var ( kubeClient *mock_client.MockClient helper nodeLabelModuleVersionHelperAPI diff --git a/internal/utils/kmmlabels.go b/internal/utils/kmmlabels.go index 1318384d7..8b0982ec4 100644 --- a/internal/utils/kmmlabels.go +++ b/internal/utils/kmmlabels.go @@ -23,8 +23,8 @@ func GetWorkerPodVersionLabelName(namespace, name string) string { return fmt.Sprintf("%s.%s.%s", constants.WorkerPodVersionLabelPrefix, namespace, name) } -func GetDevicePluginVersionLabelName(namespace, name string) string { - return fmt.Sprintf("%s.%s.%s", constants.DevicePluginVersionLabelPrefix, namespace, name) +func GetSchedulePluginVersionLabelName(namespace, name string) string { + return fmt.Sprintf("%s.%s.%s", constants.SchedulePluginVersionLabelPrefix, namespace, name) } func GetNamespaceNameFromVersionLabel(label string) (string, string, error) { @@ -36,7 +36,7 @@ func GetNamespaceNameFromVersionLabel(label string) (string, string, error) { } func IsVersionLabel(label string) bool { - return IsModuleVersionLabel(label) || IsWorkerPodVersionLabel(label) || IsDevicePluginVersionLabel(label) + return IsModuleVersionLabel(label) || IsWorkerPodVersionLabel(label) || IsSchedulePluginVersionLabel(label) } func IsModuleVersionLabel(label string) bool { @@ -47,16 +47,14 @@ func IsWorkerPodVersionLabel(label string) bool { return strings.HasPrefix(label, constants.WorkerPodVersionLabelPrefix) } -func IsDevicePluginVersionLabel(label string) bool { - return strings.HasPrefix(label, constants.DevicePluginVersionLabelPrefix) +func IsSchedulePluginVersionLabel(label string) bool { + return strings.HasPrefix(label, constants.SchedulePluginVersionLabelPrefix) } func GetNodesVersionLabels(nodeLabels map[string]string) map[string]string { versionLabels := map[string]string{} for label, labelValue := range nodeLabels { - if strings.HasPrefix(label, constants.WorkerPodVersionLabelPrefix) || - strings.HasPrefix(label, constants.DevicePluginVersionLabelPrefix) || - strings.HasPrefix(label, constants.ModuleVersionLabelPrefix) { + if IsVersionLabel(label) { versionLabels[label] = labelValue } } diff --git a/internal/utils/kmmlabels_test.go b/internal/utils/kmmlabels_test.go index 50cf4d907..5c1a67b9a 100644 --- a/internal/utils/kmmlabels_test.go +++ b/internal/utils/kmmlabels_test.go @@ -19,10 +19,10 @@ var _ = Describe("GetWorkerPodVersionLabelName", func() { }) }) -var _ = Describe("GetDevicePluginVersionLabelName", func() { +var _ = Describe("GetSchedulePluginVersionLabelName", func() { It("should work as expected", func() { - res := GetDevicePluginVersionLabelName("some-namespace", "some-name") - Expect(res).To(Equal("beta.kmm.node.kubernetes.io/version-device-plugin.some-namespace.some-name")) + res := GetSchedulePluginVersionLabelName("some-namespace", "some-name") + Expect(res).To(Equal("beta.kmm.node.kubernetes.io/version-schedule-plugin.some-namespace.some-name")) }) }) @@ -33,6 +33,18 @@ var _ = Describe("GetDRANodeLabel", func() { }) }) +var _ = Describe("IsSchedulePluginVersionLabel", func() { + DescribeTable("should work as expected", + func(input string, expected bool) { + Expect(IsSchedulePluginVersionLabel(input)).To(Equal(expected)) + }, + Entry("schedule plugin version label", "beta.kmm.node.kubernetes.io/version-schedule-plugin.ns.name", true), + Entry("worker pod version label", "beta.kmm.node.kubernetes.io/version-worker-pod.ns.name", false), + Entry("module version label", "kmm.node.kubernetes.io/version-module.ns.name", false), + Entry("unrelated label", "some-other-label", false), + ) +}) + var _ = Describe("GetNamespaceNameFromVersionLabel", func() { DescribeTable("should return correct name and namespace", func(versionLabel, expectedNamespace, expectedName string, expectsErr bool) { @@ -47,7 +59,7 @@ var _ = Describe("GetNamespaceNameFromVersionLabel", func() { Expect(name).To(Equal(expectedName)) }, Entry("workerPod label", "beta.kmm.node.kubernetes.io/version-worker-pod.some-namespace.some-name", "some-namespace", "some-name", false), - Entry("devicePlugin label", "beta.kmm.node.kubernetes.io/version-device-plugin.some-namespace.some-name", "some-namespace", "some-name", false), + Entry("schedule plugin label", "beta.kmm.node.kubernetes.io/version-schedule-plugin.some-namespace.some-name", "some-namespace", "some-name", false), Entry("module label", "kmm.node.kubernetes.io/version-module.some-namespace.some-name", "some-namespace", "some-name", false), Entry("with error", "version-module-some-namespace-some-name", "some-namespace", "some-name", true), ) diff --git a/internal/webhook/module.go b/internal/webhook/module.go index 95ca00901..e2e3b1f7a 100644 --- a/internal/webhook/module.go +++ b/internal/webhook/module.go @@ -39,7 +39,8 @@ import ( ) // maxCombinedLength is the maximum combined length of Module name and namespace when the version field is set. -const maxCombinedLength = 40 +// 63 (max label key length after slash) - len("version-schedule-plugin.") = 38 +const maxCombinedLength = 38 var kubeVersionRe = regexp.MustCompile(`^v?(\d+)\.(\d+)`) diff --git a/internal/webhook/module_test.go b/internal/webhook/module_test.go index 57048804b..4b1bbc66a 100644 --- a/internal/webhook/module_test.go +++ b/internal/webhook/module_test.go @@ -68,7 +68,7 @@ var _ = Describe("maxCombinedLength", func() { baseLength = l } - if l := getLengthAfterSlash(utils.GetDevicePluginVersionLabelName("", "")); l > baseLength { + if l := getLengthAfterSlash(utils.GetSchedulePluginVersionLabelName("", "")); l > baseLength { baseLength = l }