diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go index 20c6d6841..03f870399 100644 --- a/pkg/nfd-master/nfd-master-internal_test.go +++ b/pkg/nfd-master/nfd-master-internal_test.go @@ -17,7 +17,9 @@ limitations under the License. package nfdmaster import ( + "errors" "fmt" + "maps" "os" "path/filepath" "regexp" @@ -26,56 +28,43 @@ import ( "testing" "time" - "github.com/smarty/assertions" . "github.com/smartystreets/goconvey/convey" - "github.com/stretchr/testify/mock" - "github.com/vektra/errors" "golang.org/x/net/context" + corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" - meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" k8sclient "k8s.io/client-go/kubernetes" + fakeclient "k8s.io/client-go/kubernetes/fake" + fakecorev1client "k8s.io/client-go/kubernetes/typed/core/v1/fake" + clienttesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" - "sigs.k8s.io/node-feature-discovery/pkg/apihelper" + nfdv1alpha1 "sigs.k8s.io/node-feature-discovery/pkg/apis/nfd/v1alpha1" - "sigs.k8s.io/node-feature-discovery/pkg/generated/clientset/versioned/fake" + fakenfdclient "sigs.k8s.io/node-feature-discovery/pkg/generated/clientset/versioned/fake" nfdscheme "sigs.k8s.io/node-feature-discovery/pkg/generated/clientset/versioned/scheme" nfdinformers "sigs.k8s.io/node-feature-discovery/pkg/generated/informers/externalversions" "sigs.k8s.io/node-feature-discovery/pkg/labeler" "sigs.k8s.io/node-feature-discovery/pkg/utils" - "sigs.k8s.io/yaml" ) const ( - mockNodeName = "mock-node" + testNodeName = "mock-node" ) -func newMockNode() *corev1.Node { +func newTestNode() *corev1.Node { n := corev1.Node{} - n.Name = mockNodeName + n.Name = testNodeName n.Labels = map[string]string{} n.Annotations = map[string]string{} - n.Status.Capacity = corev1.ResourceList{} + n.Status.Capacity = corev1.ResourceList{"cpu": resource.MustParse("2")} return &n } -func mockNodeList() *corev1.NodeList { - l := corev1.NodeList{} - - for i := 0; i < 1000; i++ { - n := corev1.Node{} - n.Name = fmt.Sprintf("node %v", i) - n.Labels = map[string]string{} - n.Annotations = map[string]string{} - n.Status.Capacity = corev1.ResourceList{} - - l.Items = append(l.Items, n) - } - return &l -} - -func newMockNfdAPIController(client *fake.Clientset) *nfdController { +func newFakeNfdAPIController(client *fakenfdclient.Clientset) *nfdController { c := &nfdController{ stopChan: make(chan struct{}, 1), updateAllNodesChan: make(chan struct{}, 1), @@ -114,123 +103,102 @@ func newMockNfdAPIController(client *fake.Clientset) *nfdController { return c } -func newMockMaster(apihelper apihelper.APIHelpers) *nfdMaster { +func newFakeMaster(cli k8sclient.Interface) *nfdMaster { return &nfdMaster{ - nodeName: mockNodeName, + nodeName: testNodeName, config: &NFDConfig{LabelWhiteList: utils.RegexpVal{Regexp: *regexp.MustCompile("")}}, - apihelper: apihelper, + k8sClient: cli, } } func TestUpdateNodeObject(t *testing.T) { Convey("When I update the node using fake client", t, func() { - fakeFeatureLabels := map[string]string{ + featureLabels := map[string]string{ nfdv1alpha1.FeatureLabelNs + "/source-feature.1": "1", nfdv1alpha1.FeatureLabelNs + "/source-feature.2": "2", nfdv1alpha1.FeatureLabelNs + "/source-feature.3": "val3", - nfdv1alpha1.ProfileLabelNs + "/profile-a": "val4"} - fakeAnnotations := map[string]string{"my-annotation": "my-val"} - fakeExtResources := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/source-feature.1": "1", nfdv1alpha1.FeatureLabelNs + "/source-feature.2": "2"} - - fakeFeatureLabelNames := make([]string, 0, len(fakeFeatureLabels)) - for k := range fakeFeatureLabels { - fakeFeatureLabelNames = append(fakeFeatureLabelNames, strings.TrimPrefix(k, nfdv1alpha1.FeatureLabelNs+"/")) + nfdv1alpha1.ProfileLabelNs + "/profile-a": "val4", } - sort.Strings(fakeFeatureLabelNames) - - fakeFeatureAnnotationsNames := make([]string, 0, len(fakeFeatureLabels)) - for k := range fakeAnnotations { - fakeFeatureAnnotationsNames = append(fakeFeatureAnnotationsNames, strings.TrimPrefix(k, nfdv1alpha1.FeatureAnnotationNs+"/")) + featureAnnotations := map[string]string{ + "feature.node.kubernetesl.io/my-annotation": "my-val", } - sort.Strings(fakeFeatureAnnotationsNames) - - fakeExtResourceNames := make([]string, 0, len(fakeExtResources)) - for k := range fakeExtResources { - fakeExtResourceNames = append(fakeExtResourceNames, strings.TrimPrefix(k, nfdv1alpha1.FeatureLabelNs+"/")) - } - sort.Strings(fakeExtResourceNames) - - // Create a list of expected node status patches - statusPatches := []utils.JsonPatch{} - for k, v := range fakeExtResources { - statusPatches = append(statusPatches, utils.NewJsonPatch("add", "/status/capacity", k, v)) + featureExtResources := map[string]string{ + nfdv1alpha1.FeatureLabelNs + "/source-feature.1": "1", + nfdv1alpha1.FeatureLabelNs + "/source-feature.2": "2", } - mockAPIHelper := new(apihelper.MockAPIHelpers) - mockMaster := newMockMaster(mockAPIHelper) - mockClient := &k8sclient.Clientset{} - // Mock node with old features - mockNode := newMockNode() - mockNode.Labels[nfdv1alpha1.FeatureLabelNs+"/old-feature"] = "old-value" - mockNode.Annotations[nfdv1alpha1.AnnotationNs+"/feature-labels"] = "old-feature" + featureLabelNames := make([]string, 0, len(featureLabels)) + for k := range featureLabels { + featureLabelNames = append(featureLabelNames, strings.TrimPrefix(k, nfdv1alpha1.FeatureLabelNs+"/")) + } + sort.Strings(featureLabelNames) + + featureAnnotationNames := make([]string, 0, len(featureLabels)) + for k := range featureAnnotations { + featureAnnotationNames = append(featureAnnotationNames, strings.TrimPrefix(k, nfdv1alpha1.FeatureAnnotationNs+"/")) + } + sort.Strings(featureAnnotationNames) + + featureExtResourceNames := make([]string, 0, len(featureExtResources)) + for k := range featureExtResources { + featureExtResourceNames = append(featureExtResourceNames, strings.TrimPrefix(k, nfdv1alpha1.FeatureLabelNs+"/")) + } + sort.Strings(featureExtResourceNames) + + // Create a node with some existing features + testNode := newTestNode() + testNode.Labels[nfdv1alpha1.FeatureLabelNs+"/old-feature"] = "old-value" + testNode.Annotations[nfdv1alpha1.AnnotationNs+"/feature-labels"] = "old-feature" + + // Create fake api client and initialize NfdMaster instance + fakeCli := fakeclient.NewSimpleClientset(testNode) + fakeMaster := newFakeMaster(fakeCli) Convey("When I successfully update the node with feature labels", func() { - // Create a list of expected node metadata patches - metadataPatches := []utils.JsonPatch{ - utils.NewJsonPatch("replace", "/metadata/annotations", nfdv1alpha1.AnnotationNs+"/feature-labels", strings.Join(fakeFeatureLabelNames, ",")), - utils.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureAnnotationsTrackingAnnotation, strings.Join(fakeFeatureAnnotationsNames, ",")), - utils.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.AnnotationNs+"/extended-resources", strings.Join(fakeExtResourceNames, ",")), - utils.NewJsonPatch("remove", "/metadata/labels", nfdv1alpha1.FeatureLabelNs+"/old-feature", ""), - } - for k, v := range fakeFeatureLabels { - metadataPatches = append(metadataPatches, utils.NewJsonPatch("add", "/metadata/labels", k, v)) - } - for k, v := range fakeAnnotations { - metadataPatches = append(metadataPatches, utils.NewJsonPatch("add", "/metadata/annotations", k, v)) - } - - mockAPIHelper.On("GetClient").Return(mockClient, nil) - mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Twice() - mockAPIHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(statusPatches))).Return(nil) - mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(metadataPatches))).Return(nil) - err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil) - + err := fakeMaster.updateNodeObject(testNodeName, featureLabels, featureAnnotations, featureExtResources, nil) Convey("Error is nil", func() { So(err, ShouldBeNil) }) - }) - Convey("When I fail to update the node with feature labels", func() { - expectedError := fmt.Errorf("no client is passed, client: ") - mockAPIHelper.On("GetClient").Return(nil, expectedError) - err := mockMaster.updateNodeObject(nil, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil) + Convey("Node object is updated", func() { + expectedAnnotations := map[string]string{ + nfdv1alpha1.FeatureLabelsAnnotation: strings.Join(featureLabelNames, ","), + nfdv1alpha1.FeatureAnnotationsTrackingAnnotation: strings.Join(featureAnnotationNames, ","), + nfdv1alpha1.ExtendedResourceAnnotation: strings.Join(featureExtResourceNames, ","), + } + maps.Copy(expectedAnnotations, featureAnnotations) - Convey("Error is produced", func() { - So(err, ShouldResemble, expectedError) + expectedCapacity := testNode.Status.Capacity.DeepCopy() + for k, v := range featureExtResources { + expectedCapacity[v1.ResourceName(k)] = resource.MustParse(v) + } + + // Get the node + updatedNode, err := fakeCli.CoreV1().Nodes().Get(context.TODO(), testNodeName, metav1.GetOptions{}) + + So(err, ShouldBeNil) + So(updatedNode.Labels, ShouldEqual, featureLabels) + So(updatedNode.Annotations, ShouldEqual, expectedAnnotations) + So(updatedNode.Status.Capacity, ShouldEqual, expectedCapacity) }) }) - Convey("When I fail to get a mock client while updating feature labels", func() { - expectedError := fmt.Errorf("no client is passed, client: ") - mockAPIHelper.On("GetClient").Return(nil, expectedError) - err := mockMaster.updateNodeObject(nil, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil) + Convey("When I fail to get a node while updating feature labels", func() { + err := fakeMaster.updateNodeObject("non-existent-node", featureLabels, featureAnnotations, featureExtResources, nil) Convey("Error is produced", func() { - So(err, ShouldResemble, expectedError) + So(err, ShouldBeError) }) }) - Convey("When I fail to get a mock node while updating feature labels", func() { - expectedError := errors.New("fake error") - mockAPIHelper.On("GetClient").Return(mockClient, nil) - mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(nil, expectedError).Twice() - err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil) - - Convey("Error is produced", func() { - So(err, ShouldEqual, expectedError) + Convey("When I fail to patch a node", func() { + fakeCli.CoreV1().(*fakecorev1client.FakeCoreV1).PrependReactor("patch", "nodes", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { + return true, &v1.Node{}, errors.New("Fake error when patching node") }) - }) - - Convey("When I fail to update a mock node while updating feature labels", func() { - expectedError := errors.New("fake error") - mockAPIHelper.On("GetClient").Return(mockClient, nil) - mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Twice() - mockAPIHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(statusPatches))).Return(nil) - mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.Anything).Return(expectedError).Twice() - err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil) + err := fakeMaster.updateNodeObject(testNodeName, nil, featureAnnotations, ExtendedResources{"": ""}, nil) Convey("Error is produced", func() { - So(err.Error(), ShouldEndWith, expectedError.Error()) + So(err, ShouldBeError) }) }) @@ -239,46 +207,46 @@ func TestUpdateNodeObject(t *testing.T) { func TestUpdateMasterNode(t *testing.T) { Convey("When updating the nfd-master node", t, func() { - mockHelper := &apihelper.MockAPIHelpers{} - mockMaster := newMockMaster(mockHelper) - mockClient := &k8sclient.Clientset{} - mockNode := newMockNode() + testNode := newTestNode() + testNode.Annotations["nfd.node.kubernetes.io/master.version"] = "foo" + Convey("When update operation succeeds", func() { - expectedPatches := []utils.JsonPatch{} - mockHelper.On("GetClient").Return(mockClient, nil) - mockHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil) - mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) - err := mockMaster.updateMasterNode() + fakeCli := fakeclient.NewSimpleClientset(testNode) + fakeMaster := newFakeMaster(fakeCli) + err := fakeMaster.updateMasterNode() Convey("No error should be returned", func() { So(err, ShouldBeNil) }) - }) - mockErr := errors.New("failed to patch node annotations: mock-error'") - Convey("When getting API client fails", func() { - mockHelper.On("GetClient").Return(mockClient, mockErr) - err := mockMaster.updateMasterNode() - Convey("An error should be returned", func() { - So(err, ShouldEqual, mockErr) + Convey("Master version annotation was removed", func() { + updatedNode, err := fakeCli.CoreV1().Nodes().Get(context.TODO(), testNodeName, metav1.GetOptions{}) + So(err, ShouldBeNil) + So(updatedNode.Annotations, ShouldBeEmpty) }) }) Convey("When getting API node object fails", func() { - mockHelper.On("GetClient").Return(mockClient, nil) - mockHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, mockErr) - err := mockMaster.updateMasterNode() + fakeCli := fakeclient.NewSimpleClientset(testNode) + fakeMaster := newFakeMaster(fakeCli) + fakeMaster.nodeName = "does-not-exist" + + err := fakeMaster.updateMasterNode() Convey("An error should be returned", func() { - So(err, ShouldEqual, mockErr) + So(err, ShouldBeError) }) }) Convey("When updating node object fails", func() { - mockHelper.On("GetClient").Return(mockClient, nil) - mockHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil) - mockHelper.On("PatchNode", mockClient, mockNodeName, mock.Anything).Return(mockErr) - err := mockMaster.updateMasterNode() + fakeErr := errors.New("Fake error when patching node") + fakeCli := fakeclient.NewSimpleClientset(testNode) + fakeCli.CoreV1().(*fakecorev1client.FakeCoreV1).PrependReactor("patch", "nodes", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { + return true, &v1.Node{}, fakeErr + }) + fakeMaster := newFakeMaster(fakeCli) + + err := fakeMaster.updateMasterNode() Convey("An error should be returned", func() { - So(err.Error(), ShouldEndWith, mockErr.Error()) + So(err, ShouldWrap, fakeErr) }) }) }) @@ -286,42 +254,42 @@ func TestUpdateMasterNode(t *testing.T) { func TestAddingExtResources(t *testing.T) { Convey("When adding extended resources", t, func() { - mockMaster := newMockMaster(nil) + fakeMaster := newFakeMaster(nil) Convey("When there are no matching labels", func() { - mockNode := newMockNode() - mockResourceLabels := ExtendedResources{} - patches := mockMaster.createExtendedResourcePatches(mockNode, mockResourceLabels) + testNode := newTestNode() + resourceLabels := ExtendedResources{} + patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels) So(len(patches), ShouldEqual, 0) }) Convey("When there are matching labels", func() { - mockNode := newMockNode() - mockResourceLabels := ExtendedResources{"feature-1": "1", "feature-2": "2"} + testNode := newTestNode() + resourceLabels := ExtendedResources{"feature-1": "1", "feature-2": "2"} expectedPatches := []utils.JsonPatch{ utils.NewJsonPatch("add", "/status/capacity", "feature-1", "1"), utils.NewJsonPatch("add", "/status/capacity", "feature-2", "2"), } - patches := mockMaster.createExtendedResourcePatches(mockNode, mockResourceLabels) + patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels) So(sortJsonPatches(patches), ShouldResemble, sortJsonPatches(expectedPatches)) }) Convey("When the resource already exists", func() { - mockNode := newMockNode() - mockNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI) - mockResourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-1": "1"} - patches := mockMaster.createExtendedResourcePatches(mockNode, mockResourceLabels) + testNode := newTestNode() + testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI) + resourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-1": "1"} + patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels) So(len(patches), ShouldEqual, 0) }) Convey("When the resource already exists but its capacity has changed", func() { - mockNode := newMockNode() - mockNode.Status.Capacity[corev1.ResourceName("feature-1")] = *resource.NewQuantity(2, resource.BinarySI) - mockResourceLabels := ExtendedResources{"feature-1": "1"} + testNode := newTestNode() + testNode.Status.Capacity[corev1.ResourceName("feature-1")] = *resource.NewQuantity(2, resource.BinarySI) + resourceLabels := ExtendedResources{"feature-1": "1"} expectedPatches := []utils.JsonPatch{ utils.NewJsonPatch("replace", "/status/capacity", "feature-1", "1"), utils.NewJsonPatch("replace", "/status/allocatable", "feature-1", "1"), } - patches := mockMaster.createExtendedResourcePatches(mockNode, mockResourceLabels) + patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels) So(sortJsonPatches(patches), ShouldResemble, sortJsonPatches(expectedPatches)) }) }) @@ -329,32 +297,32 @@ func TestAddingExtResources(t *testing.T) { func TestRemovingExtResources(t *testing.T) { Convey("When removing extended resources", t, func() { - mockMaster := newMockMaster(nil) + fakeMaster := newFakeMaster(nil) Convey("When none are removed", func() { - mockNode := newMockNode() - mockResourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-1": "1", nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"} - mockNode.Annotations[nfdv1alpha1.AnnotationNs+"/extended-resources"] = "feature-1,feature-2" - mockNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI) - mockNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI) - patches := mockMaster.createExtendedResourcePatches(mockNode, mockResourceLabels) + testNode := newTestNode() + resourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-1": "1", nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"} + testNode.Annotations[nfdv1alpha1.AnnotationNs+"/extended-resources"] = "feature-1,feature-2" + testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI) + testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI) + patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels) So(len(patches), ShouldEqual, 0) }) Convey("When the related label is gone", func() { - mockNode := newMockNode() - mockResourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-4": "", nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"} - mockNode.Annotations[nfdv1alpha1.AnnotationNs+"/extended-resources"] = "feature-4,feature-2" - mockNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-4")] = *resource.NewQuantity(4, resource.BinarySI) - mockNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI) - patches := mockMaster.createExtendedResourcePatches(mockNode, mockResourceLabels) + testNode := newTestNode() + resourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-4": "", nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"} + testNode.Annotations[nfdv1alpha1.AnnotationNs+"/extended-resources"] = "feature-4,feature-2" + testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-4")] = *resource.NewQuantity(4, resource.BinarySI) + testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI) + patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels) So(len(patches), ShouldBeGreaterThan, 0) }) Convey("When the extended resource is no longer wanted", func() { - mockNode := newMockNode() - mockNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI) - mockNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI) - mockResourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"} - mockNode.Annotations[nfdv1alpha1.AnnotationNs+"/extended-resources"] = "feature-1,feature-2" - patches := mockMaster.createExtendedResourcePatches(mockNode, mockResourceLabels) + testNode := newTestNode() + testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI) + testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI) + resourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"} + testNode.Annotations[nfdv1alpha1.AnnotationNs+"/extended-resources"] = "feature-1,feature-2" + patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels) So(len(patches), ShouldBeGreaterThan, 0) }) }) @@ -362,138 +330,75 @@ func TestRemovingExtResources(t *testing.T) { func TestSetLabels(t *testing.T) { Convey("When servicing SetLabels request", t, func() { - const workerName = mockNodeName - const workerVer = "0.1-test" - mockHelper := &apihelper.MockAPIHelpers{} - mockMaster := newMockMaster(mockHelper) - mockClient := &k8sclient.Clientset{} - mockNode := newMockNode() - mockCtx := context.Background() + testNode := newTestNode() + // We need to populate the node with some annotations or the patching in the fake client fails + testNode.Labels["feature.node.kubernetes.io/foo"] = "bar" + testNode.Annotations[nfdv1alpha1.FeatureLabelsAnnotation] = "foo" + + ctx := context.Background() // In the gRPC request the label names may omit the default ns - mockLabels := map[string]string{"feature.node.kubernetes.io/feature-1": "1", "example.io/feature-2": "val-2", "feature.node.kubernetes.io/feature-3": "3"} - mockReq := &labeler.SetLabelsRequest{NodeName: workerName, NfdVersion: workerVer, Labels: mockLabels} - - mockLabelNames := make([]string, 0, len(mockLabels)) - for k := range mockLabels { - mockLabelNames = append(mockLabelNames, strings.TrimPrefix(k, nfdv1alpha1.FeatureLabelNs+"/")) + featureLabels := map[string]string{ + "feature.node.kubernetes.io/feature-1": "1", + "example.io/feature-2": "val-2", + "feature.node.kubernetes.io/feature-3": "3", } - sort.Strings(mockLabelNames) - - expectedStatusPatches := []utils.JsonPatch{} + req := &labeler.SetLabelsRequest{NodeName: testNodeName, NfdVersion: "0.1-test", Labels: featureLabels} Convey("When node update succeeds", func() { - mockMaster.config.ExtraLabelNs = map[string]struct{}{"example.io": {}} - expectedPatches := []utils.JsonPatch{ - utils.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, strings.Join(mockLabelNames, ",")), - } - for k, v := range mockLabels { - expectedPatches = append(expectedPatches, utils.NewJsonPatch("add", "/metadata/labels", k, v)) - } + fakeCli := fakeclient.NewSimpleClientset(testNode) + fakeMaster := newFakeMaster(fakeCli) - mockHelper.On("GetClient").Return(mockClient, nil) - mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil).Twice() - mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedStatusPatches))).Return(nil) - mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) - _, err := mockMaster.SetLabels(mockCtx, mockReq) + _, err := fakeMaster.SetLabels(ctx, req) Convey("No error should be returned", func() { So(err, ShouldBeNil) }) - }) - - Convey("When -label-whitelist is specified", func() { - mockMaster.config.ExtraLabelNs = map[string]struct{}{"example.io": {}} - expectedPatches := []utils.JsonPatch{ - utils.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, "example.io/feature-2"), - utils.NewJsonPatch("add", "/metadata/labels", "example.io/feature-2", mockLabels["example.io/feature-2"]), - } - - mockMaster.config.LabelWhiteList.Regexp = *regexp.MustCompile("^f.*2$") - mockHelper.On("GetClient").Return(mockClient, nil) - mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil) - mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedStatusPatches))).Return(nil) - mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) - _, err := mockMaster.SetLabels(mockCtx, mockReq) - Convey("Error is nil", func() { + Convey("Node object should be updated", func() { + updatedNode, err := fakeCli.CoreV1().Nodes().Get(context.TODO(), testNodeName, metav1.GetOptions{}) So(err, ShouldBeNil) + So(updatedNode.Labels, ShouldEqual, featureLabels) }) }) - Convey("When -extra-label-ns, -deny-label-ns and -instance are specified", func() { - // In the gRPC request the label names may omit the default ns - instance := "foo" - vendorFeatureLabel := "vendor." + nfdv1alpha1.FeatureLabelNs + "/feature-4" - vendorProfileLabel := "vendor." + nfdv1alpha1.ProfileLabelNs + "/feature-5" - mockLabels := map[string]string{ - "feature-1": "val-0", - "feature.node.kubernetes.io/feature-1": "val-1", - "valid.ns/feature-2": "val-2", - "random.denied.ns/feature-3": "val-3", - "kubernetes.io/feature-4": "val-4", - "sub.ns.kubernetes.io/feature-5": "val-5", - vendorFeatureLabel: "val-6", - vendorProfileLabel: "val-7", - "--invalid-name--": "valid-val", - "valid-name": "--invalid-val--"} - expectedPatches := []utils.JsonPatch{ - utils.NewJsonPatch("add", "/metadata/annotations", - instance+"."+nfdv1alpha1.FeatureLabelsAnnotation, - "feature-1,valid.ns/feature-2,"+vendorFeatureLabel+","+vendorProfileLabel), - utils.NewJsonPatch("add", "/metadata/labels", "feature.node.kubernetes.io/feature-1", mockLabels["feature.node.kubernetes.io/feature-1"]), - utils.NewJsonPatch("add", "/metadata/labels", "valid.ns/feature-2", mockLabels["valid.ns/feature-2"]), - utils.NewJsonPatch("add", "/metadata/labels", vendorFeatureLabel, mockLabels[vendorFeatureLabel]), - utils.NewJsonPatch("add", "/metadata/labels", vendorProfileLabel, mockLabels[vendorProfileLabel]), - } - - mockMaster.deniedNs.normal = map[string]struct{}{"random.denied.ns": {}} - mockMaster.config.ExtraLabelNs = map[string]struct{}{"valid.ns": {}} - mockMaster.args.Instance = instance - mockHelper.On("GetClient").Return(mockClient, nil) - mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil) - mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedStatusPatches))).Return(nil) - mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) - mockReq := &labeler.SetLabelsRequest{NodeName: workerName, NfdVersion: workerVer, Labels: mockLabels} - _, err := mockMaster.SetLabels(mockCtx, mockReq) - Convey("Error is nil", func() { - So(err, ShouldBeNil) - }) - mockMaster.args.Instance = "" - }) - Convey("When -resource-labels is specified", func() { - mockMaster.config.ExtraLabelNs = map[string]struct{}{"example.io": {}} - expectedPatches := []utils.JsonPatch{ - utils.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, "example.io/feature-2"), - utils.NewJsonPatch("add", "/metadata/labels", "example.io/feature-2", mockLabels["example.io/feature-2"]), - utils.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.ExtendedResourceAnnotation, "feature-1,feature-3"), - } - expectedStatusPatches := []utils.JsonPatch{ - utils.NewJsonPatch("add", "/status/capacity", "feature.node.kubernetes.io/feature-1", mockLabels["feature.node.kubernetes.io/feature-1"]), - utils.NewJsonPatch("add", "/status/capacity", "feature.node.kubernetes.io/feature-3", mockLabels["feature.node.kubernetes.io/feature-3"]), + fakeCli := fakeclient.NewSimpleClientset(testNode) + fakeMaster := newFakeMaster(fakeCli) + fakeMaster.config.ResourceLabels = map[string]struct{}{ + "feature.node.kubernetes.io/feature-3": {}, + "feature-1": {}, } - mockMaster.config.ResourceLabels = map[string]struct{}{"feature.node.kubernetes.io/feature-3": {}, "feature-1": {}} - mockHelper.On("GetClient").Return(mockClient, nil) - mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil) - mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedStatusPatches))).Return(nil) - mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) - _, err := mockMaster.SetLabels(mockCtx, mockReq) + _, err := fakeMaster.SetLabels(ctx, req) Convey("Error is nil", func() { So(err, ShouldBeNil) }) - }) - mockErr := errors.New("mock-error") - Convey("When node update fails", func() { - mockHelper.On("GetClient").Return(mockClient, mockErr) - _, err := mockMaster.SetLabels(mockCtx, mockReq) - Convey("An error should be returned", func() { - So(err, ShouldEqual, mockErr) + Convey("Node object should be updated", func() { + updatedNode, err := fakeCli.CoreV1().Nodes().Get(context.TODO(), testNodeName, metav1.GetOptions{}) + So(err, ShouldBeNil) + So(updatedNode.Labels, ShouldEqual, map[string]string{"example.io/feature-2": "val-2"}) + }) + }) + + Convey("When node update fails", func() { + fakeCli := fakeclient.NewSimpleClientset(testNode) + fakeMaster := newFakeMaster(fakeCli) + + fakeErr := errors.New("Fake error when patching node") + fakeCli.CoreV1().(*fakecorev1client.FakeCoreV1).PrependReactor("patch", "nodes", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { + return true, &v1.Node{}, fakeErr + }) + _, err := fakeMaster.SetLabels(ctx, req) + Convey("An error should be returned", func() { + So(err, ShouldWrap, fakeErr) }) }) - mockMaster.config.NoPublish = true Convey("With '-no-publish'", func() { - _, err := mockMaster.SetLabels(mockCtx, mockReq) + fakeCli := fakeclient.NewSimpleClientset(testNode) + fakeMaster := newFakeMaster(fakeCli) + + fakeMaster.config.NoPublish = true + _, err := fakeMaster.SetLabels(ctx, req) Convey("Operation should succeed", func() { So(err, ShouldBeNil) }) @@ -502,10 +407,9 @@ func TestSetLabels(t *testing.T) { } func TestFilterLabels(t *testing.T) { - mockHelper := &apihelper.MockAPIHelpers{} - mockMaster := newMockMaster(mockHelper) - mockMaster.config.ExtraLabelNs = map[string]struct{}{"example.io": {}} - mockMaster.deniedNs = deniedNs{ + fakeMaster := newFakeMaster(nil) + fakeMaster.config.ExtraLabelNs = map[string]struct{}{"example.io": {}} + fakeMaster.deniedNs = deniedNs{ normal: map[string]struct{}{"": struct{}{}, "kubernetes.io": struct{}{}, "denied.ns": struct{}{}}, wildcard: map[string]struct{}{".kubernetes.io": struct{}{}, ".denied.subns": struct{}{}}, } @@ -575,7 +479,7 @@ func TestFilterLabels(t *testing.T) { for _, tc := range tcs { t.Run(tc.description, func(t *testing.T) { - labelValue, err := mockMaster.filterFeatureLabel(tc.labelName, tc.labelValue, &tc.features) + labelValue, err := fakeMaster.filterFeatureLabel(tc.labelName, tc.labelValue, &tc.features) if tc.expectErr { Convey("Label should be filtered out", t, func() { @@ -640,7 +544,7 @@ func TestCreatePatches(t *testing.T) { func TestRemoveLabelsWithPrefix(t *testing.T) { Convey("When removing labels", t, func() { n := &corev1.Node{ - ObjectMeta: meta_v1.ObjectMeta{ + ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "single-label": "123", "multiple_A": "a", @@ -839,40 +743,35 @@ nfdApiParallelism: 100 }) } -func BenchmarkNfdAPIUpdateAllNodes(b *testing.B) { - mockAPIHelper := new(apihelper.MockAPIHelpers) - - mockMaster := newMockMaster(mockAPIHelper) - mockMaster.nfdController = newMockNfdAPIController(fake.NewSimpleClientset()) - mockMaster.config.NoPublish = true - - mockNodeUpdaterPool := newNodeUpdaterPool(mockMaster) - mockMaster.nodeUpdaterPool = mockNodeUpdaterPool - - mockClient := &k8sclient.Clientset{} - - statusPatches := []utils.JsonPatch{} - metadataPatches := []utils.JsonPatch{ - {Op: "add", Path: "/metadata/annotations/nfd.node.kubernetes.io~1feature-labels", Value: ""}, {Op: "add", Path: "/metadata/annotations/nfd.node.kubernetes.io~1extended-resources", Value: ""}, - } - - mockAPIHelper.On("GetClient").Return(mockClient, nil) - mockAPIHelper.On("GetNodes", mockClient).Return(mockNodeList(), nil) - - mockNodeUpdaterPool.start(10) +func newTestNodeList() *corev1.NodeList { + l := corev1.NodeList{} for i := 0; i < 1000; i++ { - nodeName := fmt.Sprintf("node %v", i) - node := corev1.Node{} - node.Name = nodeName - mockAPIHelper.On("GetNode", mockClient, nodeName).Return(&node, nil) - mockAPIHelper.On("PatchNodeStatus", mockClient, nodeName, mock.MatchedBy(jsonPatchMatcher(statusPatches))).Return(nil) - mockAPIHelper.On("PatchNode", mockClient, nodeName, mock.MatchedBy(jsonPatchMatcher(metadataPatches))).Return(nil) + n := corev1.Node{} + n.Name = fmt.Sprintf("node %v", i) + n.Labels = map[string]string{} + n.Annotations = map[string]string{} + n.Status.Capacity = corev1.ResourceList{} + + l.Items = append(l.Items, n) } + return &l +} + +func BenchmarkNfdAPIUpdateAllNodes(b *testing.B) { + fakeCli := fakeclient.NewSimpleClientset(newTestNodeList()) + fakeMaster := newFakeMaster(fakeCli) + fakeMaster.nfdController = newFakeNfdAPIController(fakenfdclient.NewSimpleClientset()) + + nodeUpdaterPool := newNodeUpdaterPool(fakeMaster) + fakeMaster.nodeUpdaterPool = nodeUpdaterPool + + nodeUpdaterPool.start(10) + b.ResetTimer() for i := 0; i < b.N; i++ { - _ = mockMaster.nfdAPIUpdateAllNodes() + _ = fakeMaster.nfdAPIUpdateAllNodes() } fmt.Println(b.Elapsed()) } @@ -909,23 +808,6 @@ func withTimeout(actual interface{}, expected ...interface{}) string { } } -func jsonPatchMatcher(expected []utils.JsonPatch) func([]utils.JsonPatch) bool { - return func(actual []utils.JsonPatch) bool { - // We don't care about modifying the original slices - ok, msg := assertions.So(sortJsonPatches(actual), ShouldResemble, sortJsonPatches(expected)) - if !ok { - // We parse the cryptic string message for better readability - var f assertions.FailureView - if err := yaml.Unmarshal([]byte(msg), &f); err == nil { - _, _ = Printf("%s\n", f.Message) - } else { - _, _ = Printf("%s\n", msg) - } - } - return ok - } -} - func sortJsonPatches(p []utils.JsonPatch) []utils.JsonPatch { sort.Slice(p, func(i, j int) bool { return p[i].Path < p[j].Path }) return p diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go index 46a790eb2..36ec9b1a0 100644 --- a/pkg/nfd-master/nfd-master.go +++ b/pkg/nfd-master/nfd-master.go @@ -19,6 +19,7 @@ package nfdmaster import ( "crypto/tls" "crypto/x509" + "encoding/json" "fmt" "maps" "net" @@ -41,8 +42,8 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8sLabels "k8s.io/apimachinery/pkg/labels" - "k8s.io/client-go/kubernetes" - restclient "k8s.io/client-go/rest" + "k8s.io/apimachinery/pkg/types" + k8sclient "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/leaderelection" "k8s.io/client-go/tools/leaderelection/resourcelock" "k8s.io/klog/v2" @@ -52,7 +53,6 @@ import ( taintutils "k8s.io/kubernetes/pkg/util/taints" "sigs.k8s.io/yaml" - "sigs.k8s.io/node-feature-discovery/pkg/apihelper" nfdv1alpha1 "sigs.k8s.io/node-feature-discovery/pkg/apis/nfd/v1alpha1" "sigs.k8s.io/node-feature-discovery/pkg/apis/nfd/v1alpha1/nodefeaturerule" "sigs.k8s.io/node-feature-discovery/pkg/apis/nfd/validate" @@ -150,8 +150,7 @@ type nfdMaster struct { healthServer *grpc.Server stop chan struct{} ready chan bool - apihelper apihelper.APIHelpers - kubeconfig *restclient.Config + k8sClient k8sclient.Interface nodeUpdaterPool *nodeUpdaterPool deniedNs config *NFDConfig @@ -495,12 +494,7 @@ func (m *nfdMaster) prune() error { return nil } - cli, err := m.apihelper.GetClient() - if err != nil { - return err - } - - nodes, err := m.apihelper.GetNodes(cli) + nodes, err := m.getNodes() if err != nil { return err } @@ -509,21 +503,21 @@ func (m *nfdMaster) prune() error { klog.InfoS("pruning node...", "nodeName", node.Name) // Prune labels and extended resources - err := m.updateNodeObject(cli, node.Name, Labels{}, Annotations{}, ExtendedResources{}, []corev1.Taint{}) + err := m.updateNodeObject(node.Name, Labels{}, Annotations{}, ExtendedResources{}, []corev1.Taint{}) if err != nil { nodeUpdateFailures.Inc() return fmt.Errorf("failed to prune node %q: %v", node.Name, err) } // Prune annotations - node, err := m.apihelper.GetNode(cli, node.Name) + node, err := m.getNode(node.Name) if err != nil { return err } maps.DeleteFunc(node.Annotations, func(k, v string) bool { return strings.HasPrefix(k, m.instanceAnnotation(nfdv1alpha1.AnnotationNs)) }) - err = m.apihelper.UpdateNode(cli, node) + _, err = m.k8sClient.CoreV1().Nodes().Update(context.TODO(), node, metav1.UpdateOptions{}) if err != nil { return fmt.Errorf("failed to prune annotations from node %q: %v", node.Name, err) } @@ -536,11 +530,7 @@ func (m *nfdMaster) prune() error { // "nfd.node.kubernetes.io/master.version" annotation, if it exists. // TODO: Drop when nfdv1alpha1.MasterVersionAnnotation is removed. func (m *nfdMaster) updateMasterNode() error { - cli, err := m.apihelper.GetClient() - if err != nil { - return err - } - node, err := m.apihelper.GetNode(cli, m.nodeName) + node, err := m.getNode(m.nodeName) if err != nil { return err } @@ -550,7 +540,8 @@ func (m *nfdMaster) updateMasterNode() error { node.Annotations, nil, "/metadata/annotations") - err = m.apihelper.PatchNode(cli, node.Name, p) + + err = m.patchNode(node.Name, p) if err != nil { return fmt.Errorf("failed to patch node annotations: %w", err) } @@ -701,13 +692,8 @@ func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.Se klog.InfoS("gRPC SetLabels request received", "nodeName", r.NodeName) } if !m.config.NoPublish { - cli, err := m.apihelper.GetClient() - if err != nil { - return &pb.SetLabelsReply{}, err - } - // Create labels et al - if err := m.refreshNodeFeatures(cli, r.NodeName, r.GetLabels(), r.GetFeatures()); err != nil { + if err := m.refreshNodeFeatures(r.NodeName, r.GetLabels(), r.GetFeatures()); err != nil { nodeUpdateFailures.Inc() return &pb.SetLabelsReply{}, err } @@ -718,12 +704,7 @@ func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.Se func (m *nfdMaster) nfdAPIUpdateAllNodes() error { klog.InfoS("will process all nodes in the cluster") - cli, err := m.apihelper.GetClient() - if err != nil { - return err - } - - nodes, err := m.apihelper.GetNodes(cli) + nodes, err := m.getNodes() if err != nil { return err } @@ -794,11 +775,7 @@ func (m *nfdMaster) nfdAPIUpdateOneNode(nodeName string) error { // Update node labels et al. This may also mean removing all NFD-owned // labels (et al.), for example in the case no NodeFeature objects are // present. - cli, err := m.apihelper.GetClient() - if err != nil { - return err - } - if err := m.refreshNodeFeatures(cli, nodeName, features.Labels, &features.Features); err != nil { + if err := m.refreshNodeFeatures(nodeName, features.Labels, &features.Features); err != nil { return err } @@ -843,7 +820,7 @@ func filterExtendedResource(name, value string, features *nfdv1alpha1.Features) return filteredValue, nil } -func (m *nfdMaster) refreshNodeFeatures(cli *kubernetes.Clientset, nodeName string, labels map[string]string, features *nfdv1alpha1.Features) error { +func (m *nfdMaster) refreshNodeFeatures(nodeName string, labels map[string]string, features *nfdv1alpha1.Features) error { if m.config.AutoDefaultNs { labels = addNsToMapKeys(labels, nfdv1alpha1.FeatureLabelNs) } else if labels == nil { @@ -872,7 +849,7 @@ func (m *nfdMaster) refreshNodeFeatures(cli *kubernetes.Clientset, nodeName stri taints = filterTaints(crTaints) } - err := m.updateNodeObject(cli, nodeName, labels, annotations, extendedResources, taints) + err := m.updateNodeObject(nodeName, labels, annotations, extendedResources, taints) if err != nil { klog.ErrorS(err, "failed to update node", "nodeName", nodeName) return err @@ -884,9 +861,9 @@ func (m *nfdMaster) refreshNodeFeatures(cli *kubernetes.Clientset, nodeName stri // setTaints sets node taints and annotations based on the taints passed via // nodeFeatureRule custom resorce. If empty list of taints is passed, currently // NFD owned taints and annotations are removed from the node. -func (m *nfdMaster) setTaints(cli *kubernetes.Clientset, taints []corev1.Taint, nodeName string) error { +func (m *nfdMaster) setTaints(taints []corev1.Taint, nodeName string) error { // Fetch the node object. - node, err := m.apihelper.GetNode(cli, nodeName) + node, err := m.getNode(nodeName) if err != nil { return err } @@ -928,7 +905,7 @@ func (m *nfdMaster) setTaints(cli *kubernetes.Clientset, taints []corev1.Taint, } if taintsUpdated { - err = controller.PatchNodeTaints(context.TODO(), cli, nodeName, node, newNode) + err = controller.PatchNodeTaints(context.TODO(), m.k8sClient, nodeName, node, newNode) if err != nil { return fmt.Errorf("failed to patch the node %v", node.Name) } @@ -949,7 +926,7 @@ func (m *nfdMaster) setTaints(cli *kubernetes.Clientset, taints []corev1.Taint, patches := createPatches([]string{nfdv1alpha1.NodeTaintsAnnotation}, node.Annotations, newAnnotations, "/metadata/annotations") if len(patches) > 0 { - err = m.apihelper.PatchNode(cli, node.Name, patches) + err = m.patchNode(node.Name, patches) if err != nil { return fmt.Errorf("error while patching node object: %w", err) } @@ -1047,13 +1024,9 @@ func (m *nfdMaster) processNodeFeatureRule(nodeName string, features *nfdv1alpha // updateNodeObject ensures the Kubernetes node object is up to date, // creating new labels and extended resources where necessary and removing // outdated ones. Also updates the corresponding annotations. -func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string, labels Labels, featureAnnotations Annotations, extendedResources ExtendedResources, taints []corev1.Taint) error { - if cli == nil { - return fmt.Errorf("no client is passed, client: %v", cli) - } - +func (m *nfdMaster) updateNodeObject(nodeName string, labels Labels, featureAnnotations Annotations, extendedResources ExtendedResources, taints []corev1.Taint) error { // Get the worker node object - node, err := m.apihelper.GetNode(cli, nodeName) + node, err := m.getNode(nodeName) if err != nil { return err } @@ -1110,13 +1083,13 @@ func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string, // patch node status with extended resource changes statusPatches := m.createExtendedResourcePatches(node, extendedResources) - err = m.apihelper.PatchNodeStatus(cli, node.Name, statusPatches) + err = m.patchNodeStatus(node.Name, statusPatches) if err != nil { return fmt.Errorf("error while patching extended resources: %w", err) } // Patch the node object in the apiserver - err = m.apihelper.PatchNode(cli, node.Name, patches) + err = m.patchNode(node.Name, patches) if err != nil { return fmt.Errorf("error while patching node object: %w", err) } @@ -1129,7 +1102,7 @@ func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string, } // Set taints - err = m.setTaints(cli, taints, node.Name) + err = m.setTaints(taints, node.Name) if err != nil { return err } @@ -1137,14 +1110,6 @@ func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string, return err } -func (m *nfdMaster) getKubeconfig() (*restclient.Config, error) { - var err error - if m.kubeconfig == nil { - m.kubeconfig, err = utils.GetKubeconfig(m.args.Kubeconfig) - } - return m.kubeconfig, err -} - // createPatches is a generic helper that returns json patch operations to perform func createPatches(removeKeys []string, oldItems map[string]string, newItems map[string]string, jsonPath string) []utils.JsonPatch { patches := []utils.JsonPatch{} @@ -1273,11 +1238,15 @@ func (m *nfdMaster) configure(filepath string, overrides string) error { } if !c.NoPublish { - kubeconfig, err := m.getKubeconfig() + kubeconfig, err := utils.GetKubeconfig(m.args.Kubeconfig) if err != nil { return err } - m.apihelper = apihelper.K8sHelpers{Kubeconfig: kubeconfig} + cli, err := k8sclient.NewForConfig(kubeconfig) + if err != nil { + return err + } + m.k8sClient = cli } // Pre-process DenyLabelNS into 2 lists: one for normal ns, and the other for wildcard ns @@ -1365,7 +1334,7 @@ func (m *nfdMaster) instanceAnnotation(name string) string { } func (m *nfdMaster) startNfdApiController() error { - kubeconfig, err := m.getKubeconfig() + kubeconfig, err := utils.GetKubeconfig(m.args.Kubeconfig) if err != nil { return err } @@ -1382,17 +1351,12 @@ func (m *nfdMaster) startNfdApiController() error { func (m *nfdMaster) nfdAPIUpdateHandlerWithLeaderElection() { ctx := context.Background() - client, err := m.apihelper.GetClient() - if err != nil { - klog.ErrorS(err, "failed to get Kubernetes client") - m.Stop() - } lock := &resourcelock.LeaseLock{ LeaseMeta: metav1.ObjectMeta{ Name: "nfd-master.nfd.kubernetes.io", Namespace: m.namespace, }, - Client: client.CoordinationV1(), + Client: m.k8sClient.CoordinationV1(), LockConfig: resourcelock.ResourceLockConfig{ // add uuid to prevent situation where 2 nfd-master nodes run on same node Identity: m.nodeName + "_" + uuid.NewString(), @@ -1410,7 +1374,7 @@ func (m *nfdMaster) nfdAPIUpdateHandlerWithLeaderElection() { }, OnStoppedLeading: func() { // We lost the lock. - klog.ErrorS(err, "leaderelection lock was lost") + klog.InfoS("leaderelection lock was lost") m.Stop() }, }, @@ -1440,3 +1404,26 @@ func (m *nfdMaster) filterFeatureAnnotations(annotations map[string]string) map[ } return outAnnotations } + +func (m *nfdMaster) getNode(nodeName string) (*corev1.Node, error) { + return m.k8sClient.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{}) +} + +func (m *nfdMaster) getNodes() (*corev1.NodeList, error) { + return m.k8sClient.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{}) +} + +func (m *nfdMaster) patchNode(nodeName string, patches []utils.JsonPatch, subresources ...string) error { + if len(patches) == 0 { + return nil + } + data, err := json.Marshal(patches) + if err == nil { + _, err = m.k8sClient.CoreV1().Nodes().Patch(context.TODO(), nodeName, types.JSONPatchType, data, metav1.PatchOptions{}) + } + return err +} + +func (m *nfdMaster) patchNodeStatus(nodeName string, patches []utils.JsonPatch) error { + return m.patchNode(nodeName, patches, "status") +} diff --git a/pkg/nfd-master/node-updater-pool_test.go b/pkg/nfd-master/node-updater-pool_test.go index 4e6ae1816..5c6924bf6 100644 --- a/pkg/nfd-master/node-updater-pool_test.go +++ b/pkg/nfd-master/node-updater-pool_test.go @@ -22,14 +22,11 @@ import ( "time" . "github.com/smartystreets/goconvey/convey" - "github.com/stretchr/testify/mock" - k8sclient "k8s.io/client-go/kubernetes" - "sigs.k8s.io/node-feature-discovery/pkg/apihelper" - "sigs.k8s.io/node-feature-discovery/pkg/generated/clientset/versioned/fake" - "sigs.k8s.io/node-feature-discovery/pkg/utils" + fakek8sclient "k8s.io/client-go/kubernetes/fake" + fakenfdclient "sigs.k8s.io/node-feature-discovery/pkg/generated/clientset/versioned/fake" ) -func newMockNodeUpdaterPool(nfdMaster *nfdMaster) *nodeUpdaterPool { +func newFakeNodeUpdaterPool(nfdMaster *nfdMaster) *nodeUpdaterPool { return &nodeUpdaterPool{ nfdMaster: nfdMaster, wg: sync.WaitGroup{}, @@ -37,65 +34,53 @@ func newMockNodeUpdaterPool(nfdMaster *nfdMaster) *nodeUpdaterPool { } func TestNodeUpdaterStart(t *testing.T) { - mockHelper := &apihelper.MockAPIHelpers{} - mockMaster := newMockMaster(mockHelper) - mockNodeUpdaterPool := newMockNodeUpdaterPool(mockMaster) + fakeMaster := newFakeMaster(nil) + nodeUpdaterPool := newFakeNodeUpdaterPool(fakeMaster) Convey("When starting the node updater pool", t, func() { - mockNodeUpdaterPool.start(10) - q := mockNodeUpdaterPool.queue + nodeUpdaterPool.start(10) + q := nodeUpdaterPool.queue Convey("Node updater pool queue properties should change", func() { So(q, ShouldNotBeNil) So(q.ShuttingDown(), ShouldBeFalse) }) - mockNodeUpdaterPool.start(10) + nodeUpdaterPool.start(10) Convey("Node updater pool queue should not change", func() { - So(mockNodeUpdaterPool.queue, ShouldEqual, q) + So(nodeUpdaterPool.queue, ShouldEqual, q) }) }) } func TestNodeUpdaterStop(t *testing.T) { - mockHelper := &apihelper.MockAPIHelpers{} - mockMaster := newMockMaster(mockHelper) - mockNodeUpdaterPool := newMockNodeUpdaterPool(mockMaster) + fakeMaster := newFakeMaster(nil) + nodeUpdaterPool := newFakeNodeUpdaterPool(fakeMaster) - mockNodeUpdaterPool.start(10) + nodeUpdaterPool.start(10) Convey("When stoping the node updater pool", t, func() { - mockNodeUpdaterPool.stop() + nodeUpdaterPool.stop() Convey("Node updater pool queue should be removed", func() { // Wait for the wg.Done() So(func() interface{} { - return mockNodeUpdaterPool.queue.ShuttingDown() + return nodeUpdaterPool.queue.ShuttingDown() }, withTimeout, 2*time.Second, ShouldBeTrue) }) }) } func TestRunNodeUpdater(t *testing.T) { - mockAPIHelper := &apihelper.MockAPIHelpers{} - mockMaster := newMockMaster(mockAPIHelper) - mockMaster.nfdController = newMockNfdAPIController(fake.NewSimpleClientset()) - mockClient := &k8sclient.Clientset{} - mockNode := newMockNode() - mockNodeUpdaterPool := newMockNodeUpdaterPool(mockMaster) - statusPatches := []utils.JsonPatch{} - metadataPatches := []utils.JsonPatch{} + fakeMaster := newFakeMaster(fakek8sclient.NewSimpleClientset()) + fakeMaster.nfdController = newFakeNfdAPIController(fakenfdclient.NewSimpleClientset()) + nodeUpdaterPool := newFakeNodeUpdaterPool(fakeMaster) - mockAPIHelper.On("GetClient").Return(mockClient, nil) - mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil) - mockAPIHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(statusPatches))).Return(nil) - mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(metadataPatches))).Return(nil) - - mockNodeUpdaterPool.start(10) + nodeUpdaterPool.start(10) Convey("Queue has no element", t, func() { - So(mockNodeUpdaterPool.queue.Len(), ShouldEqual, 0) + So(nodeUpdaterPool.queue.Len(), ShouldEqual, 0) }) - mockNodeUpdaterPool.queue.Add(mockNodeName) + nodeUpdaterPool.queue.Add(testNodeName) Convey("Added element to the queue should be removed", t, func() { - So(func() interface{} { return mockNodeUpdaterPool.queue.Len() }, + So(func() interface{} { return nodeUpdaterPool.queue.Len() }, withTimeout, 2*time.Second, ShouldEqual, 0) }) }