Skip to content

Find k3s device from volume ID instead of trusting openstack #941

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Find k3s device from volume ID instead of trusting openstack #941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion roles/infra/templates/outputs.tf.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ output "cluster_nodes" {
{% endfor %}
]
facts = {
k3s_storage_device = openstack_compute_volume_attach_v2.data_volume_attach.device
k3s_storage_volume_id = openstack_compute_volume_attach_v2.data_volume_attach.volume_id

# Set the gateway IP as a fact that can be consumed
{% if not infra_provisioning_network_id and infra_use_floatingip %}
Expand Down
1 change: 0 additions & 1 deletion roles/k3s/defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ k3s_binary_checksum_url: "{{ k3s_repo }}/releases/download/{{ k3s_version }}/sha
k3s_binary_checksum: "sha256:{{ lookup('url', k3s_binary_checksum_url, wantlist=True) | first | split | first }}"

# Settings for an additional block device that will hold the k3s state, if present
k3s_storage_device: /dev/sdb
k3s_storage_fstype: xfs

# Indicates if the Traefik ingress controller should be enabled
Expand Down
46 changes: 37 additions & 9 deletions roles/k3s/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,41 @@
---
- name: Check if k3s storage device is attached
ansible.builtin.stat:
path: "{{ k3s_storage_device }}"
register: k3s_storage_device_stat

- name: Fail if k3s storage device is missing
ansible.builtin.fail:
msg: "K3s storage device not found at {{ k3s_storage_device }}"
when: not k3s_storage_device_stat.stat.exists
- name: Find the block device for the k3s storage volume
block:
- name: Stat candidate device paths
ansible.builtin.stat:
path: "/dev/disk/by-id/{{ item }}"
loop:
# KVM
- "{{ 'virtio-{}'.format(k3s_storage_volume_id[:20]) }}"
# KVM #852
- "{{ 'virtio-{}'.format(k3s_storage_volume_id) }}"
# KVM virtio-scsi
- "{{ 'scsi-0QEMU_QEMU_HARDDISK_{}'.format(k3s_storage_volume_id[:20]) }}"
# KVM virtio-scsi #852
- "{{ 'scsi-0QEMU_QEMU_HARDDISK_{}'.format(k3s_storage_volume_id) }}"
# ESXi
- "{{ 'wwn-0x{}'.format(k3s_storage_volume_id | replace('-', '')) }}"
register: candidate_device_paths_stat

- name: Set volume block device name
ansible.builtin.set_fact:
k3s_storage_device: >-
/dev/{{
candidate_device_paths_stat.results |
selectattr("stat.exists") |
map(attribute = "stat.lnk_source") |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about this, I think we want the actual /dev/disk/by-id path in /etc/fstab (which happens in the mount filesytem task below), otherwise I think we could end up in a situation like this:

  1. Attach a volume, it is /dev/disk/by-id/SOME-UUID => /dev/vdb
  2. Ansible here adds /dev/vdb to /etc/fstab
  3. Reboot the machine, libvirt shuffles the cards and now /dev/disk/by-id/SOME-UUID => /dev/vdc
  4. Machine never boots because its missing a device it expects to see in /etc/fstab.
  5. (Machine miraculously boots, but then we run the ansible again and we have /dev/vdb and /dev/vdc in /etc/fstab.

I think this task should look more like:

    - name: Set volume block device name
      ansible.builtin.set_fact:
        k3s_storage_device: >-
          {{
            candidate_device_paths_stat.results |
              selectattr("stat.exists") |
              map(attribute = "stat.path") |
              first |
              default("") |
             }}

then k3s_storage_device will end up with the /dev/disk/by-id path, and we should be fully immune from libvirt shuffling.

If you go this way, then you can set when: k3s_storage_device == "" in the next task, which is somewhat cleaner I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self - if we move to this method, we should do it in a release where we change the base image, because that way we don't have to worry about the oldstyle (/dev/vdb) and newstyle (/dev/disk/by-id/) coexisting in /etc/fstab, because we'll be rolling the image and will have a fresh fstab.

first |
default("") |
trim("/") |
split("/") |
last
}}
- name: Fail if block device was not found
ansible.builtin.fail:
msg: "Could not locate block device for volume {{ k3s_storage_volume_id }}."
when: k3s_storage_device == "/dev/"


- name: Ensure filesystem exists on storage device
community.general.filesystem:
Expand Down
Loading