atomic_helper.py: ensure presence of parent directories #4938

sshedi · 2024-02-22T19:05:48Z

Proposed Commit Message

fix(atomic_helper.py): ensure presence of parent directories

Additional Context

Test Steps

Steps to reproduce:

$ cloud-init clean -ls
$ cloud-init -d modules -m init
2024-02-22 18:59:46,522 - handlers.py[DEBUG]: start: modules-init: running modules for init
2024-02-22 18:59:46,522 - util.py[DEBUG]: Reading from /proc/uptime (quiet=False)
....
[('set_hostname', FileNotFoundError(2, 'No such file or directory'))]

Checklist

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

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

aciba90

cloud-init modules is expected to run after cloud-init init --local and cloud-init init and be driven by init-system services.

The config modules corresponding to the init phase, cloud-init modules -m init, are run as part of cloud-init init.

Not sure if we want to support this path in general, if yes, I have some concerns as:

What's the performance impact of this change.
What about the permissions of created parent directories?
Is this the correct place to do this, or would be better to ensure that necessary parent directories are pre-created upper in the call stack? or should we control the creation of the parent dir with a parameter in this function.

That said, why not just run cloud-init init --local, which will create the paths you are missing, before executing cloud-init modules -m init.

holmanb · 2024-02-23T15:57:08Z

@sshedi what is the absolute file path for the missing file? and how are you hitting this?

sshedi · 2024-02-23T18:20:07Z

@sshedi what is the absolute file path for the missing file? and how are you hitting this?

2024-02-23 18:18:43,099 - util.py[WARNING]: Running module set_hostname (<module 'cloudinit.config.cc_set_hostname' from '/usr/lib/python3.10/site-packages/cloudinit/config/cc_set_hostname.py'>) failed
2024-02-23 18:18:43,099 - util.py[DEBUG]: Running module set_hostname (<module 'cloudinit.config.cc_set_hostname' from '/usr/lib/python3.10/site-packages/cloudinit/config/cc_set_hostname.py'>) failed
Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/cloudinit/config/modules.py", line 255, in _run_modules
    ran, _r = cc.run(
  File "/usr/lib/python3.10/site-packages/cloudinit/cloud.py", line 60, in run
    return self._runners.run(name, functor, args, freq, clear_on_fail)
  File "/usr/lib/python3.10/site-packages/cloudinit/helpers.py", line 172, in run
    results = functor(**args)
  File "/usr/lib/python3.10/site-packages/cloudinit/config/cc_set_hostname.py", line 135, in handle
    write_json(prev_fn, {"hostname": hostname, "fqdn": fqdn})
  File "/usr/lib/python3.10/site-packages/cloudinit/atomic_helper.py", line 89, in write_json
    return write_file(
  File "/usr/lib/python3.10/site-packages/cloudinit/atomic_helper.py", line 65, in write_file
    raise e
  File "/usr/lib/python3.10/site-packages/cloudinit/atomic_helper.py", line 46, in write_file
    tf = tempfile.NamedTemporaryFile(
  File "/usr/lib/python3.10/tempfile.py", line 559, in NamedTemporaryFile
    file = _io.open(dir, mode, buffering=buffering,
  File "/usr/lib/python3.10/tempfile.py", line 556, in opener
    fd, name = _mkstemp_inner(dir, prefix, suffix, flags, output_type)
  File "/usr/lib/python3.10/tempfile.py", line 256, in _mkstemp_inner
    fd = _os.open(file, flags, 0o600)
FileNotFoundError: [Errno 2] No such file or directory: '/var/lib/cloud/data/tmplmv88658'

Above is the full traceback of the error.

sshedi · 2024-02-23T18:44:55Z

Thanks for the inputs and clarifications. Please find my replies inline.

cloud-init modules is expected to run after cloud-init init --local and cloud-init init and be driven by init-system services.

[sshedi]: In that case, cloud-init init should not be allowed to run before running cloud-init --local and it should be validated, right? Please correct me if my understanding is wrong.

The config modules corresponding to the init phase, cloud-init modules -m init, are run as part of cloud-init init.

Not sure if we want to support this path in general, if yes, I have some concerns as:

[sshedi]: Even if it's not supported, this should not end in a traceback. There is possibility of a use case where someone might use cloud-init for setting hostname only and nothing else.

1. What's the performance impact of this change.

[sshedi]: should not be much, if it's done in the order you mentioned, this directory gets created when local stage is run but now this is happening here.

2. What about the permissions of created parent directories?

[sshedi]: this is creating the missing data dir under /var/lib/cloud/ and with and without this change, the permissions are 755.

3. Is this the correct place to do this, or would be better to ensure that necessary parent directories are pre-created upper in the call stack? or should we control the creation of the parent dir with a parameter in this function.

[sshedi]: Adding a parameter to this function might be an overkill, enure_dir will create the dir only if the dir doesn't exist.
My question here is, should we use ensure_dirs instead.

That said, why not just run cloud-init init --local, which will create the paths you are missing, before executing cloud-init modules -m init.
[sshedi]: Like I mentioned above, it's a debatable topic. Running --local sounds reasonable but it'll be good if set_hostname without a traceback.

aciba90

Thanks, @sshedi!

cloud-init modules is expected to run after cloud-init init --local and cloud-init init and be driven by init-system services.

[sshedi]: In that case, cloud-init init should not be allowed to run before running cloud-init --local and it should be validated, right? Please correct me if my understanding is wrong.

A normal execution path is:

cloud-init init --local
cloud-init init
cloud-init modules --mode config
cloud-init modules --mode final

in that order, and the init config modules are executed as part of cloud-init init. cloud-init modules --mode init is a command mostly for debugging purposes that we are going to deprecate to avoid confusion.

See boot-stages for more info.

[sshedi]: Even if it's not supported, this should not end in a traceback. There is possibility of a use case where someone might use cloud-init for setting hostname only and nothing else.

I agree, we shouldn't end up with a traceback. Maybe cloud-init single --name set_hostname --frequency always is more appropiate to your use case, if you only want to set the hostname, see single. But, if cloud-init init --local / cloud-init init did not run, some ds meta-data / cloud-config could be not yet fetched.

What about the permissions of created parent directories?

[sshedi]: this is creating the missing data dir under /var/lib/cloud/ and with and without this change, the permissions are 755.

The concern was about other code paths creating folders on other locations with opener permissions, I have double-checked that the main folders that cloud-init interacts are all with 755 root:root, so we are safe with util.ensure_dir.

# tree -pdug /etc/cloud/ /var/run/cloud-init/ /var/lib/cloud/
[drwxr-xr-x root     root    ]  /etc/cloud/
├── [drwxr-xr-x root     root    ]  clean.d
├── [drwxr-xr-x root     root    ]  cloud.cfg.d
└── [drwxr-xr-x root     root    ]  templates
[drwxr-xr-x root     root    ]  /var/run/cloud-init/
└── [drwxr-xr-x root     root    ]  sem
[drwxr-xr-x root     root    ]  /var/lib/cloud/
├── [drwxr-xr-x root     root    ]  data
├── [drwxr-xr-x root     root    ]  handlers
├── [lrwxrwxrwx root     root    ]  instance -> /var/lib/cloud/instances/5b18e235-17ad-41b5-a5b7-44569c20bdf3
├── [drwxr-xr-x root     root    ]  instances
│   └── [drwxr-xr-x root     root    ]  5b18e235-17ad-41b5-a5b7-44569c20bdf3
│       ├── [drwxr-xr-x root     root    ]  handlers
│       ├── [drwxr-xr-x root     root    ]  scripts
│       └── [drwxr-xr-x root     root    ]  sem
├── [drwxr-xr-x root     root    ]  scripts
│   ├── [drwxr-xr-x root     root    ]  per-boot
│   ├── [drwxr-xr-x root     root    ]  per-instance
│   ├── [drwxr-xr-x root     root    ]  per-once
│   └── [drwxr-xr-x root     root    ]  vendor
├── [drwxr-xr-x root     root    ]  seed
└── [drwxr-xr-x root     root    ]  sem

[sshedi]: Adding a parameter to this function might be an overkill, enure_dir will create the dir only if the dir doesn't exist.
My question here is, should we use ensure_dirs instead.

Yeah, I think util.ensure_dir is more appropriate as we know we are going to create only one directory.

cloudinit/atomic_helper.py

Signed-off-by: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>

aciba90

LGTM, thanks @sshedi!

sshedi · 2024-03-12T13:47:47Z

Thanks for taking the fix 🙏 Have a nice time ahead.

sshedi force-pushed the write-file branch from 67a083d to d10a23a Compare February 22, 2024 19:19

aciba90 self-assigned this Feb 23, 2024

aciba90 reviewed Feb 23, 2024

View reviewed changes

aciba90 removed their assignment Feb 23, 2024

sshedi force-pushed the write-file branch 2 times, most recently from c71049b to ef12d0b Compare March 2, 2024 04:46

aciba90 self-assigned this Mar 4, 2024

aciba90 requested changes Mar 7, 2024

View reviewed changes

cloudinit/atomic_helper.py Outdated Show resolved Hide resolved

atomic_helper.py: ensure presence of parent directory

0b72513

Signed-off-by: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>

sshedi force-pushed the write-file branch from ef12d0b to 0b72513 Compare March 7, 2024 18:29

aciba90 approved these changes Mar 11, 2024

View reviewed changes

aciba90 merged commit ccd438e into canonical:main Mar 11, 2024
29 checks passed

sshedi deleted the write-file branch May 1, 2024 06:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

atomic_helper.py: ensure presence of parent directories #4938

atomic_helper.py: ensure presence of parent directories #4938

sshedi commented Feb 22, 2024 •

edited

aciba90 left a comment

holmanb commented Feb 23, 2024

sshedi commented Feb 23, 2024

sshedi commented Feb 23, 2024

aciba90 left a comment •

edited

aciba90 left a comment

sshedi commented Mar 12, 2024

atomic_helper.py: ensure presence of parent directories #4938

atomic_helper.py: ensure presence of parent directories #4938

Conversation

sshedi commented Feb 22, 2024 • edited

Proposed Commit Message

Additional Context

Test Steps

Checklist

Merge type

aciba90 left a comment

Choose a reason for hiding this comment

holmanb commented Feb 23, 2024

sshedi commented Feb 23, 2024

sshedi commented Feb 23, 2024

aciba90 left a comment • edited

Choose a reason for hiding this comment

aciba90 left a comment

Choose a reason for hiding this comment

sshedi commented Mar 12, 2024

sshedi commented Feb 22, 2024 •

edited

aciba90 left a comment •

edited