Skip to content
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
1 change: 1 addition & 0 deletions Dockerfile.assisted_installer_agent
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ RUN dnf install -y --nodocs --setopt=install_weak_deps=False \
chrony \
kmod \
golang \
rpm-ostree \
&& dnf clean all

# Set Go environment variables for compatibility with e2e tests
Expand Down
276 changes: 246 additions & 30 deletions src/commands/actions/download_boot_artifacts_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"time"

"github.com/openshift/assisted-installer-agent/src/config"
"github.com/openshift/assisted-installer-agent/src/util"
"github.com/openshift/assisted-service/models"
log "github.com/sirupsen/logrus"
)
Expand Down Expand Up @@ -45,10 +46,25 @@ func (a *downloadBootArtifacts) Args() []string {
return a.args
}

// Helper functions to build mounted folder paths from hostFsMountDir
func getMountedBootFolder(hostFsMountDir string) string {
return path.Join(hostFsMountDir, "boot")
}

func getMountedArtifactsFolder(hostFsMountDir string) string {
return path.Join(hostFsMountDir, "boot", artifactsFolder)
}

func getMountedBootLoaderFolder(hostFsMountDir string) string {
return path.Join(hostFsMountDir, "boot", bootLoaderFolder)
}

const (
retryDownloadAmount = 5
defaultDownloadRetryDelay = 1 * time.Minute
artifactsFolder string = "/boot/discovery"
defaultRetryAmount = 5
defaultRetryDelay = 1 * time.Minute
artifactsFolder string = "/discovery"
bootLoaderFolder string = "/loader/entries"
tempBootArtifactsFolder string = "/tmp/boot"
kernelFile string = "vmlinuz"
initrdFile string = "initrd"
bootLoaderConfigFileName string = "/00-assisted-discovery.conf"
Expand All @@ -70,38 +86,60 @@ func run(infraEnvId, downloaderRequestStr, caCertPath string) error {
return fmt.Errorf("failed unmarshalling download boot artifacts request: %w", err)
}

bootFolder := path.Join(*req.HostFsMountDir, "/boot")
if err := syscall.Mount(bootFolder, bootFolder, "", syscall.MS_REMOUNT, ""); err != nil {
return fmt.Errorf("failed remounting /host/boot folder as rw: %w", err)
if bootArtifactsExist(*req.HostFsMountDir) {
log.Info("Boot artifacts are already present on the host in the /boot folder")
return nil
}

hostArtifactsFolder := path.Join(*req.HostFsMountDir, artifactsFolder)
bootLoaderFolder := path.Join(*req.HostFsMountDir, "/boot/loader/entries")
if err := createFolders(hostArtifactsFolder, bootLoaderFolder); err != nil {
return fmt.Errorf("failed creating folders: %w", err)
err := createFolders(*req.HostFsMountDir, defaultRetryAmount)
if err != nil {
log.Errorf("failed creating folders: %s", err.Error())
return fmt.Errorf("failed creating folders: %s", err.Error())
}

httpClient, err := createHTTPClient(caCertPath)
if err != nil {
return fmt.Errorf("failed creating secure assisted service client: %w", err)
if err := downloadArtifactsToTempFolder(req, caCertPath); err != nil {
log.Errorf("failed downloading boot artifacts: %s", err.Error())
return fmt.Errorf("failed downloading boot artifacts: %s", err.Error())
}
log.Info("Successfully downloaded boot artifacts")

if err := download(httpClient, path.Join(hostArtifactsFolder, kernelFile), *req.KernelURL, retryDownloadAmount); err != nil {
return fmt.Errorf("failed downloading kernel to host: %w", err)
if err := createBootLoaderConfigInTempFolder(*req.RootfsURL); err != nil {
log.Errorf("failed creating bootloader config file on host: %s", err.Error())
return fmt.Errorf("failed creating bootloader config file on host: %s", err.Error())
}
log.Infof("Successfully wrote bootloader config to %s", path.Join(tempBootArtifactsFolder, bootLoaderConfigFileName))

if err := download(httpClient, path.Join(hostArtifactsFolder, initrdFile), *req.InitrdURL, retryDownloadAmount); err != nil {
return fmt.Errorf("failed downloading initrd to host: %w", err)
if err := ensureBootHasSpace(getMountedBootFolder(*req.HostFsMountDir)); err != nil {
log.Errorf("failed to ensure boot folder has enough space: %s", err.Error())
return fmt.Errorf("failed to ensure boot folder has enough space: %s", err.Error())
}

if err := createBootLoaderConfig(*req.RootfsURL, artifactsFolder, bootLoaderFolder); err != nil {
return fmt.Errorf("failed creating bootloader config file on host: %w", err)
if err := copyFilesToBootFolder(*req.HostFsMountDir); err != nil {
log.Errorf("failed to move files to boot folder: %s", err.Error())
return fmt.Errorf("failed to move files to boot folder: %s", err.Error())
}

log.Infof("Successfully downloaded boot artifacts and created bootloader config.")
log.Infof("Download boot artifacts completed successfully.")
return nil
}

// bootArtifactsExist checks if the boot artifacts already exist in the host filesystem
func bootArtifactsExist(hostFsMountDir string) bool {
kernelFilePath := path.Join(getMountedArtifactsFolder(hostFsMountDir), kernelFile)
initrdFilePath := path.Join(getMountedArtifactsFolder(hostFsMountDir), initrdFile)
bootLoaderConfigFilePath := path.Join(getMountedBootLoaderFolder(hostFsMountDir), bootLoaderConfigFileName)
_, err := os.Stat(kernelFilePath)
if err != nil {
return false
}
_, err = os.Stat(initrdFilePath)
if err != nil {
return false
}
_, err = os.Stat(bootLoaderConfigFilePath)
return err == nil
}

func createHTTPClient(caCertPath string) (*http.Client, error) {
client := &http.Client{}
if caCertPath != "" {
Expand Down Expand Up @@ -136,7 +174,7 @@ func download(httpClient *http.Client, filePath, url string, retry int) error {
downloadErr = fmt.Errorf("failed downloading boot artifact from %s, status code received: %d, attempt %d/%d, download error: %w",
url, res.StatusCode, attempts, retry, downloadErr)
log.Warn(downloadErr.Error())
time.Sleep(defaultDownloadRetryDelay)
time.Sleep(defaultRetryDelay)
}

if downloadErr != nil {
Expand All @@ -155,16 +193,34 @@ func download(httpClient *http.Client, filePath, url string, retry int) error {
return nil
}

func createBootLoaderConfig(rootfsUrl, artifactsPath, bootLoaderPath string) error {
kernelPath := path.Join(artifactsPath, kernelFile)
initrdPath := path.Join(artifactsPath, initrdFile)
bootLoaderConfigFile := path.Join(bootLoaderPath, bootLoaderConfigFileName)
func downloadArtifactsToTempFolder(req models.DownloadBootArtifactsRequest, caCertPath string) error {
httpClient, err := createHTTPClient(caCertPath)
if err != nil {
return fmt.Errorf("failed creating secure assisted service client: %s", err.Error())
}

if err := download(httpClient, path.Join(tempBootArtifactsFolder, kernelFile), *req.KernelURL, defaultRetryAmount); err != nil {
return fmt.Errorf("failed downloading kernel to host: %s", err.Error())
}

if err := download(httpClient, path.Join(tempBootArtifactsFolder, initrdFile), *req.InitrdURL, defaultRetryAmount); err != nil {
return fmt.Errorf("failed downloading initrd to host: %s", err.Error())
}
return nil
}

func createBootLoaderConfigInTempFolder(rootfsUrl string) error {
// These are the actual paths to the kernel and initrd in the actual host
kernelPath := path.Join("/boot", artifactsFolder, kernelFile)
initrdPath := path.Join("/boot", artifactsFolder, initrdFile)

var bootLoaderConfig string
bootLoaderConfig = fmt.Sprintf(bootLoaderConfigTemplate, rootfsUrl, kernelPath, initrdPath)
if runtime.GOARCH == "s390x" {
bootLoaderConfig = fmt.Sprintf(bootLoaderConfigTemplateS390x, rootfsUrl, kernelPath, initrdPath)
}

bootLoaderConfigFile := path.Join(tempBootArtifactsFolder, bootLoaderConfigFileName)
if err := os.WriteFile(bootLoaderConfigFile, []byte(bootLoaderConfig), 0644); err != nil { //nolint:gosec
return fmt.Errorf("failed writing bootloader config content to %s: %w", bootLoaderConfigFile, err)
}
Expand All @@ -178,14 +234,174 @@ func createFolderIfNotExist(folder string) error {
return nil
}

func createFolders(artifactsPath, bootLoaderPath string) error {
err := createFolderIfNotExist(artifactsPath)
func createFolders(hostFsMountDir string, retryAmount int) error {
var err error
mountedBootFolder := getMountedBootFolder(hostFsMountDir)
mountedArtifactsFolder := getMountedArtifactsFolder(hostFsMountDir)
mountedBootLoaderFolder := getMountedBootLoaderFolder(hostFsMountDir)

for i := 0; i < retryAmount; i++ {
log.Debugf("Creating folders attempt %d/%d", i, retryAmount)
err = syscall.Mount(mountedBootFolder, mountedBootFolder, "", syscall.MS_REMOUNT, "")
if err != nil {
log.Warnf("failed to mount boot folder [%s]: %s\nRetrying in %s", mountedBootFolder, err.Error(), defaultRetryDelay)
time.Sleep(defaultRetryDelay)
continue
}
syscall.Sync()
if err = createFolderIfNotExist(mountedArtifactsFolder); err != nil {
log.Warnf("failed to create artifacts folder [%s]: %s\nRetrying in %s", mountedArtifactsFolder, err.Error(), defaultRetryDelay)
time.Sleep(defaultRetryDelay)
continue
}
if err = createFolderIfNotExist(mountedBootLoaderFolder); err != nil {
log.Warnf("failed to create bootloader folder [%s]: %s\nRetrying in %s", mountedBootLoaderFolder, err.Error(), defaultRetryDelay)
time.Sleep(defaultRetryDelay)
continue
}
if err = createFolderIfNotExist(tempBootArtifactsFolder); err != nil {
log.Warnf("failed to create temp boot artifacts folder [%s]: %s\nRetrying in %s", tempBootArtifactsFolder, err.Error(), defaultRetryDelay)
time.Sleep(defaultRetryDelay)
continue
}
log.Debug("All folders created successfully")
return nil
}
return fmt.Errorf("failed to create folders: %w", err)
}

func ensureBootHasSpace(hostFsMountDir string) error {
mountedBootFolder := getMountedBootFolder(hostFsMountDir)
artifactsSize, err := calculateBootArtifactsSize()
if err != nil {
return fmt.Errorf("failed to calculate size of boot artifacts: %w", err)
}
log.Debugf("Boot artifacts total size: %d bytes", artifactsSize)

freeSpace, err := getFreeSpace(mountedBootFolder)
if err != nil {
return fmt.Errorf("failed to get free space of boot folder [%s]: %w", mountedBootFolder, err)
}
log.Debugf("Free space in boot folder: %d bytes", freeSpace)

if freeSpace > artifactsSize {
return nil
}

log.Warnf("Boot folder does not have enough space. Wanted: %d bytes, Available: %d bytes. Attempting to reclaim space", artifactsSize, freeSpace)
if err = reclaimBootFolderSpace(); err != nil {
return fmt.Errorf("failed to reclaim boot folder space: %w", err)
}

freeSpace, err = getFreeSpace(mountedBootFolder)
if err != nil {
return fmt.Errorf("failed to get free space of boot folder [%s]: %w", mountedBootFolder, err)
}

if freeSpace < artifactsSize {
return fmt.Errorf("boot folder does not have enough space, artifacts size: %d, free space: %d\nRetrying...", artifactsSize, freeSpace)
}
return nil
}

// getFreeSpace returns the available space in the given folder
func getFreeSpace(folder string) (uint64, error) {
var stat syscall.Statfs_t
if err := syscall.Statfs(folder, &stat); err != nil {
return 0, fmt.Errorf("failed to statfs %s: %w", folder, err)
}
// G115: Integer overflow is not possible for realistic filesystem sizes (<< uint64 max)
return stat.Bavail * uint64(stat.Bsize), nil // #nosec G115
}

// calculateBootArtifactsSize calculates the total size of downloaded boot artifacts and bootloader config
func calculateBootArtifactsSize() (uint64, error) {
var totalSize uint64

// Calculate size of kernel file
kernelPath := path.Join(tempBootArtifactsFolder, kernelFile)
kernelInfo, err := os.Stat(kernelPath)
if err != nil {
return 0, fmt.Errorf("failed to stat kernel file %s: %w", kernelPath, err)
}
// G115: Integer overflow is not possible for realistic filesystem sizes (<< uint64 max)
totalSize += uint64(kernelInfo.Size()) // #nosec G115

// Calculate size of initrd file
initrdPath := path.Join(tempBootArtifactsFolder, initrdFile)
initrdInfo, err := os.Stat(initrdPath)
if err != nil {
return fmt.Errorf("failed to create artifacts folder [%s]: %w", artifactsPath, err)
return 0, fmt.Errorf("failed to stat initrd file %s: %w", initrdPath, err)
}
err = createFolderIfNotExist(bootLoaderPath)
// G115: Integer overflow is not possible for realistic filesystem sizes (<< uint64 max)
totalSize += uint64(initrdInfo.Size()) // #nosec G115

// Calculate size of bootloader config file
bootLoaderConfigPath := path.Join(tempBootArtifactsFolder, bootLoaderConfigFileName)
bootLoaderInfo, err := os.Stat(bootLoaderConfigPath)
if err != nil {
return fmt.Errorf("failed to create bootloader folder [%s]: %w", bootLoaderPath, err)
return 0, fmt.Errorf("failed to stat bootloader config file %s: %w", bootLoaderConfigPath, err)
}
// G115: Integer overflow is not possible for realistic filesystem sizes (<< uint64 max)
totalSize += uint64(bootLoaderInfo.Size()) // #nosec G115

return totalSize, nil
}

func reclaimBootFolderSpace() error {
stdout, stderr, exitCode := util.ExecutePrivileged("rpm-ostree", "cleanup", "--os=rhcos", "-r")
log.Debugf("Cleanup RHCOS stdout: %s\nstderr: %s\nexitCode: %d", stdout, stderr, exitCode)
if exitCode != 0 {
return fmt.Errorf("Cleanup command for RHCOS failed: %s: %s", stdout, stderr)
}
log.Info("Successfully cleaned up RHCOS")
return nil
}
Comment on lines +351 to +359
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing retry logic for rpm-ostree cleanup — contradicts PR description.

The PR description explicitly states "Includes retry logic to attempt the cleanup again if the first attempt fails," but reclaimBootFolderSpace runs rpm-ostree cleanup exactly once. Transient failures (e.g., RPM database lock contention) will surface as permanent errors with no recovery opportunity.

🐛 Proposed fix
+const reclaimRetryAmount = 3
+const defaultReclaimRetryDelay = 30 * time.Second
+
 func reclaimBootFolderSpace() error {
-	stdout, stderr, exitCode := util.ExecutePrivileged("rpm-ostree", "cleanup", "--os=rhcos", "-r")
-	log.Debugf("Cleanup RHCOS stdout: %s\nstderr: %s\nexitCode: %d", stdout, stderr, exitCode)
-	if exitCode != 0 {
-		return fmt.Errorf("Cleanup command for RHCOS failed: %s: %s", stdout, stderr)
-	}
-	log.Info("Successfully cleaned up RHCOS")
-	return nil
+	var lastErr error
+	for i := 0; i < reclaimRetryAmount; i++ {
+		stdout, stderr, exitCode := util.ExecutePrivileged("rpm-ostree", "cleanup", "--os=rhcos", "-r")
+		log.Debugf("Cleanup RHCOS stdout: %s\nstderr: %s\nexitCode: %d", stdout, stderr, exitCode)
+		if exitCode == 0 {
+			log.Info("Successfully cleaned up RHCOS")
+			return nil
+		}
+		lastErr = fmt.Errorf("cleanup command for RHCOS failed: %s: %s", stdout, stderr)
+		log.WithError(lastErr).Errorf("Failed to reclaim boot space (attempt %d/%d)", i+1, reclaimRetryAmount)
+		if i < reclaimRetryAmount-1 {
+			time.Sleep(defaultReclaimRetryDelay)
+		}
+	}
+	return lastErr
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/actions/download_boot_artifacts_cmd.go` around lines 314 - 322,
The reclaimBootFolderSpace function currently calls
util.ExecutePrivileged("rpm-ostree", "cleanup", "--os=rhcos", "-r") only once;
add retry logic around that call (in reclaimBootFolderSpace) to attempt the
command multiple times (e.g., 3 attempts) with a short backoff between attempts
(time.Sleep) and log each attempt via log.Debugf/log.Warnf; on success return
nil immediately, on repeated failure return the final fmt.Errorf including
stdout/stderr/exitCode from util.ExecutePrivileged so callers can see the last
error. Ensure you reference the util.ExecutePrivileged call and preserve the
existing log messages (including the successful log.Info on success).


func copyFilesToBootFolder(hostFsMountDir string) error {
mountedArtifactsFolder := getMountedArtifactsFolder(hostFsMountDir)
mountedBootLoaderFolder := getMountedBootLoaderFolder(hostFsMountDir)
if err := copyFile(path.Join(tempBootArtifactsFolder, kernelFile), path.Join(mountedArtifactsFolder, kernelFile)); err != nil {
return fmt.Errorf("failed to copy file %s to %s: %w", path.Join(tempBootArtifactsFolder, kernelFile), path.Join(mountedArtifactsFolder, kernelFile), err)
}
if err := copyFile(path.Join(tempBootArtifactsFolder, initrdFile), path.Join(mountedArtifactsFolder, initrdFile)); err != nil {
return fmt.Errorf("failed to copy file %s to %s: %w", path.Join(tempBootArtifactsFolder, initrdFile), path.Join(mountedArtifactsFolder, initrdFile), err)
}
if err := copyFile(path.Join(tempBootArtifactsFolder, bootLoaderConfigFileName), path.Join(mountedBootLoaderFolder, bootLoaderConfigFileName)); err != nil {
return fmt.Errorf("failed to copy file %s to %s: %w", path.Join(tempBootArtifactsFolder, bootLoaderConfigFileName), path.Join(mountedBootLoaderFolder, bootLoaderConfigFileName), err)
}

log.Info("Successfully copied files to /boot folder.")
return nil
}

func copyFile(src, dst string) error {
sourceFile, err := os.Open(src)
if err != nil {
return fmt.Errorf("failed to open source file %s: %w", src, err)
}
defer sourceFile.Close()

destFile, err := os.Create(dst)
if err != nil {
return fmt.Errorf("failed to create destination file %s: %w", dst, err)
}
defer destFile.Close()

if _, err = io.Copy(destFile, sourceFile); err != nil {
return fmt.Errorf("failed to copy data from %s to %s: %w", src, dst, err)
}

// Preserve file permissions
sourceInfo, err := os.Stat(src)
if err != nil {
return fmt.Errorf("failed to stat source file %s: %w", src, err)
}
if err = os.Chmod(dst, sourceInfo.Mode()); err != nil {
return fmt.Errorf("failed to set permissions on %s: %w", dst, err)
}
if err = destFile.Sync(); err != nil {
return fmt.Errorf("failed to sync destination file %s: %w", dst, err)
}
return nil
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
Loading