fix: cloud-init clean --logs should not remove non-files

[blackboxsw]

Proposed Commit Message

Add test coverage for the fix that prevents cloud-init clean --logs
from attempting to remove non-file paths (e.g., character devices,
directories) that may appear in the log file configuration.

The test verifies that when /dev/null is configured as a log path,
it is safely skipped while actual log files are still removed.

Additional Context

Test Steps

 tox -e py3 -- tests/unittests/cmd/test_clean.py::TestClean::test_remove_artifacts_skips_non_file_logs

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[holmanb]

Thanks for this @blackboxsw. I left a couple of comments. Also, can you share the error you saw?

[holmanb]

This is used in various places. This would have previously deleted a directory or symlink, but now will not. Is that what we want in every case that it is used?

[blackboxsw]

Good point, while it wouldn't have removed a directory, it previously would have removed a symlink.

  File ".../src/cloud-init/cloudinit/util.py", line 2082, in del_file
    os.unlink(path)
IsADirectoryError: [Errno 21] Is a directory: 'bogus'

Aligned with your comment though, I've made the following changes:

1. I'll add special lstat checks to allow symlink removal, file and named pipes, but avoid device files
2. Let's limit this changeset to user-configurable paths (via changes only in cmd/clean.py instead of util.del_file as those supplemental guard-rails are not necessary on every del_file call, but on paths we think are user-configurable and more prone to error inputs.

[holmanb]

This will change behavior for anyone that is using a named pipe as a destination for their logs - the pipe will no longer be cleaned up.

[blackboxsw]

I've pulled this out of util.del_file so we only impact cloud-init clean behavior for potentially customized log-config.

[blackboxsw]

Thanks for this @blackboxsw. I left a couple of comments. Also, can you share the error you saw?

@holmanb here is the error which could be caused be custom /etc/cloud/cloud.cfg.d/05_logging.cfg changing log files to character devices or symlinks.

root@dd:~# cloud-init clean --logs
Traceback (most recent call last):
  File "/usr/bin/cloud-init", line 9, in <module>
    sys.exit(main.main())
             ~~~~~~~~~^^
  File "/usr/lib/python3/dist-packages/cloudinit/cmd/main.py", line 1308, in main
    return sub_main(args, parser)
  File "/usr/lib/python3/dist-packages/cloudinit/cmd/main.py", line 1438, in sub_main
    retval = functor(name, args)
  File "/usr/lib/python3/dist-packages/cloudinit/cmd/clean.py", line 189, in handle_clean_args
    exit_code = remove_artifacts(
        init, args.remove_logs, args.remove_seed, args.remove_config
    )
  File "/usr/lib/python3/dist-packages/cloudinit/cmd/clean.py", line 129, in remove_artifacts
    del_file(log_file)
    ~~~~~~~~^^^^^^^^^^
  File "/usr/lib/python3/dist-packages/cloudinit/util.py", line 2072, in del_file
    os.unlink(path)
    ~~~~~~~~~^^^^^^
OSError: [Errno 16] Device or resource busy: '/dev/console'

[holmanb]

Thanks for this @blackboxsw. I left some feedback inline. Also, the proposed commit message is confusing. Block devices, character devices, directories, symlinks, and pipes are all files.

More succinctly, the goal here is to avoid tracebacks and aborted operations when a reasonable configuration is provided. Perhaps we can just say something like that in the commit message.

[holmanb]

This is redundant: del_file is already safe against trying to remove non-existent files.

[holmanb]

This seems unlikely. While device files such as /dev/null are a widely known kernel-provided option for ignoring output, a symlink is far less common and would almost certainly have been inserted at runtime or image build time.

The same is true for other kinds of files: named pipes, for example, which don't get this special treatment in this code. The choice to ignore symlinks but not named pipes seems arbitrary - why should a user expect them to be treated differently? It doesn't seem likely and treating some cases differently than others doesn't seem necessary.

In short, this seems "magical", and doesn't fit the real need addressed in this PR: prevent an unwanted traceback seen in a reasonable logging configuration.

[holmanb]

Is writing to the console really desirable here? It seems like unnecessary noise.