-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add machine init --run-playbook
#25043
base: main
Are you sure you want to change the base?
Conversation
cc @baude |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, jakecorrenti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Cross linking containers/podman-machine-os#68 (comment) Here this seems quite limiting in what it would do. AFAICT ansible playbooks can consist of many different files that are included, the directory structure can be complex, see https://docs.ansible.com/ansible/2.8/user_guide/playbooks_best_practices.html#directory-layout As such I really don't see how this would help many users with complex things that have more than a single file. Would it not be much better to require the user to have ansible installed on the host and run it from there? |
no, this is quite useful. it can be a single file and in this case, it is perfectly OK to be ... this, in conjunction with something like a |
Sure it can be a single file. But this restricts the purpose AFAICT and I bet we just get more people asking to allow multiple files and whatever else ansible can do. Thus I am just not sure if it makes sense to go with a design that we cannot easily extend later. Also isn't this just super racy? We simply start a playbook but never wait for it to be done which means whatever config should be applied may not be ready thus causing hard to diagnose issues. Also unless someone knows to ssh into the machine and knows the unit name they will not be able to get the output in case of failures. And then this currently says it runs on first boot but the unit would be run on every boot unless I am missing something. |
pkg/machine/ignition/ignition.go
Outdated
// read the config file to a string | ||
s, err := os.ReadFile(input.Name()) | ||
if err != nil { | ||
return err | ||
} |
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.
The caller opens the file then passes the file and then you disregard that and open it again.
Either the caller passes the name directly or use io.ReadAll()
so we do not open the file twice
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 for all the comments so far. Should be good now
d1e4b61
to
418cd5a
Compare
What am I missing here? I did |
418cd5a
to
fe5ddda
Compare
I think this won't work on Windows when using WSL until we add ansible in the image used in that case. More generally, with this PR, we are adding an implicit dependency between podman and the machine image: if ansible is not pre-installed in the machine image (i.e., it's a custom one), Also, playbooks with a local path won't work (as mentioned in the man page). But this can be worked around if paths in the playbook are converted as it's currently done for volumes and additional build contexts. |
@jakecorrenti please file a PR to add ansible to the machine image. |
I made a PR to fix that: containers/podman-machine-wsl-os#11. I don't have a way to test it, however.
Would it be enough to add in the manpage that custom images are required to have Ansible added? Or would you rather have something completely separate.
|
I think something in the man page like that says this requires ansible be present in the image which is default is a fair thing to do. But more importantly, lets add a statement to --image that basically says we only support the images we provide. wdyt ? |
I think that sounds like a good idea |
add ansible to the machine image so users can execute a playbook once the machine is booted. Users have been asking for a way to automaticaly configure a machine with various options, files (like certificates), and configuration tweaks. Ansible is an excellent tool to do this in the machine. See also containers/podman#25043 Signed-off-by: Brent Baude <[email protected]>
0faa4f6
to
ade327b
Compare
Cockpit tests failed for commit ade327b. @martinpitt, @jelly, @mvollmer please check. |
ade327b
to
f7393b3
Compare
Allow the user to provide an Ansible playbook file on init which will then be run on boot. Signed-off-by: Jake Correnti <[email protected]> Signed-off-by: Brent Baude <[email protected]>
f7393b3
to
4d05bf7
Compare
Signed-off-by: Jake Correnti <[email protected]>
Allow the user to provide an Ansible playbook file on init which will then be run on first boot.
This is dependent on containers/podman-machine-os#68 and containers/podman-machine-wsl-os#11 getting merged.
Does this PR introduce a user-facing change?