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.

Sign up for GitHub

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

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25e033a
daemon: Make binary writing idempotent
cgwalters Jul 27, 2023
edc68be
daemon: make error message explicit
sinnykumari Jul 28, 2023
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
49 changes: 27 additions & 22 deletions pkg/daemon/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah so we skip the write if the targetBin exists? Might be good to also make that explicit here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, updated

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 {
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