From 090d9cfeeddf24cef3ac99ee148895b1510ff76c Mon Sep 17 00:00:00 2001 From: georgieva Date: Tue, 2 Nov 2021 13:27:22 +0200 Subject: [PATCH 1/3] Added a fix and test for PreviewChanges when os/k8s is not supported. Added test to algo installer to check for rollback when an error occurs mid installation. --- agent/installer/installer.go | 10 ++- agent/installer/installer_test.go | 10 +++ agent/installer/internal/algo/algo_test.go | 37 ++++++++++ .../internal/algo/mock_ubuntu_with_error.go | 69 +++++++++++++++++++ 4 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 agent/installer/internal/algo/mock_ubuntu_with_error.go diff --git a/agent/installer/installer.go b/agent/installer/installer.go index 19797be4e..55879e7b6 100644 --- a/agent/installer/installer.go +++ b/agent/installer/installer.go @@ -186,14 +186,18 @@ func getSupportedRegistryDescription() registry { func PreviewChanges(os, k8sVer string) (install, uninstall string, err error) { stepPreviewer := stringPrinter{msgFmt: "# %s"} reg := getSupportedRegistry(&bundleDownloader{}, &stepPreviewer) - installer := reg.GetInstaller(os, k8sVer).(algo.Installer) - err = installer.Install() + installer := reg.GetInstaller(os, k8sVer) + if installer == nil { + return + } + algoInstaller := installer.(algo.Installer) + err = algoInstaller.Install() if err != nil { return } install = stepPreviewer.String() stepPreviewer.steps = nil - err = installer.Uninstall() + err = algoInstaller.Uninstall() if err != nil { return } diff --git a/agent/installer/installer_test.go b/agent/installer/installer_test.go index 17d0a7930..6e9a4bf6b 100644 --- a/agent/installer/installer_test.go +++ b/agent/installer/installer_test.go @@ -99,6 +99,16 @@ var _ = Describe("Byohost Installer Tests", func() { Expect(uninstall).ShouldNot(ContainSubstring("Installing")) }) }) + Context("When PreviewChanges is called for non-supported os and k8s", func() { + It("Should return empty result", func() { + os := "a" + k8s := "a" + install, uninstall, err := PreviewChanges(os, k8s) + Expect(err).ShouldNot((HaveOccurred())) + Expect(install).Should(Equal("")) + Expect(uninstall).Should(Equal("")) + }) + }) }) func NewPreviewInstaller(os string, ob algo.OutputBuilder) *installer { diff --git a/agent/installer/internal/algo/algo_test.go b/agent/installer/internal/algo/algo_test.go index 0fcac6cb8..19fe6cb4c 100644 --- a/agent/installer/internal/algo/algo_test.go +++ b/agent/installer/internal/algo/algo_test.go @@ -4,6 +4,9 @@ package algo import ( + "log" + "os" + . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -43,4 +46,38 @@ var _ = Describe("Installer Algo Tests", func() { Expect(outputBuilderCounter.LogCalledCnt).Should(Equal(stepsNum)) }) }) + Context("When error occurs during installation", func() { + var ( + err error + mockUbuntu MockUbuntuWithError + ) + const ( + stepWhenError = 5 + obcExpectedTotal = stepWhenError*4 + 2 + ) + BeforeEach(func() { + mockUbuntu = MockUbuntuWithError{} + mockUbuntu.OutputBuilder = &outputBuilderCounter + mockUbuntu.BundlePath, err = os.MkdirTemp("", "algoErrTest") + if err != nil { + log.Fatal(err) + } + + installer = &BaseK8sInstaller{ + BundlePath: mockUbuntu.BundlePath, + K8sStepProvider: &mockUbuntu, + OutputBuilder: &outputBuilderCounter} + }) + AfterEach(func() { + err = os.RemoveAll(mockUbuntu.BundlePath) + if err != nil { + log.Fatal(err) + } + }) + It("Should rollback all applied steps", func() { + err = installer.Install() + Expect(err).Should((HaveOccurred())) + Expect(outputBuilderCounter.LogCalledCnt).Should(Equal(obcExpectedTotal)) + }) + }) }) diff --git a/agent/installer/internal/algo/mock_ubuntu_with_error.go b/agent/installer/internal/algo/mock_ubuntu_with_error.go new file mode 100644 index 000000000..810d4387c --- /dev/null +++ b/agent/installer/internal/algo/mock_ubuntu_with_error.go @@ -0,0 +1,69 @@ +// Copyright 2021 VMware, Inc. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package algo + +type MockUbuntuWithError struct { + BaseK8sInstaller +} + +func getEmptyStep(desc string, bki *BaseK8sInstaller) Step { + return &ShellStep{ + BaseK8sInstaller: bki, + Desc: desc, + DoCmd: `:`, + UndoCmd: `:`} +} + +func (u *MockUbuntuWithError) swapStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("SWAP", bki) +} + +func (u *MockUbuntuWithError) firewallStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("FIREWALL", bki) +} + +func (u *MockUbuntuWithError) kernelModsLoadStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("KERNEL MODULES", bki) +} + +func (u *MockUbuntuWithError) unattendedUpdStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("AUTO OS UPGRADES", bki) +} + +func (u *MockUbuntuWithError) osWideCfgUpdateStep(bki *BaseK8sInstaller) Step { + // Here DoCmd should return error when executed + return &ShellStep{ + BaseK8sInstaller: bki, + Desc: "OS CONFIGURATION", + DoCmd: `a`, + UndoCmd: `:`} +} + +func (u *MockUbuntuWithError) criToolsStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("cri-tools.deb", bki) +} + +func (u *MockUbuntuWithError) criKubernetesStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("kubernetes-cni.deb", bki) +} + +func (u *MockUbuntuWithError) kubectlStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("kubectl.deb", bki) +} + +func (u *MockUbuntuWithError) kubeadmStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("kubeadm.deb", bki) +} + +func (u *MockUbuntuWithError) kubeletStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("kubelet.deb", bki) +} + +func (u *MockUbuntuWithError) containerdStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("CONTAINERD", bki) +} + +func (u *MockUbuntuWithError) containerdDaemonStep(bki *BaseK8sInstaller) Step { + return getEmptyStep("CONTAINERD SERVICE", bki) +} From f1f51a6ae67383399fec758eab96815c5a987f8d Mon Sep 17 00:00:00 2001 From: georgieva Date: Wed, 3 Nov 2021 11:41:12 +0200 Subject: [PATCH 2/3] Addressing feedback from review. Made PreviewChanges and its test to check for error. Implemented mockStep for testing the mockUbuntu and made mockUbuntu to count number of calls for do and undo steps as well as the ability to define which step is with error. --- agent/installer/installer.go | 6 +- agent/installer/installer_test.go | 9 ++- agent/installer/internal/algo/algo_test.go | 25 ++----- .../internal/algo/mock_ubuntu_with_error.go | 70 +++++++++++++------ 4 files changed, 60 insertions(+), 50 deletions(-) diff --git a/agent/installer/installer.go b/agent/installer/installer.go index 55879e7b6..353212bb8 100644 --- a/agent/installer/installer.go +++ b/agent/installer/installer.go @@ -188,16 +188,16 @@ func PreviewChanges(os, k8sVer string) (install, uninstall string, err error) { reg := getSupportedRegistry(&bundleDownloader{}, &stepPreviewer) installer := reg.GetInstaller(os, k8sVer) if installer == nil { + err = ErrOsK8sNotSupported return } - algoInstaller := installer.(algo.Installer) - err = algoInstaller.Install() + err = installer.(algo.Installer).Install() if err != nil { return } install = stepPreviewer.String() stepPreviewer.steps = nil - err = algoInstaller.Uninstall() + err = installer.(algo.Installer).Uninstall() if err != nil { return } diff --git a/agent/installer/installer_test.go b/agent/installer/installer_test.go index 6e9a4bf6b..a5e07ae6c 100644 --- a/agent/installer/installer_test.go +++ b/agent/installer/installer_test.go @@ -100,13 +100,12 @@ var _ = Describe("Byohost Installer Tests", func() { }) }) Context("When PreviewChanges is called for non-supported os and k8s", func() { - It("Should return empty result", func() { + It("Should return error", func() { os := "a" k8s := "a" - install, uninstall, err := PreviewChanges(os, k8s) - Expect(err).ShouldNot((HaveOccurred())) - Expect(install).Should(Equal("")) - Expect(uninstall).Should(Equal("")) + _, _, err := PreviewChanges(os, k8s) + Expect(err).Should((HaveOccurred())) + Expect(err).Should(Equal(ErrOsK8sNotSupported)) }) }) }) diff --git a/agent/installer/internal/algo/algo_test.go b/agent/installer/internal/algo/algo_test.go index 19fe6cb4c..e783f371d 100644 --- a/agent/installer/internal/algo/algo_test.go +++ b/agent/installer/internal/algo/algo_test.go @@ -4,9 +4,6 @@ package algo import ( - "log" - "os" - . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -48,36 +45,22 @@ var _ = Describe("Installer Algo Tests", func() { }) Context("When error occurs during installation", func() { var ( - err error mockUbuntu MockUbuntuWithError ) - const ( - stepWhenError = 5 - obcExpectedTotal = stepWhenError*4 + 2 - ) BeforeEach(func() { mockUbuntu = MockUbuntuWithError{} + mockUbuntu.errorOnStep = 5 mockUbuntu.OutputBuilder = &outputBuilderCounter - mockUbuntu.BundlePath, err = os.MkdirTemp("", "algoErrTest") - if err != nil { - log.Fatal(err) - } installer = &BaseK8sInstaller{ - BundlePath: mockUbuntu.BundlePath, K8sStepProvider: &mockUbuntu, OutputBuilder: &outputBuilderCounter} }) - AfterEach(func() { - err = os.RemoveAll(mockUbuntu.BundlePath) - if err != nil { - log.Fatal(err) - } - }) It("Should rollback all applied steps", func() { - err = installer.Install() + err := installer.Install() Expect(err).Should((HaveOccurred())) - Expect(outputBuilderCounter.LogCalledCnt).Should(Equal(obcExpectedTotal)) + Expect(mockUbuntu.doCnt).Should(Equal(mockUbuntu.errorOnStep)) + Expect(mockUbuntu.undoCnt).Should(Equal(mockUbuntu.errorOnStep)) }) }) }) diff --git a/agent/installer/internal/algo/mock_ubuntu_with_error.go b/agent/installer/internal/algo/mock_ubuntu_with_error.go index 810d4387c..f7fac7e7d 100644 --- a/agent/installer/internal/algo/mock_ubuntu_with_error.go +++ b/agent/installer/internal/algo/mock_ubuntu_with_error.go @@ -3,67 +3,95 @@ package algo +import "errors" + +type mockStep struct { + Step + Desc string + Err error + doCnt *int + undoCnt *int + *BaseK8sInstaller +} + +func (s *mockStep) do() error { + s.OutputBuilder.Msg("Installing: " + s.Desc) + *s.doCnt++ + return s.Err +} + +func (s *mockStep) undo() error { + s.OutputBuilder.Msg("Uninstalling: " + s.Desc) + *s.undoCnt++ + return s.Err +} + type MockUbuntuWithError struct { BaseK8sInstaller + errorOnStep int + curStep int + doCnt int + undoCnt int } -func getEmptyStep(desc string, bki *BaseK8sInstaller) Step { - return &ShellStep{ +func (u *MockUbuntuWithError) getEmptyStep(desc string, bki *BaseK8sInstaller) Step { + u.curStep++ + var err error + if u.curStep == u.errorOnStep { + err = errors.New("error") + } + return &mockStep{ BaseK8sInstaller: bki, Desc: desc, - DoCmd: `:`, - UndoCmd: `:`} + Err: err, + doCnt: &u.doCnt, + undoCnt: &u.undoCnt} } func (u *MockUbuntuWithError) swapStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("SWAP", bki) + return u.getEmptyStep("SWAP", bki) } func (u *MockUbuntuWithError) firewallStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("FIREWALL", bki) + return u.getEmptyStep("FIREWALL", bki) } func (u *MockUbuntuWithError) kernelModsLoadStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("KERNEL MODULES", bki) + return u.getEmptyStep("KERNEL MODULES", bki) } func (u *MockUbuntuWithError) unattendedUpdStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("AUTO OS UPGRADES", bki) + return u.getEmptyStep("AUTO OS UPGRADES", bki) } func (u *MockUbuntuWithError) osWideCfgUpdateStep(bki *BaseK8sInstaller) Step { - // Here DoCmd should return error when executed - return &ShellStep{ - BaseK8sInstaller: bki, - Desc: "OS CONFIGURATION", - DoCmd: `a`, - UndoCmd: `:`} + return u.getEmptyStep("OS CONFIGURATION", bki) } func (u *MockUbuntuWithError) criToolsStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("cri-tools.deb", bki) + return u.getEmptyStep("cri-tools.deb", bki) } func (u *MockUbuntuWithError) criKubernetesStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("kubernetes-cni.deb", bki) + return u.getEmptyStep("kubernetes-cni.deb", bki) } func (u *MockUbuntuWithError) kubectlStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("kubectl.deb", bki) + return u.getEmptyStep("kubectl.deb", bki) } func (u *MockUbuntuWithError) kubeadmStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("kubeadm.deb", bki) + return u.getEmptyStep("kubeadm.deb", bki) } func (u *MockUbuntuWithError) kubeletStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("kubelet.deb", bki) + return u.getEmptyStep("kubelet.deb", bki) } func (u *MockUbuntuWithError) containerdStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("CONTAINERD", bki) + return u.getEmptyStep("CONTAINERD", bki) } func (u *MockUbuntuWithError) containerdDaemonStep(bki *BaseK8sInstaller) Step { - return getEmptyStep("CONTAINERD SERVICE", bki) + return u.getEmptyStep("CONTAINERD SERVICE", bki) } From f688f853f6c6c6d34f1fe448dd4af8ab61689def Mon Sep 17 00:00:00 2001 From: georgieva Date: Wed, 3 Nov 2021 14:04:22 +0200 Subject: [PATCH 3/3] Changed the check for rollback to be based on the names of the steps instead of only their count. --- agent/installer/internal/algo/algo_test.go | 11 ++++---- .../internal/algo/mock_ubuntu_with_error.go | 26 ++++++++----------- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/agent/installer/internal/algo/algo_test.go b/agent/installer/internal/algo/algo_test.go index e783f371d..2953b1367 100644 --- a/agent/installer/internal/algo/algo_test.go +++ b/agent/installer/internal/algo/algo_test.go @@ -50,17 +50,18 @@ var _ = Describe("Installer Algo Tests", func() { BeforeEach(func() { mockUbuntu = MockUbuntuWithError{} mockUbuntu.errorOnStep = 5 - mockUbuntu.OutputBuilder = &outputBuilderCounter installer = &BaseK8sInstaller{ - K8sStepProvider: &mockUbuntu, - OutputBuilder: &outputBuilderCounter} + K8sStepProvider: &mockUbuntu} }) It("Should rollback all applied steps", func() { err := installer.Install() Expect(err).Should((HaveOccurred())) - Expect(mockUbuntu.doCnt).Should(Equal(mockUbuntu.errorOnStep)) - Expect(mockUbuntu.undoCnt).Should(Equal(mockUbuntu.errorOnStep)) + Expect(len(mockUbuntu.doSteps)).Should(Equal(mockUbuntu.errorOnStep)) + Expect(len(mockUbuntu.undoSteps)).Should(Equal(mockUbuntu.errorOnStep)) + for i, sz := 0, len(mockUbuntu.doSteps); i < sz; i++ { + Expect(mockUbuntu.doSteps[i]).Should(Equal(mockUbuntu.undoSteps[sz-i-1])) + } }) }) }) diff --git a/agent/installer/internal/algo/mock_ubuntu_with_error.go b/agent/installer/internal/algo/mock_ubuntu_with_error.go index f7fac7e7d..7311b0e95 100644 --- a/agent/installer/internal/algo/mock_ubuntu_with_error.go +++ b/agent/installer/internal/algo/mock_ubuntu_with_error.go @@ -7,22 +7,19 @@ import "errors" type mockStep struct { Step - Desc string - Err error - doCnt *int - undoCnt *int + Desc string + Err error *BaseK8sInstaller + *MockUbuntuWithError } func (s *mockStep) do() error { - s.OutputBuilder.Msg("Installing: " + s.Desc) - *s.doCnt++ + s.doSteps = append(s.doSteps, s.Desc) return s.Err } func (s *mockStep) undo() error { - s.OutputBuilder.Msg("Uninstalling: " + s.Desc) - *s.undoCnt++ + s.undoSteps = append(s.undoSteps, s.Desc) return s.Err } @@ -30,8 +27,8 @@ type MockUbuntuWithError struct { BaseK8sInstaller errorOnStep int curStep int - doCnt int - undoCnt int + doSteps []string + undoSteps []string } func (u *MockUbuntuWithError) getEmptyStep(desc string, bki *BaseK8sInstaller) Step { @@ -41,11 +38,10 @@ func (u *MockUbuntuWithError) getEmptyStep(desc string, bki *BaseK8sInstaller) S err = errors.New("error") } return &mockStep{ - BaseK8sInstaller: bki, - Desc: desc, - Err: err, - doCnt: &u.doCnt, - undoCnt: &u.undoCnt} + BaseK8sInstaller: bki, + Desc: desc, + Err: err, + MockUbuntuWithError: u} } func (u *MockUbuntuWithError) swapStep(bki *BaseK8sInstaller) Step {