Skip to content
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

fix: Fix PV grow_to_fill feature #502

Merged
merged 3 commits into from
Jan 21, 2025
Merged

fix: Fix PV grow_to_fill feature #502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c82c31
fix: Fix PV grow_to_fill feature
vojtechtrefny Jan 15, 2025
4182490
tests: Test LV resize in the PV grow test as well
vojtechtrefny Jan 16, 2025
f085afd
tests: Check return code instead of ouput for does_library_support
vojtechtrefny Jan 17, 2025
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
4 changes: 2 additions & 2 deletions library/blivet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1853,8 +1853,8 @@
pv.format.update_size_info() # set pv to be resizable

if pv.format.resizable:
pv.grow_to_fill = True
ac = ActionResizeFormat(pv, self._device.size)
pv.format.grow_to_fill = True
ac = ActionResizeFormat(pv, pv.size)

Check warning on line 1857 in library/blivet.py

View check run for this annotation

Codecov / codecov/patch

library/blivet.py#L1856-L1857 

Added lines #L1856 - L1857 were not covered by tests
self._blivet.devicetree.actions.add(ac)
else:
log.warning("cannot grow/resize PV '%s', format is not resizable", pv.name)
Expand Down
3 changes: 2 additions & 1 deletion tests/scripts/does_library_support.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,5 @@ def is_supported(var):
print("Usage: python %s <parameter>" % sys.argv[0])
sys.exit(-1)

print(is_supported(sys.argv[1]))
ret = is_supported(sys.argv[1])
sys.exit(0) if ret else sys.exit(1)
3 changes: 2 additions & 1 deletion tests/test-verify-pool-members.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,15 @@
executable: "{{ ansible_python.executable }}"
register: grow_supported
changed_when: false
failed_when: grow_supported.rc not in [0, 1]

- name: Verify that PVs fill the whole devices when they should
include_tasks: verify-pool-member-pvsize.yml
loop: "{{ _storage_test_pool_pvs | default([]) }}"
loop_control:
loop_var: st_pool_pv
when:
- grow_supported.stdout | trim == 'True'
- grow_supported.rc == 0
- storage_test_pool.type == "lvm"
- storage_test_pool.grow_to_fill | bool

Expand Down
17 changes: 2 additions & 15 deletions tests/tests_lvm_pool_pv_grow.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@
become: true
vars:
storage_safe_mode: false
mount_location1: '/opt/test1'
mount_location2: '/opt/test2'
pv_size: '8g'
volume1_size: '2g'
volume2_size: '3g'
tags:
- tests::lvm

Expand Down Expand Up @@ -57,11 +53,7 @@
state: present
volumes:
- name: test1
size: "{{ volume1_size }}"
mount_point: "{{ mount_location1 }}"
- name: test2
size: "{{ volume2_size }}"
mount_point: "{{ mount_location2 }}"
size: 100%

- name: Verify role results
include_tasks: verify-role-results.yml
Expand All @@ -77,11 +69,7 @@
state: present
volumes:
- name: test1
size: "{{ volume1_size }}"
mount_point: "{{ mount_location1 }}"
- name: test2
size: "{{ volume2_size }}"
mount_point: "{{ mount_location2 }}"
size: 100%

- name: Verify role results
include_tasks: verify-role-results.yml
Expand All @@ -96,7 +84,6 @@
state: "absent"
volumes:
- name: test1
- name: test2

- name: Verify role results
include_tasks: verify-role-results.yml
13 changes: 10 additions & 3 deletions tests/verify-pool-member-pvsize.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,28 @@
register: actual_pv_size
changed_when: false

- name: Get PE start for the PV
command: "pvs --noheadings --nosuffix --units b -o PESTART {{ st_pool_pv }}"
register: pv_pe_start
changed_when: false

- name: Convert blkinfo size to bytes
bsize:
size: "{{ storage_test_blkinfo.info[st_pool_pv]['size'] }}"
register: dev_size

# the difference should be at maximum 1 PE + PE start
- name: Verify each PV size
assert:
that: (dev_size.bytes - actual_pv_size.stdout | int) |
abs / actual_pv_size.stdout | int < 0.04
that: (dev_size.bytes - actual_pv_size.stdout | int) <=
(4194304 + pv_pe_start.stdout | int)
Copy link
Contributor

Choose a reason for hiding this comment

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

@guazhangRH Can you try this out so we can see if this is going to cause problems with some of the devices and large disk sizes you use that typically show problems with this sort of check?

Copy link

Choose a reason for hiding this comment

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

test pass with tests_lvm_pool_pv_grow_nvme_generated.yml with 1T nvme test server.

ansible-playbook -i host  -vv tests_lvm_pool_pv_grow_nvme_generated.yml 

[root@smicro-s110p-01 ~]# lsblk
NAME                            MAJ:MIN RM   SIZE RO TYPE MOUNTPOINTS
sda                               8:0    0 447.1G  0 disk 
├─sda1                            8:1    0   600M  0 part /boot/efi
├─sda2                            8:2    0     1G  0 part /boot
└─sda3                            8:3    0 445.5G  0 part 
  ├─rhel_smicro--s110p--01-root 253:0    0    70G  0 lvm  /
  ├─rhel_smicro--s110p--01-swap 253:1    0  31.3G  0 lvm  [SWAP]
  └─rhel_smicro--s110p--01-home 253:2    0 344.3G  0 lvm  /home
nvme4n1                         259:0    0 931.5G  0 disk 
nvme0n1                         259:1    0 931.5G  0 disk 
nvme5n1                         259:5    0 931.5G  0 disk 
nvme3n1                         259:6    0 931.5G  0 disk 
nvme1n1                         259:7    0 931.5G  0 disk 
nvme2n1                         259:8    0 931.5G  0 disk 
[root@smicro-s110p-01 ~]#

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for testing. Using the PE start value should be safe here -- that's the part of the VG metadata size which is affected by hardware specific things things like optimal IO size.

msg: >-
PV resize failure; size difference too big
Unexpected difference between PV size and block device size:
(device size: {{ dev_size.bytes }})
(actual PV size: {{ actual_pv_size.stdout }})

- name: Clean up test variables
set_fact:
actual_pv_size: null
pv_pe_start: null
dev_size: null
Loading