From cfbfd2eb18eae405db9250ea7d10ce8127fc0cb1 Mon Sep 17 00:00:00 2001 From: adrianc Date: Mon, 29 Jan 2024 15:05:19 +0200 Subject: [PATCH 1/5] Makefile enhancements - remove redundant "vet" target as lint tests already cover its functionality - remove commented out code - add option to provide specific pkgs to test in make test target via TESTPKGS env var, preserving current behaviour if not provided Signed-off-by: adrianc --- Makefile | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/Makefile b/Makefile index df5f6c91d..a7f063780 100644 --- a/Makefile +++ b/Makefile @@ -30,6 +30,7 @@ export WATCH_NAMESPACE?=openshift-sriov-network-operator export GOFLAGS+=-mod=vendor export GO111MODULE=on PKGS=$(shell go list ./... | grep -v -E '/vendor/|/test|/examples') +TESTPKGS?=./... # go source files, ignore vendor directory SRC = $(shell find . -type f -name '*.go' -not -path "./vendor/*") @@ -56,7 +57,7 @@ GOLANGCI_LINT_VER = v1.55.2 .PHONY: all build clean gendeepcopy test test-e2e test-e2e-k8s run image fmt sync-manifests test-e2e-conformance manifests update-codegen -all: generate vet build +all: generate lint build build: manager _build-sriov-network-config-daemon _build-webhook @@ -76,14 +77,14 @@ image: ; $(info Building images...) $(IMAGE_BUILDER) build -f $(DOCKERFILE_WEBHOOK) -t $(WEBHOOK_IMAGE_TAG) $(CURPATH) $(IMAGE_BUILD_OPTS) # Run tests -test: generate vet manifests envtest - KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir=/tmp -p path)" HOME="$(shell pwd)" go test ./... -coverprofile cover.out -v +test: generate lint manifests envtest + KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir=/tmp -p path)" HOME="$(shell pwd)" go test -coverprofile cover.out -v ${TESTPKGS} # Build manager binary -manager: generate vet _build-manager +manager: generate _build-manager # Run against the configured Kubernetes cluster in ~/.kube/config -run: vet skopeo install +run: skopeo install hack/run-locally.sh # Install CRDs into a cluster @@ -132,10 +133,6 @@ fmt: ## Go fmt your code fmt-code: go fmt ./... -# Run go vet against code -vet: - go vet ./... - # Generate code generate: controller-gen $(CONTROLLER_GEN) object:headerFile="hack/boilerplate.go.txt" paths="./..." @@ -205,7 +202,7 @@ redeploy-operator-virtual-cluster: test-e2e-validation-only: SUITE=./test/validation ./hack/run-e2e-conformance.sh -test-e2e: generate vet manifests skopeo envtest +test-e2e: generate manifests skopeo envtest KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir=/tmp -p path)"; source hack/env.sh; HOME="$(shell pwd)" go test ./test/e2e/... -timeout 60m -coverprofile cover.out -v test-e2e-k8s: export NAMESPACE=sriov-network-operator @@ -214,14 +211,9 @@ test-e2e-k8s: test-e2e test-bindata-scripts: fakechroot fakechroot ./test/scripts/enable-kargs_test.sh -test-%: generate vet manifests envtest +test-%: generate manifests envtest KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir=/tmp -p path)" HOME="$(shell pwd)" go test ./$*/... -coverprofile cover-$*.out -coverpkg ./... -v -# deploy-setup-k8s: export NAMESPACE=sriov-network-operator -# deploy-setup-k8s: export ADMISSION_CONTROLLERS_ENABLED=false -# deploy-setup-k8s: export CNI_BIN_PATH=/opt/cni/bin -# test-e2e-k8s: test-e2e - GOCOVMERGE = $(BIN_DIR)/gocovmerge gocovmerge: ## Download gocovmerge locally if necessary. $(call go-install-tool,$(GOCOVMERGE),github.com/shabbyrobe/gocovmerge/cmd/gocovmerge@latest) From 3c15708a6b7e0df16bb0d6469c244f18ef8402cf Mon Sep 17 00:00:00 2001 From: adrianc Date: Mon, 29 Jan 2024 15:08:10 +0200 Subject: [PATCH 2/5] run controllers test with kubernetes cluster type Add step to run controller tests with k8s cluster type as well. Signed-off-by: adrianc --- .github/workflows/test.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 4476a96d7..cbab249e8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -46,6 +46,9 @@ jobs: - name: test controllers on opensfhit run: CLUSTER_TYPE=openshift make test-controllers + - name: test controllers on kubernetes + run: CLUSTER_TYPE=kubernetes make test-controllers + - name: test bindata/scripts run: make test-bindata-scripts From 41075810212f0c2bc41026d7dd483c84f073fbf5 Mon Sep 17 00:00:00 2001 From: adrianc Date: Mon, 29 Jan 2024 15:09:08 +0200 Subject: [PATCH 3/5] Remove envtest from webhook tests its not needed to start envtest. moreover without this change, envtest environment would have remained running in the system as it was not stopped when test finished. Signed-off-by: adrianc --- pkg/webhook/validate_test.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/pkg/webhook/validate_test.go b/pkg/webhook/validate_test.go index 6197ad660..d67e1a011 100644 --- a/pkg/webhook/validate_test.go +++ b/pkg/webhook/validate_test.go @@ -10,8 +10,6 @@ import ( . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes" - "sigs.k8s.io/controller-runtime/pkg/envtest" . "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" constants "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts" @@ -1044,13 +1042,8 @@ func TestValidatePolicyForNodeStateWithInvalidDevice(t *testing.T) { }, } g := NewGomegaWithT(t) - var testEnv = &envtest.Environment{} - cfg, err := testEnv.Start() - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(cfg).ToNot(BeNil()) - kubeclient = kubernetes.NewForConfigOrDie(cfg) - _, err = validatePolicyForNodeState(policy, state, NewNode()) + _, err := validatePolicyForNodeState(policy, state, NewNode()) g.Expect(err).NotTo(HaveOccurred()) } From 78fabc85f6979b0153fb876de281120c81e6314d Mon Sep 17 00:00:00 2001 From: adrianc Date: Mon, 29 Jan 2024 15:11:33 +0200 Subject: [PATCH 4/5] Controllers test enhancements - skip sriovnetworkpoolconfig only test-case in kubernetes cluster as it only valid for openshift. - general: only stop envtest if it was successfully started to avoid nil pointer dereference in case system was missconfigured since AfterSuite Ginkgo node would run even if BeforeSuite node failed Signed-off-by: adrianc --- controllers/sriovnetworkpoolconfig_controller_test.go | 6 ++++++ controllers/suite_test.go | 11 ++++++----- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/controllers/sriovnetworkpoolconfig_controller_test.go b/controllers/sriovnetworkpoolconfig_controller_test.go index 5b76d1d19..b95f5edac 100644 --- a/controllers/sriovnetworkpoolconfig_controller_test.go +++ b/controllers/sriovnetworkpoolconfig_controller_test.go @@ -13,12 +13,18 @@ import ( mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" + "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts" constants "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts" + "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars" ) var _ = Describe("Operator", func() { Context("When is up", func() { It("should be able to create machine config for MachineConfigPool specified in sriov pool config", func() { + if vars.ClusterType != consts.ClusterTypeOpenshift { + Skip("test should only be executed with openshift cluster type") + } + config := &sriovnetworkv1.SriovNetworkPoolConfig{} config.SetNamespace(testNamespace) config.SetName("ovs-hw-offload-config") diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 0493ab7c6..6a9b1af5d 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -67,7 +67,7 @@ const ( interval = time.Millisecond * 250 ) -var _ = BeforeSuite(func(done Done) { +var _ = BeforeSuite(func() { logf.SetLogger(zap.New( zap.WriteTo(GinkgoWriter), zap.UseDevMode(true), @@ -213,15 +213,16 @@ var _ = BeforeSuite(func(done Done) { poolConfig.SetName(constants.DefaultConfigName) poolConfig.Spec = sriovnetworkv1.SriovNetworkPoolConfigSpec{} Expect(k8sClient.Create(context.TODO(), poolConfig)).Should(Succeed()) - close(done) }) var _ = AfterSuite(func() { By("tearing down the test environment") cancel() - Eventually(func() error { - return testEnv.Stop() - }, timeout, time.Second).ShouldNot(HaveOccurred()) + if testEnv != nil { + Eventually(func() error { + return testEnv.Stop() + }, timeout, time.Second).ShouldNot(HaveOccurred()) + } }) func TestAPIs(t *testing.T) { From 6867541c5d2bb839bf30ef7cf6a3461eabe84fa2 Mon Sep 17 00:00:00 2001 From: adrianc Date: Mon, 29 Jan 2024 15:15:34 +0200 Subject: [PATCH 5/5] e2e tests enhancements - remove deprecated use of async node - fail if creating client returned nil - use ptr pkg instead of pointer package as its deprecated Signed-off-by: adrianc --- test/conformance/tests/init.go | 3 +++ test/e2e/e2e_tests_suite_test.go | 8 +++----- test/e2e/sriovoperatornodepolicy_test.go | 2 ++ test/util/clean/clean.go | 4 ++++ test/validation/tests/test_validation.go | 19 ++++++++++--------- 5 files changed, 22 insertions(+), 14 deletions(-) diff --git a/test/conformance/tests/init.go b/test/conformance/tests/init.go index 56b836dd9..99458e8dc 100644 --- a/test/conformance/tests/init.go +++ b/test/conformance/tests/init.go @@ -18,4 +18,7 @@ func init() { } clients = testclient.New("") + if clients == nil { + panic("failed package init, failed to create ClientSet") + } } diff --git a/test/e2e/e2e_tests_suite_test.go b/test/e2e/e2e_tests_suite_test.go index d6c132d14..920def35a 100644 --- a/test/e2e/e2e_tests_suite_test.go +++ b/test/e2e/e2e_tests_suite_test.go @@ -13,7 +13,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -55,7 +55,7 @@ func TestSriovTests(t *testing.T) { var sriovIface *sriovnetworkv1.InterfaceExt -var _ = BeforeSuite(func(done Done) { +var _ = BeforeSuite(func() { logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) // Go to project root directory @@ -64,7 +64,7 @@ var _ = BeforeSuite(func(done Done) { By("bootstrapping test environment") testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{filepath.Join("config", "crd", "bases"), filepath.Join("test", "util", "crds")}, - UseExistingCluster: pointer.BoolPtr(true), + UseExistingCluster: ptr.To[bool](true), } var err error @@ -102,8 +102,6 @@ var _ = BeforeSuite(func(done Done) { doneNetNsSet = make(chan error, 1) go netns.SetPfVfLinkNetNs(testPciDev, testNsPath, devPollInterval, quitNetNsSet, doneNetNsSet) } - - close(done) }) var _ = AfterSuite(func() { diff --git a/test/e2e/sriovoperatornodepolicy_test.go b/test/e2e/sriovoperatornodepolicy_test.go index 891c06d12..ba2ec5e64 100644 --- a/test/e2e/sriovoperatornodepolicy_test.go +++ b/test/e2e/sriovoperatornodepolicy_test.go @@ -28,6 +28,8 @@ var _ = Describe("Operator", func() { execute.BeforeAll(func() { clients := testclient.New("") + Expect(clients).ToNot(BeNil()) + Eventually(func() *cluster.EnabledNodes { sriovInfos, _ = cluster.DiscoverSriov(clients, testNamespace) return sriovInfos diff --git a/test/util/clean/clean.go b/test/util/clean/clean.go index 74e6b13d3..fbc1ae1c4 100644 --- a/test/util/clean/clean.go +++ b/test/util/clean/clean.go @@ -20,6 +20,10 @@ func All() error { operatorNamespace = "openshift-sriov-network-operator" } clients := client.New("") + if clients == nil { + return fmt.Errorf("failed to create ClientSet") + } + if RestoreNodeDrainState { err := cluster.SetDisableNodeDrainState(clients, operatorNamespace, false) if err != nil { diff --git a/test/validation/tests/test_validation.go b/test/validation/tests/test_validation.go index 0df4fefb9..4c7980389 100644 --- a/test/validation/tests/test_validation.go +++ b/test/validation/tests/test_validation.go @@ -24,15 +24,6 @@ var ( operatorNamespace string ) -func init() { - operatorNamespace = os.Getenv("OPERATOR_NAMESPACE") - if operatorNamespace == "" { - operatorNamespace = "openshift-sriov-network-operator" - } - - clients = testclient.New("") -} - const ( sriovOperatorDeploymentName = "sriov-network-operator" // SriovNetworkNodePolicies contains the name of the sriov network node policies CRD @@ -45,6 +36,16 @@ const ( sriovOperatorConfigs = "sriovoperatorconfigs.sriovnetwork.openshift.io" ) +var _ = BeforeSuite(func() { + operatorNamespace = os.Getenv("OPERATOR_NAMESPACE") + if operatorNamespace == "" { + operatorNamespace = "openshift-sriov-network-operator" + } + + clients = testclient.New("") + Expect(clients).ToNot(BeNil()) +}) + var _ = Describe("validation", func() { Context("sriov", func() {