Skip to content

Conversation

seunghun1ee
Copy link
Member

@seunghun1ee seunghun1ee commented Jan 19, 2024

Running pvresize before lvextend ensures that there is enough space for extension.

There are some cases where lvextend fails because initial usable space of a physical volume is not enough.
This leads the automated setup failure at package or python module installation stage.
As this happens earlier than kayobe setup, kayobe playbook run etc/kayobe/ansible/growroot.yml can't be used.

- name: Grow LVM PV
command: "pvresize {{ disk }}"
vars:
pv: "{{ pvs.stdout | from_json }}"
disk: "{{ pv.report[0].pv[0].pv_name }}"
become: true
when: lvm_check.rc == 0 or growroot_ignore_lvm_check

Therefore I propose adding bash equivalent of the Ansible task above before lvextend.

The command gets the name of the physical volume by filtering the output of sudo pvs --noheadings.
head -n 1 is used to take first line of output only then awk '{print $1}' is used to take first element of it, which is essentially the way the growroot.yml does to get PV name.

Running pvresize before lvextend ensures that there is enough spaces for extension.
@seunghun1ee seunghun1ee requested a review from a team as a code owner January 19, 2024 14:20
@Alex-Welsh
Copy link
Member

Growroot is no longer required if pvresize is being run early on, so you can delete this line

Alex-Welsh
Alex-Welsh previously approved these changes Jan 19, 2024
Copy link
Member

@Alex-Welsh Alex-Welsh left a comment

Choose a reason for hiding this comment

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

LGTM but two notes:

  1. Your change could be one line:
    sudo pvresize $(sudo pvs --noheadings | head -n 1 | awk '{print $1}')

  2. It's a bit of a shame we don't use growroot anymore, but I think it's more important to keep this script clean and consistent than to try and have some complex conditional that does both a pvresize and growroot.

@seunghun1ee
Copy link
Member Author

seunghun1ee commented Jan 22, 2024

  1. Your change could be one line:
    sudo pvresize $(sudo pvs --noheadings | head -n 1 | awk '{print $1}')

Yeah I agree. Looks like setting a variable is not needed here.

Alex-Welsh
Alex-Welsh previously approved these changes Jan 22, 2024
@Alex-Welsh Alex-Welsh enabled auto-merge (squash) January 22, 2024 11:50
@markgoddard markgoddard disabled auto-merge January 26, 2024 11:40
@markgoddard markgoddard merged commit 00ff42d into stackhpc/yoga Jan 26, 2024
@markgoddard markgoddard deleted the pvresize-before-kayobe branch January 26, 2024 11:41
@seunghun1ee
Copy link
Member Author

@markgoddard What do you think of also making PR with this change for stackhpc/zed and stackhpc/2023.1?

@markgoddard
Copy link
Contributor

@markgoddard What do you think of also making PR with this change for stackhpc/zed and stackhpc/2023.1?

We merge changes forwards in this repo, so they will get fixed automatically. I can propose some merges.

This was referenced Jan 26, 2024
@seunghun1ee
Copy link
Member Author

@markgoddard What do you think of also making PR with this change for stackhpc/zed and stackhpc/2023.1?

We merge changes forwards in this repo, so they will get fixed automatically. I can propose some merges.

Thanks Mark

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants