From 878f744e9cae91dc85026ec0ab6046fb4bf75097 Mon Sep 17 00:00:00 2001 From: georgievaVMW <90902341+georgievaVMW@users.noreply.github.com> Date: Tue, 8 Feb 2022 09:28:41 +0200 Subject: [PATCH] Made changes to the methods New and newUnchecked (removed bundleRepo argument) (#316) Removed BundleDownloader argument from getSupportedRegistry as it was not used Made changes to installer tests and cli to work with the changes. Made changes to agent/reconciler/host_reconciler.go so tests run Updated the path where the bundle is contained to have the bundle-registry in the path. Made install and uninstall to set the bundleRepo Changed all required things so it can run with those changes. Made unit test to check if download dir is correctly created and renamed after successful download. Squashed all commits into one. --- agent/installer/bundle_downloader.go | 15 ++++++--- agent/installer/bundle_downloader_test.go | 19 +++++++++++ agent/installer/cli.go | 9 +++-- agent/installer/installer.go | 40 +++++++++++++---------- agent/installer/installer_test.go | 32 ++++++++---------- agent/reconciler/host_reconciler.go | 8 ++--- 6 files changed, 73 insertions(+), 50 deletions(-) diff --git a/agent/installer/bundle_downloader.go b/agent/installer/bundle_downloader.go index 14d52dfb6..6355f2510 100644 --- a/agent/installer/bundle_downloader.go +++ b/agent/installer/bundle_downloader.go @@ -50,7 +50,10 @@ func (bd *bundleDownloader) DownloadFromRepo( tag string, downloadByTool func(string, string) error) error { - err := ensureDirExist(bd.downloadPath) + downloadPathWithRepo := bd.getBundlePathWithRepo() + + err := ensureDirExist(downloadPathWithRepo) + defer os.Remove(downloadPathWithRepo) if err != nil { return err } @@ -65,7 +68,7 @@ func (bd *bundleDownloader) DownloadFromRepo( bd.logger.Info("Cache miss", "path", bundleDirPath) - dir, err := os.MkdirTemp(bd.downloadPath, "tempBundle") + dir, err := os.MkdirTemp(downloadPathWithRepo, "tempBundle") // It is fine if the dir path does not exist. defer func() { err = os.RemoveAll(dir) @@ -76,7 +79,6 @@ func (bd *bundleDownloader) DownloadFromRepo( if err != nil { return err } - bundleAddr := bd.getBundleAddr(normalizedOsVersion, k8sVersion, tag) err = convertError(downloadByTool(bundleAddr, dir)) if err != nil { @@ -125,7 +127,7 @@ func (bd *bundleDownloader) GetBundleDirPath(k8sVersion, tag string) string { // Not storing tag as a subdir of k8s because we can't atomically move // the temp bundle dir to a non-existing dir. // Using "-" instead of ":" because Windows doesn't like the latter - return fmt.Sprintf("%s-%s", filepath.Join(bd.downloadPath, k8sVersion), tag) + return fmt.Sprintf("%s-%s", filepath.Join(bd.getBundlePathWithRepo(), k8sVersion), tag) } // GetBundleName returns the name of the bundle in normalized format. @@ -133,6 +135,11 @@ func GetBundleName(normalizedOsVersion, k8sVersion string) string { return strings.ToLower(fmt.Sprintf("byoh-bundle-%s_k8s_%s", normalizedOsVersion, k8sVersion)) } +// getBundlePathWithRepo returns the path +func (bd *bundleDownloader) getBundlePathWithRepo() string { + return filepath.Join(bd.downloadPath, strings.ReplaceAll(bd.repoAddr, "/", ".")) +} + // getBundleAddr returns the exact address to the bundle in the repo. func (bd *bundleDownloader) getBundleAddr(normalizedOsVersion, k8sVersion, tag string) string { return fmt.Sprintf("%s/%s:%s", bd.repoAddr, GetBundleName(normalizedOsVersion, k8sVersion), tag) diff --git a/agent/installer/bundle_downloader_test.go b/agent/installer/bundle_downloader_test.go index f05b4340f..d8f2cb92a 100644 --- a/agent/installer/bundle_downloader_test.go +++ b/agent/installer/bundle_downloader_test.go @@ -84,6 +84,25 @@ var _ = Describe("Byohost Installer Tests", func() { mi.Get) Expect(err).ShouldNot(HaveOccurred()) }) + It("Should create and rename dir correctly after successful download", func() { + bd.repoAddr = "repo.ccoomm/r/" + bd.downloadPath = filepath.Join(bd.downloadPath) + err := bd.DownloadFromRepo( + normalizedOsVersion, + k8sVersion, + testTag, + mi.Get) + Expect(err).ShouldNot((HaveOccurred())) + _, err = os.Stat(bd.GetBundleDirPath(k8sVersion, testTag)) + Expect(err).ShouldNot((HaveOccurred())) + notExist := os.IsNotExist(err) + Expect(notExist).ShouldNot(BeTrue()) + + _, err = os.Stat(bd.GetBundleDirPath(k8sVersion+"a", testTag)) + Expect(err).Should((HaveOccurred())) + notExist = os.IsNotExist(err) + Expect(notExist).Should(BeTrue()) + }) }) Context("When there is error during download", func() { It("Should return error if given bad repo", func() { diff --git a/agent/installer/cli.go b/agent/installer/cli.go index 321bb0c63..70685ae57 100644 --- a/agent/installer/cli.go +++ b/agent/installer/cli.go @@ -130,23 +130,22 @@ func runInstaller(install bool) { var err error if *osFlag != "" { // Override current OS detection - i, err = newUnchecked(*osFlag, *bundleRepoFlag, *cachePathFlag, klogger, &logPrinter{klogger}) + i, err = newUnchecked(*osFlag, *cachePathFlag, klogger, &logPrinter{klogger}) if err != nil { klogger.Error(err, "unable to create installer") return } } else { - i, err = New(*bundleRepoFlag, *cachePathFlag, klogger) + i, err = New(*cachePathFlag, klogger) if err != nil { klogger.Error(err, "unable to create installer") return } } - if install { - err = i.Install(*k8sFlag, *tagFlag) + err = i.Install(*bundleRepoFlag, *k8sFlag, *tagFlag) } else { - err = i.Uninstall(*k8sFlag, *tagFlag) + err = i.Uninstall(*bundleRepoFlag, *k8sFlag, *tagFlag) } if err != nil { klogger.Error(err, "error installing/uninstalling") diff --git a/agent/installer/installer.go b/agent/installer/installer.go index 095a0087f..35e9332ae 100644 --- a/agent/installer/installer.go +++ b/agent/installer/installer.go @@ -32,7 +32,7 @@ type installer struct { } // getSupportedRegistry returns a registry with installers for the supported OS and K8s -func getSupportedRegistry(bd *bundleDownloader, ob algo.OutputBuilder) registry { +func getSupportedRegistry(ob algo.OutputBuilder) registry { reg := newRegistry() addBundleInstaller := func(osBundle, k8sBundle string, stepProvider algo.K8sStepProvider) { @@ -85,13 +85,10 @@ func (bd *bundleDownloader) DownloadOrPreview(os, k8s, tag string) error { return bd.Download(os, k8s, tag) } -// New returns an installer that downloads bundles for the current OS from OCI repository with -// address bundleRepo and stores them under downloadPath. Download path is created, +// New returns an installer that downloads bundles for the current OS +// and stores them under downloadPath. Download path is created, // if it does not exist. -func New(bundleRepo, downloadPath string, logger logr.Logger) (*installer, error) { - if bundleRepo == "" { - return nil, fmt.Errorf("empty bundle repo") - } +func New(downloadPath string, logger logr.Logger) (*installer, error) { if downloadPath == "" { return nil, fmt.Errorf("empty download path") } @@ -103,16 +100,16 @@ func New(bundleRepo, downloadPath string, logger logr.Logger) (*installer, error return nil, ErrDetectOs } - return newUnchecked(os, bundleRepo, downloadPath, logger, &logPrinter{logger}) + return newUnchecked(os, downloadPath, logger, &logPrinter{logger}) } -// newUnchecked returns an installer bypassing os detection and checks of bundleRepo and downloadPath. -// If they are empty, returned installer will run in preview mode, i.e. +// newUnchecked returns an installer bypassing os detection and checks of downloadPath. +// If it is empty, returned installer will run in preview mode, i.e. // executes everything except the actual commands. -func newUnchecked(currentOs, bundleRepo, downloadPath string, logger logr.Logger, outputBuilder algo.OutputBuilder) (*installer, error) { - bd := bundleDownloader{repoAddr: bundleRepo, downloadPath: downloadPath, logger: logger} +func newUnchecked(currentOs, downloadPath string, logger logr.Logger, outputBuilder algo.OutputBuilder) (*installer, error) { + bd := bundleDownloader{repoAddr: "", downloadPath: downloadPath, logger: logger} - reg := getSupportedRegistry(&bd, outputBuilder) + reg := getSupportedRegistry(outputBuilder) if len(reg.ListK8s(currentOs)) == 0 { return nil, ErrOsK8sNotSupported } @@ -124,8 +121,14 @@ func newUnchecked(currentOs, bundleRepo, downloadPath string, logger logr.Logger logger: logger}, nil } +// setBundleRepo sets the repo from which the bundle will be downloaded. +func (i *installer) setBundleRepo(bundleRepo string) { + i.bundleDownloader.repoAddr = bundleRepo +} + // Install installs the specified k8s version on the current OS -func (i *installer) Install(k8sVer, tag string) error { +func (i *installer) Install(bundleRepo, k8sVer, tag string) error { + i.setBundleRepo(bundleRepo) algoInst, err := i.getAlgoInstallerWithBundle(k8sVer, tag) if err != nil { return err @@ -138,8 +141,9 @@ func (i *installer) Install(k8sVer, tag string) error { return nil } -// Uninstall uninstalls the specified k8s version on the current OS -func (i *installer) Uninstall(k8sVer, tag string) error { +// Uninstal uninstalls the specified k8s version on the current OS +func (i *installer) Uninstall(bundleRepo, k8sVer, tag string) error { + i.setBundleRepo(bundleRepo) algoInst, err := i.getAlgoInstallerWithBundle(k8sVer, tag) if err != nil { return err @@ -191,7 +195,7 @@ func ListSupportedK8s(os string) []string { // getSupportedRegistryDescription returns a description registry of supported OS and k8s. // It that can only be queried for OS and k8s but cannot be used for install/uninstall. func getSupportedRegistryDescription() registry { - return getSupportedRegistry(nil, nil) + return getSupportedRegistry(nil) } // PreviewChanges describes the changes to install and uninstall K8s on OS without actually applying them. @@ -199,7 +203,7 @@ func getSupportedRegistryDescription() registry { // Can be invoked on a non-supported OS func PreviewChanges(os, k8sVer string) (install, uninstall string, err error) { stepPreviewer := stringPrinter{msgFmt: "# %s"} - reg := getSupportedRegistry(&bundleDownloader{}, &stepPreviewer) + reg := getSupportedRegistry(&stepPreviewer) installer, _ := reg.GetInstaller(os, k8sVer) if installer == nil { diff --git a/agent/installer/installer_test.go b/agent/installer/installer_test.go index f3e59c257..28bd5fe07 100644 --- a/agent/installer/installer_test.go +++ b/agent/installer/installer_test.go @@ -16,20 +16,14 @@ var _ = Describe("Byohost Installer Tests", func() { Context("When installer is created for unsupported OS", func() { It("Should return error", func() { - _, err := newUnchecked("Ubuntu_99.04.3_x86-64", "", "", logr.Discard(), nil) - Expect(err).Should(HaveOccurred()) - }) - }) - Context("When installer is created with empty bundle repo", func() { - It("Should return error", func() { - _, err := New("", "downloadPath", logr.Discard()) - Expect(err).Should(HaveOccurred()) + _, err := newUnchecked("Ubuntu_99.04.3_x86-64", "", logr.Discard(), nil) + Expect(err).Should((HaveOccurred())) }) }) Context("When installer is created with empty download path", func() { It("Should return error", func() { - _, err := New("repo", "", logr.Discard()) - Expect(err).Should(HaveOccurred()) + _, err := New("", logr.Discard()) + Expect(err).Should((HaveOccurred())) }) }) Context("When installer is created", func() { @@ -38,11 +32,11 @@ var _ = Describe("Byohost Installer Tests", func() { for _, os := range osList { i := NewPreviewInstaller(os, nil) - err := i.Install("unsupported-k8s", testTag) - Expect(err).Should(HaveOccurred()) + err := i.Install("", "unsupported-k8s", testTag) + Expect(err).Should((HaveOccurred())) - err = i.Uninstall("unsupported-k8s", testTag) - Expect(err).Should(HaveOccurred()) + err = i.Uninstall("", "unsupported-k8s", testTag) + Expect(err).Should((HaveOccurred())) } }) }) @@ -54,16 +48,16 @@ var _ = Describe("Byohost Installer Tests", func() { { ob := algo.OutputBuilderCounter{} i := NewPreviewInstaller(os, &ob) - err := i.Install(k8s, testTag) - Expect(err).ShouldNot(HaveOccurred()) + err := i.Install("", k8s, testTag) + Expect(err).ShouldNot((HaveOccurred())) Expect(ob.LogCalledCnt).Should(Equal(22)) } { ob := algo.OutputBuilderCounter{} i := NewPreviewInstaller(os, &ob) - err := i.Uninstall(k8s, testTag) - Expect(err).ShouldNot(HaveOccurred()) + err := i.Uninstall("", k8s, testTag) + Expect(err).ShouldNot((HaveOccurred())) Expect(ob.LogCalledCnt).Should(Equal(22)) } } @@ -131,7 +125,7 @@ var _ = Describe("Byohost Installer Tests", func() { }) func NewPreviewInstaller(os string, ob algo.OutputBuilder) *installer { - i, err := newUnchecked(os, "", "", logr.Discard(), ob) + i, err := newUnchecked(os, "", logr.Discard(), ob) if err != nil { panic(err) } diff --git a/agent/reconciler/host_reconciler.go b/agent/reconciler/host_reconciler.go index 8e8491e70..628aea285 100644 --- a/agent/reconciler/host_reconciler.go +++ b/agent/reconciler/host_reconciler.go @@ -254,11 +254,11 @@ func (r *HostReconciler) installK8sComponents(ctx context.Context, byoHost *infr bundleRegistry := byoHost.GetAnnotations()[infrastructurev1beta1.BundleLookupBaseRegistryAnnotation] k8sVersion := byoHost.GetAnnotations()[infrastructurev1beta1.K8sVersionAnnotation] byohBundleTag := byoHost.GetAnnotations()[infrastructurev1beta1.BundleLookupTagAnnotation] - bundleInstaller, err := installer.New(bundleRegistry, r.DownloadPath, logger) + bundleInstaller, err := installer.New(r.DownloadPath, logger) if err != nil { return err } - err = bundleInstaller.Install(k8sVersion, byohBundleTag) + err = bundleInstaller.Install(bundleRegistry, k8sVersion, byohBundleTag) if err != nil { return err } @@ -274,11 +274,11 @@ func (r *HostReconciler) uninstallk8sComponents(ctx context.Context, byoHost *in bundleRegistry := byoHost.GetAnnotations()[infrastructurev1beta1.BundleLookupBaseRegistryAnnotation] k8sVersion := byoHost.GetAnnotations()[infrastructurev1beta1.K8sVersionAnnotation] byohBundleTag := byoHost.GetAnnotations()[infrastructurev1beta1.BundleLookupTagAnnotation] - bundleInstaller, err := installer.New(bundleRegistry, r.DownloadPath, logger) + bundleInstaller, err := installer.New(r.DownloadPath, logger) if err != nil { return err } - err = bundleInstaller.Uninstall(k8sVersion, byohBundleTag) + err = bundleInstaller.Uninstall(bundleRegistry, k8sVersion, byohBundleTag) if err != nil { return err }