Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCPBUGS-16921: daemon: Make binary writing idempotent #3825

Merged
merged 2 commits into from
Jul 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 36 additions & 22 deletions pkg/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -466,40 +466,54 @@ 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually sorry, hold on a second...I think I was wrong in our chat. We do need to chroot right? We just don't want to re-exec...

Something more like

diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go
index 6a9f2079c..2e18bf5da 100644
--- a/pkg/daemon/daemon.go
+++ b/pkg/daemon/daemon.go
@@ -471,35 +471,38 @@ 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()
+	var targetBin string
+	if sourceBinarySuffix != "" {
+		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)
+		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)
+			}
 		}
-	}
 
-	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, 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)
@@ -509,6 +512,9 @@ func ReexecuteForTargetRoot(target string) error {
 		return fmt.Errorf("failed to change directory to /: %w", err)
 	}
 
+	if targetBin == "" {
+		return nil
+	}
 	// Now we will see the binary in the target root
 	targetBin = "/" + targetBinBase
 	// We have a "belt and suspenders" approach for detecting the case where

?

But there's still something weird going on here because I don't understand how we can be recursing here...IOW why are we getting that error of having the text file being busy?

Copy link
Contributor Author

@sinnykumari sinnykumari Jul 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, we don't need these new binaries copy stuff but we do need to chroot. This happens when doing things in hurry :/

}
} 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)
}
}

f, err := os.Create(targetBin)
// Be idempotent
targetBinExist, err := fileExists(targetBin)
if err != nil {
return fmt.Errorf("writing %s: %w", targetBin, err)
return err
}
if _, err := io.Copy(f, src); err != nil {
if !targetBinExist {
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()

targetBinDir := filepath.Dir(targetBin)
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we making the assumption that the error of the above is errnotfound? Might be helpful to make it explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

umm, I am not sure adding here additional error check will add much value. Irrespective of what is the error, we are returning the error anyway with no further action needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant the error return from os.Stat(), must like my other comment below, not the os.Mkdir

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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)
Expand Down
21 changes: 21 additions & 0 deletions pkg/daemon/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down