From 25e033aefb28c9ca227fe7e17588871c93049d53 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 27 Jul 2023 13:05:57 -0400 Subject: [PATCH 1/2] daemon: Make binary writing idempotent Fixes OCPBUGS-16921 --- pkg/daemon/daemon.go | 49 ++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 6a9f2079ca..d5d6a67791 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -471,35 +471,40 @@ func ReexecuteForTargetRoot(target string) error { } else { klog.Info("assuming we can use container binary chroot() to host") } - sourceBinary := "/usr/bin/machine-config-daemon" + sourceBinarySuffix - src, err := os.Open(sourceBinary) - if err != nil { - return fmt.Errorf("opening %s: %w", sourceBinary, err) - } - defer src.Close() targetBinBase := "run/bin/machine-config-daemon" targetBin := filepath.Join(target, targetBinBase) - targetBinDir := filepath.Dir(targetBin) - if _, err := os.Stat(targetBinDir); err != nil { - if err := os.Mkdir(targetBinDir, 0o755); err != nil { - return fmt.Errorf("mkdir %s: %w", targetBinDir, err) + + // Be idempotent + if _, err := os.Stat(targetBin); err != nil { + sourceBinary := "/usr/bin/machine-config-daemon" + sourceBinarySuffix + src, err := os.Open(sourceBinary) + if err != nil { + return fmt.Errorf("opening %s: %w", sourceBinary, err) } - } + defer src.Close() - f, err := os.Create(targetBin) - if err != nil { - return fmt.Errorf("writing %s: %w", targetBin, err) - } - if _, err := io.Copy(f, src); err != nil { + targetBinDir := filepath.Dir(targetBin) + if _, err := os.Stat(targetBinDir); err != nil { + if err := os.Mkdir(targetBinDir, 0o755); err != nil { + return fmt.Errorf("mkdir %s: %w", targetBinDir, err) + } + } + + f, err := os.Create(targetBin) + if err != nil { + return fmt.Errorf("writing %s: %w", targetBin, err) + } + if _, err := io.Copy(f, src); err != nil { + f.Close() + return fmt.Errorf("writing %s: %w", targetBin, err) + } + if err := f.Chmod(0o755); err != nil { + return err + } + // Must close our writable fd f.Close() - return fmt.Errorf("writing %s: %w", targetBin, err) - } - if err := f.Chmod(0o755); err != nil { - return err } - // Must close our writable fd - f.Close() if err := syscall.Chroot(target); err != nil { return fmt.Errorf("failed to chroot to %s: %w", target, err) From edc68bedee8fadadc81137333f01bf6fd610f043 Mon Sep 17 00:00:00 2001 From: Sinny Kumari Date: Fri, 28 Jul 2023 16:33:11 +0200 Subject: [PATCH 2/2] daemon: make error message explicit When we are skipping the mcd binary write when it already exists. Today this is the case for RHEL worker node during upgrade. Explicitly make sure that we are skipping because file doesn't exist --- pkg/daemon/daemon.go | 15 ++++++++++++--- pkg/daemon/update.go | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index d5d6a67791..8301faf038 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -466,7 +466,7 @@ func ReexecuteForTargetRoot(target string) error { // Otherwise, we assume that there's no suffixing needed. Hopefully // by RHEL10 the MCD will have fundamentally changed and we won't be doing the // chroot() thing anymore. - klog.Info("not chrooting for source=rhel-%s target=rhel-%s", sourceMajor, targetMajor) + klog.Infof("not chrooting for source=rhel-%s target=rhel-%s", sourceMajor, targetMajor) } } else { klog.Info("assuming we can use container binary chroot() to host") @@ -476,7 +476,11 @@ func ReexecuteForTargetRoot(target string) error { targetBin := filepath.Join(target, targetBinBase) // Be idempotent - if _, err := os.Stat(targetBin); err != nil { + targetBinExist, err := fileExists(targetBin) + if err != nil { + return err + } + if !targetBinExist { sourceBinary := "/usr/bin/machine-config-daemon" + sourceBinarySuffix src, err := os.Open(sourceBinary) if err != nil { @@ -485,7 +489,12 @@ func ReexecuteForTargetRoot(target string) error { defer src.Close() targetBinDir := filepath.Dir(targetBin) - if _, err := os.Stat(targetBinDir); err != nil { + // Before creating targetBinDir, ensure that it doesn't exist + targetBinDirExist, err := directoryExists(targetBinDir) + if err != nil { + return err + } + if !targetBinDirExist { if err := os.Mkdir(targetBinDir, 0o755); err != nil { return fmt.Errorf("mkdir %s: %w", targetBinDir, err) } diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index e4e7d9085c..ceb8b43588 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -1648,6 +1648,27 @@ func fileExists(path string) (bool, error) { return false, fmt.Errorf("cannot stat file: %w", err) } +// Determines if a directory exists by checking the returned error when we stat the file. +// Also, check that it is a directory. +func directoryExists(path string) (bool, error) { + info, err := os.Stat(path) + // If there is no error, check if it is a directory + if err == nil { + if info.IsDir() { + return true, nil + } + return false, fmt.Errorf("%s exists but it is not a directory", path) + } + + // If the error matches fs.ErrNotExist, file definitely does not exist. + if errors.Is(err, fs.ErrNotExist) { + return false, nil + } + + // An unexpected error occurred. + return false, fmt.Errorf("cannot stat file: %w", err) +} + // Removes the old SSH key path (/home/core/.ssh/authorized_keys), if found. func cleanSSHKeyPaths() error { oldKeyExists, err := fileExists(constants.RHCOS8SSHKeyPath)