-
Notifications
You must be signed in to change notification settings - Fork 908
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.
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
atomic_helper.py: ensure presence of parent directories #4938
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@sshedi what is the absolute file path for the missing file? and how are you hitting this? |
Above is the full traceback of the error. |
Thanks for the inputs and clarifications. Please find my replies inline.
[sshedi]: In that case,
[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.
[sshedi]: should not be much, if it's done in the order you mentioned, this directory gets created when
[sshedi]: this is creating the missing
[sshedi]: Adding a parameter to this function might be an overkill, enure_dir will create the dir only if the dir doesn't exist.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Signed-off-by: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @sshedi!
Thanks for taking the fix 🙏 Have a nice time ahead. |
Proposed Commit Message
Additional Context
Test Steps
Checklist
Merge type