diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 53a0843..cf109b3 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -18,6 +18,6 @@ jobs: go-version-file: go.mod - name: Run linter - uses: golangci/golangci-lint-action@v6 + uses: golangci/golangci-lint-action@v7 with: - version: v1.63.4 + version: v2.8.0 diff --git a/.golangci.yml b/.golangci.yml index 36a5970..4a4a0c0 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,33 +1,15 @@ +version: "2" run: - timeout: 5m allow-parallel-runners: true - -issues: - # don't skip warning about doc comments - # don't exclude the default set of lint - exclude-use-default: false - # restore some of the defaults - # (fill in the rest as needed) - exclude-rules: - - path: "api/*" - linters: - - lll - - path: "internal/*" - linters: - - dupl - - lll linters: - disable-all: true + default: none enable: + - copyloopvar - dupl - errcheck - - copyloopvar - ginkgolinter - goconst - gocyclo - - gofmt - - goimports - - gosimple - govet - ineffassign - lll @@ -36,14 +18,41 @@ linters: - prealloc - revive - staticcheck - - typecheck - unconvert - unparam - unused + settings: + lll: + line-length: 170 + revive: + rules: + - name: comment-spacings + staticcheck: + dot-import-whitelist: + - "github.com/onsi/ginkgo/v2" + - "github.com/onsi/gomega" -linters-settings: - revive: + exclusions: + generated: lax rules: - - name: comment-spacings - lll: - line-length: 170 + - linters: + - lll + path: api/* + - linters: + - dupl + - lll + path: internal/* + paths: + - third_party$ + - builtin$ + - examples$ +formatters: + enable: + - gofmt + - goimports + exclusions: + generated: lax + paths: + - third_party$ + - builtin$ + - examples$ diff --git a/internal/controller/nodedrain_controller.go b/internal/controller/nodedrain_controller.go index 82158c6..686247c 100644 --- a/internal/controller/nodedrain_controller.go +++ b/internal/controller/nodedrain_controller.go @@ -79,18 +79,18 @@ func (r *NodeDrainReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } - if !controllerutil.ContainsFinalizer(nodeDrain, gezbcoukalphav1.NodeDrainFinalizer) && nodeDrain.ObjectMeta.DeletionTimestamp.IsZero() { + if !controllerutil.ContainsFinalizer(nodeDrain, gezbcoukalphav1.NodeDrainFinalizer) && nodeDrain.DeletionTimestamp.IsZero() { // Add finalizer when object is created controllerutil.AddFinalizer(nodeDrain, gezbcoukalphav1.NodeDrainFinalizer) - } else if controllerutil.ContainsFinalizer(nodeDrain, gezbcoukalphav1.NodeDrainFinalizer) && !nodeDrain.ObjectMeta.DeletionTimestamp.IsZero() { + } else if controllerutil.ContainsFinalizer(nodeDrain, gezbcoukalphav1.NodeDrainFinalizer) && !nodeDrain.DeletionTimestamp.IsZero() { // The object is being deleted // Do nothing special on deletion for now // Remove finalizer controllerutil.RemoveFinalizer(nodeDrain, gezbcoukalphav1.NodeDrainFinalizer) - if err := r.Client.Update(ctx, nodeDrain); err != nil { + if err := r.Update(ctx, nodeDrain); err != nil { return r.onReconcileError(ctx, nil, nodeDrain, err) } return ctrl.Result{}, nil @@ -154,7 +154,7 @@ func (r *NodeDrainReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return r.Client.Status().Update(ctx, nodeDrain) }) if updateErr != nil { - r.logger.Error(err, "Failed to update NodeDrain Status") + r.logger.Error(updateErr, "Failed to update NodeDrain Status") return r.onReconcileError(ctx, drainer, nodeDrain, updateErr) } } @@ -181,6 +181,23 @@ func (r *NodeDrainReconciler) reconcilePending(ctx context.Context, drainer *dra } } + if nodeDrain.Spec.NodeRole != node.Labels["role"] { + message := fmt.Sprintf("Node role mismatch: expected %s got %s", nodeDrain.Spec.NodeRole, node.Labels["role"]) + r.logger.Info(message) + publishEvent(r.Recorder, nodeDrain, corev1.EventTypeWarning, events.EventReasonNodeNotFound, message) + setStatus(r.Recorder, nodeDrain, gezbcoukalphav1.NodeDrainPhaseFailed) + return true, nil + } + + pattern := regexp.MustCompile(nodeDrain.Spec.VersionToDrainRegex) + if !pattern.MatchString(node.Status.NodeInfo.KubeletVersion) { + message := fmt.Sprintf("Node version mismatch: expected a value that satisfies %s got %s", nodeDrain.Spec.VersionToDrainRegex, node.Status.NodeInfo.KubeletVersion) + r.logger.Info(message) + publishEvent(r.Recorder, nodeDrain, corev1.EventTypeWarning, events.EventReasonNodeNotFound, message) + setStatus(r.Recorder, nodeDrain, gezbcoukalphav1.NodeDrainPhaseFailed) + return true, nil + } + if nodeDrain.Spec.DisableCordon { r.logger.Info("NOT Cordoning node", "node", nodeName) } else { @@ -294,7 +311,7 @@ func (r *NodeDrainReconciler) reconcileWaitForPodsToRestart(ctx context.Context, for _, nameAndNamespace := range nodeDrain.Status.PodsToBeEvicted { pod := corev1.Pod{} - err := r.Client.Get(ctx, client.ObjectKey{Namespace: nameAndNamespace.Namespace, Name: nameAndNamespace.Name}, &pod) + err := r.Get(ctx, client.ObjectKey{Namespace: nameAndNamespace.Namespace, Name: nameAndNamespace.Name}, &pod) if err != nil { r.logger.Info(fmt.Sprintf("Failed to get pod %s in namespace %s", nameAndNamespace.Name, nameAndNamespace.Namespace)) @@ -322,7 +339,7 @@ func (r *NodeDrainReconciler) getBlockingPods(ctx context.Context, nodeDrain *ge podsBlocking := []string{} // get a list of drainChecks drainCheckList := &gezbcoukalphav1.DrainCheckList{} - err := r.Client.List(ctx, drainCheckList, &client.ListOptions{}) + err := r.List(ctx, drainCheckList, &client.ListOptions{}) if err != nil { return podsBlocking, err } @@ -355,7 +372,7 @@ func (r *NodeDrainReconciler) fetchNode(ctx context.Context, drainer *drain.Help func (r *NodeDrainReconciler) areAllNodesCordoned(ctx context.Context, nodeDrain *gezbcoukalphav1.NodeDrain) (bool, error) { nodeList := &corev1.NodeList{} - err := r.Client.List(ctx, nodeList, &client.ListOptions{}) + err := r.List(ctx, nodeList, &client.ListOptions{}) if err != nil { return false, err } @@ -496,7 +513,7 @@ func createDrainer(ctx context.Context, mgrConfig *rest.Config) (*drain.Helper, } else { verbString = "Deleted" } - msg := fmt.Sprintf("pod: %s:%s %s from node: %s", pod.ObjectMeta.Namespace, pod.ObjectMeta.Name, verbString, pod.Spec.NodeName) + msg := fmt.Sprintf("pod: %s:%s %s from node: %s", pod.Namespace, pod.Name, verbString, pod.Spec.NodeName) klog.Info(msg) } return drainer, nil @@ -505,7 +522,7 @@ func createDrainer(ctx context.Context, mgrConfig *rest.Config) (*drain.Helper, // GetPodNameList returns a list of pod names from a pod list func GetPodNameList(pods []corev1.Pod) (result []string) { for _, pod := range pods { - result = append(result, pod.ObjectMeta.Name) + result = append(result, pod.Name) } return result } @@ -514,8 +531,8 @@ func GetPodNameList(pods []corev1.Pod) (result []string) { func GetNameSpaceAndName(pods []corev1.Pod) (result []gezbcoukalphav1.NamespaceAndName) { for _, pod := range pods { result = append(result, gezbcoukalphav1.NamespaceAndName{ - Name: pod.ObjectMeta.Name, - Namespace: pod.ObjectMeta.Namespace, + Name: pod.Name, + Namespace: pod.Namespace, }) } return result diff --git a/internal/controller/nodedrain_controller_test.go b/internal/controller/nodedrain_controller_test.go index a58da95..8e4fa01 100644 --- a/internal/controller/nodedrain_controller_test.go +++ b/internal/controller/nodedrain_controller_test.go @@ -484,6 +484,97 @@ var _ = Describe("Node Drain", func() { }) }) + + When("NodeDrain CR for a node that does not match the role", func() { + BeforeEach(func() { + node := getTestNode("node1", false) + node.Labels["role"] = "not-test-role" + Expect(k8sClient.Create(ctx, node)).To(Succeed()) + DeferCleanup(k8sClient.Delete, ctx, node) + + pod1 := getTestPod("pod1") + Expect(k8sClient.Create(ctx, pod1)).To(Succeed()) + pod2 := getTestPod("pod2") + Expect(k8sClient.Create(ctx, pod2)).To(Succeed()) + pod3 := getTestPod("pod3") + Expect(k8sClient.Create(ctx, pod3)).To(Succeed()) + DeferCleanup(k8sClient.Delete, ctx, pod1) + DeferCleanup(k8sClient.Delete, ctx, pod2) + DeferCleanup(k8sClient.Delete, ctx, pod3) + + nodeDrainCR = getTestNodeDrain(false, false) + Expect(k8sClient.Create(ctx, nodeDrainCR)).To(Succeed()) + DeferCleanup(k8sClient.Delete, ctx, nodeDrainCR) + }) + + It("should goto a failed state", func() { + nodeDrain := &gezbcoukalphav1.NodeDrain{} + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(nodeDrainCR), nodeDrain)).To(Succeed()) + g.Expect(nodeDrain.Status.Phase).To(Equal(gezbcoukalphav1.NodeDrainPhaseFailed)) + }, timeout, interval).Should(Succeed()) + + verifyStatusEvent(gezbcoukalphav1.NodeDrainPhasePending) + verifyStatusEvent(gezbcoukalphav1.NodeDrainPhaseFailed) + + verifyEvent(events.EventReasonNodeNotFound, "Node role mismatch: expected test-role got not-test-role") + + Expect(nodeDrain.Status.LastError).To(Equal("")) + Expect(nodeDrain.Status.TotalPods).To(Equal(0)) + Expect(nodeDrain.Status.PendingPods).To(BeEmpty()) + Expect(nodeDrain.Status.PodsToBeEvicted).To(BeEmpty()) + Expect(nodeDrain.Status.EvictionPodCount).To(Equal(0)) + Expect(nodeDrain.Status.DrainProgress).To(Equal(0)) + Expect(nodeDrain.Status.PodsBlockingDrain).To(Equal("")) + + }) + }) + + When("NodeDrain CR for a node that has version does not match the regex", func() { + BeforeEach(func() { + node := getTestNode("node1", false) + Expect(k8sClient.Create(ctx, node)).To(Succeed()) + DeferCleanup(k8sClient.Delete, ctx, node) + + pod1 := getTestPod("pod1") + Expect(k8sClient.Create(ctx, pod1)).To(Succeed()) + pod2 := getTestPod("pod2") + Expect(k8sClient.Create(ctx, pod2)).To(Succeed()) + pod3 := getTestPod("pod3") + Expect(k8sClient.Create(ctx, pod3)).To(Succeed()) + DeferCleanup(k8sClient.Delete, ctx, pod1) + DeferCleanup(k8sClient.Delete, ctx, pod2) + DeferCleanup(k8sClient.Delete, ctx, pod3) + + nodeDrainCR = getTestNodeDrain(false, false) + nodeDrainCR.Spec.VersionToDrainRegex = "^1.34.*$" + Expect(k8sClient.Create(ctx, nodeDrainCR)).To(Succeed()) + DeferCleanup(k8sClient.Delete, ctx, nodeDrainCR) + }) + + It("should goto a failed state", func() { + nodeDrain := &gezbcoukalphav1.NodeDrain{} + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(nodeDrainCR), nodeDrain)).To(Succeed()) + g.Expect(nodeDrain.Status.Phase).To(Equal(gezbcoukalphav1.NodeDrainPhaseFailed)) + }, timeout, interval).Should(Succeed()) + + verifyStatusEvent(gezbcoukalphav1.NodeDrainPhasePending) + verifyStatusEvent(gezbcoukalphav1.NodeDrainPhaseFailed) + + verifyEvent(events.EventReasonNodeNotFound, "Node version mismatch: expected a value that satisfies ^1.34.*$ got 1.35.5-eks") + + Expect(nodeDrain.Status.LastError).To(Equal("")) + Expect(nodeDrain.Status.TotalPods).To(Equal(0)) + Expect(nodeDrain.Status.PendingPods).To(BeEmpty()) + Expect(nodeDrain.Status.PodsToBeEvicted).To(BeEmpty()) + Expect(nodeDrain.Status.EvictionPodCount).To(Equal(0)) + Expect(nodeDrain.Status.DrainProgress).To(Equal(0)) + Expect(nodeDrain.Status.PodsBlockingDrain).To(Equal("")) + + }) + }) + }) func getTestPod(podName string) *corev1.Pod { @@ -521,7 +612,7 @@ func getTestNodeDrain(disableCordon bool, waitForRestart bool) *gezbcoukalphav1. }, Spec: gezbcoukalphav1.NodeDrainSpec{ NodeName: "node1", - VersionToDrainRegex: "1.35.*", + VersionToDrainRegex: "^1.35.*$", NodeRole: "test-role", DisableCordon: disableCordon, WaitForPodsToRestart: waitForRestart, diff --git a/magefile.go b/magefile.go index a551a6e..acc669e 100644 --- a/magefile.go +++ b/magefile.go @@ -26,13 +26,12 @@ func TestE2E() error { if err != nil { return err } + // make sure we delete the kind cluster + defer sh.RunWithV(env, "k3d", "cluster", "delete", CLUSTER_NAME) err = sh.RunWithV(env, "go", "test", "./test/e2e/", "-v", "-ginkgo.v") if err != nil { return err } - - // make sure we delete the kind cluster - defer sh.RunWithV(env, "k3d", "cluster", "delete", CLUSTER_NAME) return nil } diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 0743ec7..0025e51 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -235,7 +235,7 @@ var _ = Describe("Manager", Ordered, func() { expectNumberOfPodsRunning(2) - nodeDrainFile := applyNodeDrain(false) + nodeDrainFile := applyNodeDrain(false, K8sVersionRegex, k8sRole) By("Waiting for all pods to be removed") expectNumberOfPodsRunning(0) @@ -257,7 +257,7 @@ var _ = Describe("Manager", Ordered, func() { err = os.Remove(nodeDrainFile) Expect(err).NotTo(HaveOccurred(), "Failed remove nodeDrainFile") - By(" Deleting test statefulsets") + By("Deleting test statefulsets") cmd = exec.Command("kubectl", "delete", "statefulset", "nginx") _, err = utils.Run(cmd) Expect(err).NotTo(HaveOccurred(), "Failed delete nginx statefulset") @@ -289,7 +289,7 @@ var _ = Describe("Manager", Ordered, func() { drainCheckFile := applyDrainCheck("^blocking-.*") - nodeDrainFile := applyNodeDrain(false) + nodeDrainFile := applyNodeDrain(false, K8sVersionRegex, k8sRole) By("Checking we still have all pods running") expectNumberOfPodsRunning(2) @@ -323,7 +323,7 @@ var _ = Describe("Manager", Ordered, func() { By("Clean up after the test") - By(" Deleting nginx statefulsets") + By("Deleting nginx statefulsets") cmd = exec.Command("kubectl", "delete", "statefulset", "nginx") _, err = utils.Run(cmd) Expect(err).NotTo(HaveOccurred(), "Failed delete nginx statefulset") @@ -351,7 +351,7 @@ var _ = Describe("Manager", Ordered, func() { By("Uncordening our test node") utils.UncordonNode(worker2Node) - By("Verifying the number of pods on the node(s)") + By("Verifying the number of pods on the node(s) is 0") expectNumberOfPodsRunning(0) @@ -361,13 +361,13 @@ var _ = Describe("Manager", Ordered, func() { By("Waiting for all pods to be running") expectNumberOfPodsRunning(2) - By("Cordening the worker2 now it has workload on it") - utils.CordonNode(worker2Node) + // By("Cordening the worker2 now it has workload on it") + // utils.CordonNode(worker2Node) - By("Uncordening the other node") + By("Uncordening worker3") utils.UncordonNode(worker3Node) - nodeDrainFile := applyNodeDrain(false) + nodeDrainFile := applyNodeDrain(false, K8sVersionRegex, k8sRole) nodeDrainIsPhaseOtherNodesNotCordoned := func(g Gomega) { cmd := exec.Command("kubectl", "get", "nodedrain", "nodedrain-sample") @@ -395,12 +395,12 @@ var _ = Describe("Manager", Ordered, func() { By("Clean up after the test") - By(" Deleting nginx statefulsets") + By("Deleting nginx statefulsets") cmd = exec.Command("kubectl", "delete", "statefulset", "nginx") _, err = utils.Run(cmd) Expect(err).NotTo(HaveOccurred(), "Failed delete nginx statefulset") - By(" Deleting nginx2 statefulsets") + By("Deleting nginx2 statefulsets") cmd = exec.Command("kubectl", "delete", "statefulset", "nginx2") _, err = utils.Run(cmd) Expect(err).NotTo(HaveOccurred(), "Failed delete nginx statefulset") @@ -423,7 +423,7 @@ var _ = Describe("Manager", Ordered, func() { By("Uncordening our test nodes") utils.UncordonNode(worker2Node) - By("Verifying the number of pods on the node(s)") + By("Verifying the number of pods on the node(s) is 0") expectNumberOfPodsRunning(0) @@ -433,7 +433,7 @@ var _ = Describe("Manager", Ordered, func() { By("Waiting for all pods to be running") expectNumberOfPodsRunning(2) - nodeDrainFile := applyNodeDrain(true) + nodeDrainFile := applyNodeDrain(true, K8sVersionRegex, k8sRole) By("Drain should run and we should be left no pods") expectNumberOfPodsRunning(0) @@ -460,12 +460,12 @@ var _ = Describe("Manager", Ordered, func() { By("Clean up after the test") - By(" Deleting nginx statefulsets") + By("Deleting nginx statefulsets") cmd := exec.Command("kubectl", "delete", "statefulset", "nginx") _, err := utils.Run(cmd) Expect(err).NotTo(HaveOccurred(), "Failed delete nginx statefulset") - By(" Deleting nginx2 statefulsets") + By("Deleting nginx2 statefulsets") cmd = exec.Command("kubectl", "delete", "statefulset", "nginx2") _, err = utils.Run(cmd) Expect(err).NotTo(HaveOccurred(), "Failed delete nginx statefulset") @@ -480,6 +480,62 @@ var _ = Describe("Manager", Ordered, func() { By("re-cordening our test nodes") utils.CordonNode(worker2Node) }) + + It("if NodeDrain Role does not match the nodes role the drain will fail", func() { + + By("Uncordening our test nodes") + utils.UncordonNode(worker2Node) + + nodeDrainFile := applyNodeDrain(true, K8sVersionRegex, "wrong-role") + + nodeDrainIsPhaseFailed := func(g Gomega) { + cmd := exec.Command("kubectl", "get", "nodedrain", "nodedrain-sample") + output, err := utils.Run(cmd) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(output).To(ContainSubstring("nodedrain-sample Failed")) + } + Eventually(nodeDrainIsPhaseFailed, 5*time.Minute).Should(Succeed()) + + By("Clean up after the test") + + By("deleting the NodeDrain") + cmd := exec.Command("kubectl", "delete", "-f", nodeDrainFile) + _, err := utils.Run(cmd) + Expect(err).NotTo(HaveOccurred(), "Failed delete nodeDrain") + err = os.Remove(nodeDrainFile) + Expect(err).NotTo(HaveOccurred(), "Failed remove nodeDrainFile") + + By("re-cordening our test nodes") + utils.CordonNode(worker2Node) + }) + + It("if NodeDrain K8S Version regexp does not match the nodes kubelet version the drain will fail", func() { + + By("Uncordening our test nodes") + utils.UncordonNode(worker2Node) + + nodeDrainFile := applyNodeDrain(true, "^v1\\.30\\..*$", k8sRole) + + nodeDrainIsPhaseFailed := func(g Gomega) { + cmd := exec.Command("kubectl", "get", "nodedrain", "nodedrain-sample") + output, err := utils.Run(cmd) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(output).To(ContainSubstring("nodedrain-sample Failed")) + } + Eventually(nodeDrainIsPhaseFailed, 5*time.Minute).Should(Succeed()) + + By("Clean up after the test") + + By("deleting the NodeDrain") + cmd := exec.Command("kubectl", "delete", "-f", nodeDrainFile) + _, err := utils.Run(cmd) + Expect(err).NotTo(HaveOccurred(), "Failed delete nodeDrain") + err = os.Remove(nodeDrainFile) + Expect(err).NotTo(HaveOccurred(), "Failed remove nodeDrainFile") + + By("re-cordening our test nodes") + utils.CordonNode(worker2Node) + }) }) func applyDrainCheck(podRegex string) string { @@ -503,7 +559,7 @@ spec: return drainCheckFile } -func applyNodeDrain(waitForPodToRestart bool) string { +func applyNodeDrain(waitForPodToRestart bool, versionToDrainRegex string, role string) string { By("Creating NodeDrain for node :" + worker2Node) nodeDrain := fmt.Sprintf(` @@ -516,7 +572,7 @@ spec: versionToDrainRegex: %s nodeRole: %s waitForPodsToRestart: %t -`, worker2Node, K8sVersionRegex, k8sRole, waitForPodToRestart) +`, worker2Node, versionToDrainRegex, role, waitForPodToRestart) nodeDrainFile, err := utils.CreateTempFile(nodeDrain) Expect(err).NotTo(HaveOccurred(), "Failed creating NodeDrain file to apply: "+nodeDrainFile) @@ -585,7 +641,7 @@ spec: kubernetes.io/arch: amd64 role: %s `, k8sRole) - inflate = strings.Replace(inflate, "{{.metadata.name}}", name, -1) + inflate = strings.ReplaceAll(inflate, "{{.metadata.name}}", name) inflateFile, err := utils.CreateTempFile(inflate) Expect(err).NotTo(HaveOccurred(), "Failed apply "+name) cmd := exec.Command("kubectl", "apply", "-f", inflateFile) diff --git a/test/utils/utils.go b/test/utils/utils.go index a66f0bf..2303d10 100644 --- a/test/utils/utils.go +++ b/test/utils/utils.go @@ -8,7 +8,7 @@ import ( "os/exec" "strings" - . "github.com/onsi/ginkgo/v2" //nolint:golint,revive + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -190,7 +190,7 @@ func GetProjectDir() (string, error) { if err != nil { return wd, err } - wd = strings.Replace(wd, "/test/e2e", "", -1) + wd = strings.ReplaceAll(wd, "/test/e2e", "") return wd, nil }