Fix it so uploads will only be created on writeable filesystems.

[dvalinrh]

Description

Uploads need to be created on a filesystem that is writeable. Issue from bootc

Before/After Comparison

Before: If the filesystem that had the most free space was readonly, the upload creation/operation will fail.
After: Zathras now filters out the read only filesystems for upload creation, so upload is not created there.

Documentation Check

Does this change require any updates to user-facing or internal documentation?
If "yes", were those updates made?

Clerical Stuff

This closes #338
Relates to JIRA: RPOPC-741

Testing.
Ran to system that filesys with the most free space was read only, Zathras did not attempt to use it.
Verified that uploading of results still works properly.

[github-actions]

This relates to RPOPC-741

[github-actions]

PullHero Review

Provider: generativelanguage.googleapis.com Model: gemini-2.0-flash

Okay, I've reviewed the changes to the upload_disk_space Ansible role and the addition of the writeable_filesys script. Here's my analysis:

Code Quality and Maintainability

• Clarity: The change replaces a somewhat complex df, grep, sort, tail, xargs, sed, and cut pipeline with a call to a new script writeable_filesys. This is a good move as it encapsulates the logic and makes the Ansible task easier to read and understand.

• Encapsulation: The writeable_filesys script encapsulates the logic for determining the largest writable filesystem. This promotes reusability and reduces complexity in the Ansible task.

• Argument Handling: The writeable_filesys script uses getopt for argument parsing, which is good practice for Bash scripts.

Potential Bugs or Security Issues

• Security: There's no apparent introduction of security vulnerabilities. The script uses standard tools and doesn't involve any user-supplied input that could lead to command injection.

• Error Handling: The writeable_filesys script lacks robust error handling. Specifically, it doesn't check the return codes of commands like df, mount, and grep. If any of these commands fail, the script might produce incorrect output or fail silently.

• Edge Cases: The writeable_filesys script might not handle all edge cases correctly. For example:

  • Filesystems with unusual characters in their names.
  • Filesystems with very long mount points.
  • Systems where df output differs from the expected format.

• Bootc Mode: The logic in upload_disk_space/tasks/main.yml is a bit convoluted. There are two lineinfile tasks that both write kit_upload_directory. While it probably works, it could be simplified.

Adherence to Project Conventions

• The changes adhere to the project's apparent convention of using Bash scripts for complex logic within Ansible roles.

Documentation Completeness

• The changes are not documented in the docs/ directory. The docs/test_config_files.md and docs/command_line_reference.md should be updated.

Suggestions

1. Add Error Handling to writeable_filesys: Add checks for the return codes of commands within the writeable_filesys script and handle errors gracefully (e.g., by logging an error message and exiting with a non-zero status code).

2. Consider Edge Cases in writeable_filesys: Review the writeable_filesys script for potential edge cases and add appropriate handling. Consider adding tests to verify it handles various scenarios correctly.

3. Simplify logic in upload_disk_space/tasks/main.yml: Refactor the upload_disk_space/tasks/main.yml file so there is only one lineinfile task to write kit_upload_directory.

4. Add Documentation: Update the project documentation to reflect the changes introduced by this pull request.

Vote

Vote: -1

Vote: -1

PullHero

[kdvalin]

Verify documentation changes are not needed

[github-actions]

PullHero Review

Provider: generativelanguage.googleapis.com Model: gemini-2.0-flash

Okay, I will review the proposed changes and provide feedback.

Code Review

Overview

The changes introduce a new script tools_bin/writeable_filesys to determine the filesystem with the most free space and modifies the upload_disk_space Ansible role to use this script instead of the previous df command. This aims to improve the reliability of identifying suitable upload directories, especially in complex filesystem setups.

Analysis

1. Code Quality and Maintainability:

   • The new writeable_filesys script is reasonably well-structured, with a clear usage function and argument parsing using getopt.
   • The script uses temporary files (mktemp) which is a good practice to avoid naming conflicts and ensure cleanup.
   • The Ansible task is simplified by delegating the filesystem determination to the new script.

2. Potential Bugs or Security Issues:

   • The writeable_filesys script relies on parsing the output of mount and df. While this is common, it can be fragile if the output format changes in different environments. It might be worthwhile to consider more robust methods if available (e.g., using statvfs system call via a Python script).
   • The script doesn't explicitly handle errors (e.g., if df or mount fail). It would be beneficial to add error checking and logging for these cases.
   • The script filters out tmpfs and boot partitions, which is generally correct. However, consider whether other special filesystems (e.g., network filesystems) should also be excluded.
   • The script uses grep -ve to filter filesystems, consider using grep -Ev for extended regex to make it more readable and potentially more efficient.
   • The command in the ansible role uses double quotes around the shell command, but inner double quotes are escaped. This could be simplified to single quotes as the ansible variables are already evaluated at this point.

3. Adherence to Project Conventions:

   • The changes seem to adhere to the project's use of shell scripts and Ansible roles.
   • The script includes a shebang (#!/bin/bash) and is made executable (chmod 755), following common conventions.

4. Documentation Completeness:

   • The changes lack corresponding documentation updates. The README files in tools_bin and within the upload_disk_space role should be updated to reflect the script addition and the change in functionality.

Specific Feedback

• Positive:

  • The introduction of writeable_filesys script improves modularity and makes the Ansible task easier to understand.
  • Using mktemp for temporary files is a good security practice.

• Concerns:

  • The script's reliance on parsing mount and df output is fragile. Consider alternative approaches if robustness is critical.
  • Error handling in the script is missing. Add checks for command failures and appropriate logging.

• Suggestions:

  • Add comments to the writeable_filesys script explaining the purpose of each section and the logic behind the filtering.
  • Update the README files to document the new script and the modified Ansible role.
  • Consider using a more robust method for finding the largest writeable filesystem.
  • Consider using single quotes around the ansible command to avoid the need to escape inner double quotes.

Example of improved writeable_filesys script

#!/bin/bash

# Script to find the largest writable filesystem (excluding tmpfs and boot)

most_free_space=0

usage() {
    echo "Usage $1:"
    echo "No option, shows the following for all writable filesystems (not tmpfs):"
    echo "  <free_space> <mount point> <device>"
    echo "  --most_free_space:  provide the mount point of the filesystem with the most free space"
    echo "  -h --usage: this usage message"
    exit 0
}

# Function to handle errors and exit
error_exit() {
    echo "Error: $1" >&2  # Redirect to stderr
    exit 1
}

# Parse command line arguments
while getopts "h" opt; do
    case $opt in
        h)
            usage $0
            ;;
        \?)
            echo "Invalid option: -$OPTARG" >&2
            usage

**Vote**: 0

**[PullHero](https://github.com/pullhero/)**