-
Notifications
You must be signed in to change notification settings - Fork 69
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
Support bootc #830
base: bootc
Are you sure you want to change the base?
Support bootc #830
Conversation
Skipping CI for Draft Pull Request. |
roles/edpm_kernel/defaults/main.yml
Outdated
@@ -36,3 +36,4 @@ edpm_nova_compute_config_dir: /var/lib/config-data/ansible-generated/nova_libvir | |||
|
|||
# KSM control | |||
edpm_kernel_enable_ksm: false | |||
edpm_use_bootc: false |
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.
We probably need a better way to implement this globally. But at least for testing purposes, this is what I've used to get something that deploys.
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.
This is why I added the edpm_bootc role in #813 so that we had a way to do it consistently across anywhere that needs it.
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.
Also, I created a new bootc branch for edpm-ansible: https://github.com/openstack-k8s-operators/edpm-ansible/tree/bootc
Can you propose this PR to the bootc branch instead?
I'll be reverting #813 from main until we are ready to merge all bootc support into main.
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.
Branch thing is done. But that role would need to be called from each and every playbook to detect and set the bootc
variable right? I guess we can just add it as a ansibleVar and avoid calling the role each and every time we start a new service.
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.
So, we can use a custom fact. For example:
cat /etc/ansible/facts.d/bootc.fact
#!/usr/bin/env bash
is_bootc() {
BOOTC_STATUS=$(sudo bootc status --json | jq .status.type)
if [[ "$BOOTC_STATUS" == \"bootcHost\" ]]; then
BOOTC_SYSTEM="true"
else
BOOTC_SYSTEM="false"
fi
}
is_bootc
echo ${BOOTC_SYSTEM}
This is good from the perspective of not needing the user to manually define that they are using a bootc system, plus it works for our non bootc systems:
[m3@osp-df-3 bootc]$ ansible -i inv.yaml all -m setup -a "filter=ansible_local"
edpm-compute-1 | SUCCESS => {
"ansible_facts": {
"ansible_local": {
"bootc": true
},
"discovered_interpreter_python": "/usr/bin/python3"
},
"changed": false
}
So you could combine them in the same NodeSet if you wanted to. But, the down side of this approach is that we need to gather facts from each service. We have thus far tried to limit the amount of fact gathering required, so this approach my not be what we want to do without some more granular control of which facts are being gathered in each service. At the moment, we just define a variable for gather_facts
. If that variable is true, then we gather all facts. Obviously, that becomes necessary if we want to allow individual task executions that require facts, but when we want to just gather local facts, then gathering all of them introduces non-trivial time to our executions of each service.
Offering it as a potential solution that we can debate. The alternative is that we require either bootc or non-bootc nodes in each NodeSet.
- name: Push script | ||
ansible.builtin.copy: | ||
dest: /usr/local/sbin/containers-tmpwatch | ||
dest: /var/lib/openstack/cron/containers-tmpwatch |
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.
/usr
is immutable with bootc deployments. So I've proposed doing this in two different ways. 1, we bake the scripts into the container file:
https://github.com/openstack-k8s-operators/install_yamls/pull/950/files#diff-f8fb9af5355b45b9ca8936bf0d721c6f0e37e13b637f5598e2be19995dea23e7R45-R46
And 2. Which is this method of writing to /var/lib/openstack
. I personally prefer doing it this way if we can agree on a common place for any scripts that we want to use. That saves us baking things into images and then trying to keep them in sync. Better imo to have them in edpm-ansible for now.
ansible.builtin.include_role: | ||
name: osp.edpm.edpm_container_standalone | ||
vars: | ||
edpm_container_standalone_service: ovn_controller | ||
edpm_container_standalone_container_defs: | ||
ovn_controller: "{{ lookup('template', 'ovn_controller.yaml.j2') | from_yaml }}" | ||
edpm_container_standalone_kolla_config_files: | ||
ovn_controller: "{{ lookup('template', 'kolla_ovn_controller.yaml.j2') | from_yaml }}" |
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.
All of this needs to stay in order to support both deployment methodologies. It can just be conditional like:
https://github.com/openstack-k8s-operators/edpm-ansible/pull/830/files#diff-34e3323585e197e806d463771e3b5132716048c41818b1318fecb2c0d8e36cd6R45
dd3d198
to
cde9667
Compare
ffd86e6
to
54f7101
Compare
roles/edpm_ovn/tasks/run.yml
Outdated
{{ | ||
ovn_controller_pod_spec | combine({ | ||
'spec': { | ||
'containers': ovn_controller_pod_spec.spec.containers | zip_longest([], [{'image': edpm_ovn_controller_agent_image}]) | map('combine') | list, |
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.
I believe that if we were to customize the image like this then by definition the container is no longer "logically bound". Instead, it would be considered a "floating" container per https://containers.github.io/bootc/logically-bound-images.html#comparison-with-default-podman-systemd-units
and also:
There is no mechanism to inject arbitrary arguments to the podman pull (or equivalent) invocation used by bootc.
which seems to apply additional mounts or other options passed to podman pull are not possible.
The dynamically-injected ConfigMaps[1][2] may provide some customization, but that is still not likely for the app container image itself, b/c once that is changed to some other image, then that no longer fits into how logically bound images should be managed with the lifecycle of the base bootc image itself.
Point being...if we choose to allow the ability to podman run any arbitrary image at runtime, then these really aren't logically bound images at all, but are considered "floating".
The question becomes, should we adopt logically bound images, and require our end users to be building new bootc images before deploying EDPM nodes, depending on if they need to customize any of the container images? We could ship a bootc image that had all the images logically bound, but if a user wanted to run a different one (from a partner, etc) they they would need to rebuild that image.
I do like the quadlet/systemd design, and I think we can still adopt that either way.
[1] https://containers.github.io/bootc/building/guidance.html?highlight=configmap#configuration
[2] containers/bootc#22
a0569c4
to
b590229
Compare
I created a new bootc branch for edpm-ansible: https://github.com/openstack-k8s-operators/edpm-ansible/tree/bootc Can you propose this PR to the bootc branch instead? I reverted #813 from main in #844 I think that was the only other bootc related PR that has merged. |
1afffaf
to
51a8b34
Compare
Signed-off-by: Brendan Shephard <[email protected]> dnf yum-utils Signed-off-by: Brendan Shephard <[email protected]> nvme-package Signed-off-by: Brendan Shephard <[email protected]>
This change writes systemd files to etc instead of /usr/share along with adding support for Python libraries baked into the bootc image. Signed-off-by: Brendan Shephard <[email protected]>
This change moves the script we're using for the logs cronjob into the /var/lib/openstack/cron directory. This facilitates the bootc immutable filesystem where we can't write to /usr, while also consolidating scripts relevant to our deployment in a common place. Signed-off-by: Brendan Shephard <[email protected]>
Signed-off-by: Brendan Shephard <[email protected]>
Signed-off-by: Brendan Shephard <[email protected]>
51a8b34
to
c04ac6a
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bshephar 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 |
Signed-off-by: Brendan Shephard <[email protected]>
ebb8305
to
64316b3
Compare
Signed-off-by: Brendan Shephard <[email protected]>
Signed-off-by: Brendan Shephard <[email protected]>
64316b3
to
216ec24
Compare
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 entire handling of kernel args needs to be reconsidered for bootc:
https://containers.github.io/bootc/building/kernel-arguments.html
Signed-off-by: Brendan Shephard <[email protected]>
that kargs.d mechanism looks interesting. I reckon if we're doing anything for the demo, ansible should write to a /usr/lib/bootc/kargs.d/00-edpm.toml and reboot. Also we need to ensure the generated qcow2 has console= arguments removed |
Signed-off-by: Brendan Shephard <[email protected]>
5387f86
to
56bf7e4
Compare
Signed-off-by: Brendan Shephard <[email protected]>
This PR adds a number of changes to roles in order to facilitate the use of image mode RHEL.