linux-system-roles / storage

Ansible role for linux storage management
https://linux-system-roles.github.io/storage/
MIT License
101 stars 59 forks source link

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

Closed richm closed 1 year ago

richm commented 1 year ago

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 commented 1 year ago

Codecov Report

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

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

:mega: 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 ```diff @@ 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](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more. Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

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

nhosoi commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Thank you for the detailed explanations, @richm!