New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test/e2e: add basic e2e-tests for NodeFeature API #1001

Merged

k8s-ci-robot merged 2 commits into kubernetes-sigs:master from marquiz:devel/e2e-nodefeature

Dec 19, 2022

Contributor

marquiz commented Dec 14, 2022

Add an initial test set for the NodeFeature API. This is done simply by
running a second pass of the tests but with -enable-nodefeature-api
(i.e. NodeFeature API enabled and gRPC disabled). This should give basic
confidence that the API actually works and form a basis for further
imporovements on testing the new CRD API.

netlify bot commented Dec 14, 2022 •

edited

Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`b67d6d7`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/63a07c21b6fa2300084a3302
😎 Deploy Preview	https://deploy-preview-1001--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

k8s-ci-robot added the cncf-cla: yes label

Contributor

k8s-ci-robot commented Dec 14, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from adrianchiris and kad

December 14, 2022 20:21

k8s-ci-robot added approved size/XL labels

Contributor Author

marquiz commented Dec 14, 2022

We need #998 and #999 before merging this
/hold

k8s-ci-robot added the do-not-merge/hold label

Contributor Author

marquiz commented Dec 15, 2022

Actually, #998 and #999 shouldn't be mandatory
/unhold

k8s-ci-robot removed the do-not-merge/hold label

Contributor Author

marquiz commented Dec 15, 2022

/assign @fmuyassarov @ArangoGutierrez

k8s-ci-robot assigned ArangoGutierrez and fmuyassarov

marquiz force-pushed the devel/e2e-nodefeature branch from bf2c08c to 52ceb4f Compare

December 15, 2022 16:53

k8s-ci-robot added the needs-rebase label

marquiz force-pushed the devel/e2e-nodefeature branch from 52ceb4f to 2c8d2fc Compare

December 16, 2022 12:51

k8s-ci-robot removed the needs-rebase label

Contributor Author

marquiz commented Dec 16, 2022

Rebased

marquiz force-pushed the devel/e2e-nodefeature branch from 2c8d2fc to 52ceb4f Compare

December 16, 2022 20:56

k8s-ci-robot added the needs-rebase label

marquiz force-pushed the devel/e2e-nodefeature branch from 52ceb4f to 2c8d2fc Compare

December 16, 2022 21:02

k8s-ci-robot added needs-rebase and removed needs-rebase labels

marquiz force-pushed the devel/e2e-nodefeature branch from 2c8d2fc to 68ed274 Compare

December 19, 2022 08:10

k8s-ci-robot removed the needs-rebase label


          test/e2e: isolate tests into a separate function

958db56

Preparation for running the same tests with NodeFeature API enabled
(instead of gRPC).

marquiz force-pushed the devel/e2e-nodefeature branch from 68ed274 to 6a8b809 Compare

December 19, 2022 12:15

Contributor Author

marquiz commented Dec 19, 2022

Yet another rebase

fmuyassarov reviewed

View reviewed changes

Member

fmuyassarov left a comment

Looks good @marquiz. Some inline comments.

test/e2e/node_feature_discovery.go Outdated Show resolved Hide resolved

test/e2e/node_feature_discovery.go

-              				err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Delete(context.TODO(), workerPod.ObjectMeta.Name, metav1.DeleteOptions{})
+              				By("Verifying the node where nfd-master is running")
+              				// Get updated masterPod object (we want to know where it was scheduled)
+              				masterPod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Get(context.TODO(), masterPod.ObjectMeta.Name, metav1.GetOptions{})

Member

fmuyassarov Dec 19, 2022

Suggested change

      
            				masterPod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Get(context.TODO(), masterPod.ObjectMeta.Name, metav1.GetOptions{})
          
            				masterPod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Get(context.TODO(), masterPod.Name, metav1.GetOptions{})

nit

Contributor Author

marquiz Dec 19, 2022

Thx! A bit unrelated to this PR as the first patch basically just indents the code by one tab (creating a function wrapper). All these ObjectMeta fixed separately in #1005

Member

fmuyassarov Dec 19, 2022

cool, thanks

test/e2e/node_feature_discovery.go

-              				}
-              				fConf := cfg.DefaultFeatures
+              				By("Waiting for the nfd-master service to be up")
+              				Expect(e2enetwork.WaitForService(f.ClientSet, f.Namespace.Name, nfdSvc.ObjectMeta.Name, true, time.Second, 10*time.Second)).NotTo(HaveOccurred())

Member

fmuyassarov Dec 19, 2022

Suggested change

      
            				Expect(e2enetwork.WaitForService(f.ClientSet, f.Namespace.Name, nfdSvc.ObjectMeta.Name, true, time.Second, 10*time.Second)).NotTo(HaveOccurred())
          
            				Expect(e2enetwork.WaitForService(f.ClientSet, f.Namespace.Name, nfdSvc.Name, true, time.Second, 10*time.Second)).NotTo(HaveOccurred())

Contributor Author

marquiz Dec 19, 2022

test/e2e/node_feature_discovery.go

+              					checkNodeFeatureObject(workerPod.Spec.NodeName)
+              					By("Deleting the node-feature-discovery worker pod")
+              					err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Delete(context.TODO(), workerPod.ObjectMeta.Name, metav1.DeleteOptions{})

Member

fmuyassarov Dec 19, 2022

Suggested change

      
            					err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Delete(context.TODO(), workerPod.ObjectMeta.Name, metav1.DeleteOptions{})
          
            					err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Delete(context.TODO(), workerPod.Name, metav1.DeleteOptions{})

Contributor Author

marquiz Dec 19, 2022

test/e2e/node_feature_discovery.go

Comment on lines +172 to +178

+              				_, err := nfdClient.NfdV1alpha1().NodeFeatures(f.Namespace.Name).Get(context.TODO(), name, metav1.GetOptions{})
+              				if useNodeFeatureApi {
+              					By(fmt.Sprintf("Check that NodeFeature object for the node %q was created", name))
+              					Expect(err).NotTo(HaveOccurred())
+              				} else {
+              					By(fmt.Sprintf("Check that NodeFeature object for the node %q hasn't been created", name))
+              					Expect(err).To(HaveOccurred())

Member

fmuyassarov Dec 19, 2022

IINW, NodeFeature CRD will still be applied even though we are using gRPC because its definition is in the same file. I think we should extend the else statement to check that NodeFeature is empty.

Contributor Author

marquiz Dec 19, 2022

I think that's fine, doesn't do any harm afaics. I mean I think it's ok to create both CRDs even if we run with NodeFeature disabled in the first pass.

Member

fmuyassarov Dec 19, 2022

My point was, only expecting err to have occurred is not enough, because we might have no error of getting NodeFeature and still get 0 NodeFeature, which is the case that falls under else{}.

Contributor Author

marquiz Dec 19, 2022

My point was, only expecting err to have occurred is not enough, because we might have no error of getting NodeFeature and still get 0 NodeFeature, which is the case that falls under else{}.

Ach, sorry for reading this comment way too hastily. This doesn't create the CRD. It's about nfd-worker creating the NodeFeature object (for the node it's running on). We Get a NodeFeature object with a specified name in a specified namespace so we get a NotExist error if that does not exist

Member

fmuyassarov Dec 19, 2022

Actually you are right, I didn't take into account namespace and name...


          test/e2e: add basic e2e-tests for NodeFeature API

b67d6d7

Add an initial test set for the NodeFeature API. This is done simply by
running a second pass of the tests but with -enable-nodefeature-api
(i.e. NodeFeature API enabled and gRPC disabled). This should give basic
confidence that the API actually works and form a basis for further
imporovements on testing the new CRD API.

marquiz force-pushed the devel/e2e-nodefeature branch from 6a8b809 to b67d6d7 Compare

December 19, 2022 14:58

marquiz mentioned this pull request

Simplify usage of ObjectMeta fields #1005

Merged

Contributor Author

marquiz commented Dec 19, 2022

Thanks @fmuyassarov for the review. Feedback addressed/responded. WDYT, PTAL

fmuyassarov reviewed

View reviewed changes

Member

fmuyassarov left a comment

Looks good. I think with this we are close to cut the release soon.
/lgtm

k8s-ci-robot added the lgtm label

Contributor

k8s-ci-robot commented Dec 19, 2022

LGTM label has been added.

Git tree hash: 5682f49dbfb9e871e6f3dfddf1f8de63eb4bc089

k8s-ci-robot merged commit e229699 into kubernetes-sigs:master

marquiz deleted the devel/e2e-nodefeature branch

December 19, 2022 15:35

Contributor Author

marquiz commented Dec 19, 2022

I think with this we are close to cut the release soon.

👍

This was referenced Dec 20, 2022

NodeFeature CRD #828

Closed

Release v0.12.0 #985

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cncf-cla: yes lgtm size/XL