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.

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

restrict swap size to less than 128GB on EL7 #331

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

richm
Copy link
Contributor

@richm richm commented Jan 31, 2023

On EL7 the swap size must be less than 128GB, so change the tests
to restrict the size. This required introducing a max_size parameter
to find_unused_disk.

I also took this opportunity to do some ansible-lint 6.x cleanup
for the code I touched. There is still a huge amount of tests/
code that is not ansible-lint 6.x clean.

@codecov-commenter
Copy link

Codecov Report

Base: 16.54% // Head: 16.54% // No change to project coverage 👍

Coverage data is based on head (806bd81) compared to base (85353e4).
Patch has no changes to coverable lines.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #331   +/-   ##
=======================================
  Coverage   16.54%   16.54%           
=======================================
  Files           2        2           
  Lines         284      284           
  Branches       71       71           
=======================================
  Hits           47       47           
  Misses        237      237           
Flag Coverage Δ
sanity 16.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nhosoi
Copy link
Contributor

nhosoi commented Feb 1, 2023

lgtm

Out of curiosity... Instead of introducing max_size and passing it by setting 127g to find_unused_disk.py, we could quietly assign 127g to an internal variable for rhel-7, I'd think. Most likely, I haven't understood the advantage of having max_size... :p Ok, we have max_size. Then, may we want to set the default value of max_size to 127g for rhel-7 and 0 for the other versions?

What makes me wonder most is 127g is set in the test scripts in this pr. Is that the knowledge that customers should have for rhel-7? Maybe, it is...

@richm
Copy link
Contributor Author

richm commented Feb 1, 2023

lgtm

Out of curiosity... Instead of introducing max_size and passing it by setting 127g to find_unused_disk.py, we could quietly assign 127g to an internal variable for rhel-7, I'd think.

The problem is that tests/provision.fmf lists 1TB disk sizes for the scsi (and nvme) disks, because we need those large sizes for some other tests. So the tests that get an unused disk for swap need to somehow tell find_unused_disk.py to return an unused disk of less than a certain size:

standard-inventory-qcow2:
  qemu:
    m: 2048
    drive:
      - size: 10737418240
      - size: 10737418240
      - size: 10737418240
      - size: 1099511627800
        interface: scsi
      - size: 1099511627800
        interface: scsi
      - size: 10737418240
        interface: scsi
      - size: 1099511627800
        interface: nvme
      - size: 10737418240
        interface: nvme
      - size: 10737418240
        interface: nvme

I'm not sure how to tell find_unused_disk.py to return an item from this list which has a scsi interface which is less than 128GB unless I introduce a max_size parameter.

Most likely, I haven't understood the advantage of having max_size... :p Ok, we have max_size. Then, may we want to set the default value of max_size to 127g for rhel-7 and 0 for the other versions?

No, because find_unused_disk.py is used for all of the tests, not just swap related tests. For non-swap uses, there is no 128GB size restriction - only for the swap case.

What makes me wonder most is 127g is set in the test scripts in this pr. Is that the knowledge that customers should have for rhel-7? Maybe, it is...

They should know that they cannot create a swap partition of size greater than 127GB.

@nhosoi
Copy link
Contributor

nhosoi commented Feb 1, 2023

Thank you for the detailed explanations, @richm!

Copy link
Contributor

@nhosoi nhosoi left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@vojtechtrefny vojtechtrefny left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you.

On EL7 the swap size must be less than 128GB, so change the tests
to restrict the size.  This required introducing a `max_size` parameter
to find_unused_disk.

I also took this opportunity to do some ansible-lint 6.x cleanup
for the code I touched.  There is still a huge amount of tests/
code that is not ansible-lint 6.x clean.
@richm richm merged commit 15fc22f into linux-system-roles:main Feb 1, 2023
@richm richm deleted the smaller-disk-for-el7 branch February 1, 2023 19:35
richm added a commit to richm/linux-system-roles-storage that referenced this pull request Feb 1, 2023
[1.9.6] - 2023-02-01
--------------------

### New Features

- none

### Bug Fixes

- none

### Other Changes

- fix shellcheck issues (linux-system-roles#327)
- Fix issues found by CodeQL (linux-system-roles#329)
- restrict swap size to less than 128GB on EL7 (linux-system-roles#331)

Signed-off-by: Rich Megginson <rmeggins@redhat.com>
richm added a commit that referenced this pull request Feb 1, 2023
[1.9.6] - 2023-02-01
--------------------

### New Features

- none

### Bug Fixes

- none

### Other Changes

- fix shellcheck issues (#327)
- Fix issues found by CodeQL (#329)
- restrict swap size to less than 128GB on EL7 (#331)

Signed-off-by: Rich Megginson <rmeggins@redhat.com>
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.

4 participants