[BUG] lvm.lv_present doesn't like MB any more, documentation unclear

[dkacar-oradian]

Description In older releases specifying size for lvm.lv_present as 500MB worked fine. 3002.2 doesn't like that any more. Documentation is terribly unclear and I really don't understand if MB is supposed to work or not.

Setup Test file:

testme1:
  lvm.lv_present:
    - name: lv1
    - vgname: vgos
    - size: 500MB

testme2:
  lvm.lv_present:
    - name: lv2
    - vgname: vgos
    - size: 500M

You will need to have a volume group with sufficient space to create these two LVs (here called vgos). The only difference above is that the size for lv1 is 500MB and size for lv2 is 500M.

Steps to Reproduce the behavior When you run this the first time everything is fine:

First run

``` # salt stest-app-01.xdc state.apply test stest-app-01.xdc: ---------- ID: testme1 Function: lvm.lv_present Name: lv1 Result: True Comment: Created Logical Volume lv1 Started: 15:56:05.051069 Duration: 345.031 ms Changes: ---------- created: ---------- /dev/vgos/lv1: ---------- Allocated Logical Extents: -1 Allocation Policy: 0 Current Logical Extents Associated: 125 Internal Logical Volume Number: -1 Logical Volume Access: 3 Logical Volume Name: /dev/vgos/lv1 Logical Volume Size: 1024000 Logical Volume Status: 1 Major Device Number: 253 Minor Device Number: 6 Open Logical Volumes: 0 Read Ahead Sectors: -1 Volume Group Name: vgos Output from lvcreate: Logical volume "lv1" created. ---------- ID: testme2 Function: lvm.lv_present Name: lv2 Result: True Comment: Created Logical Volume lv2 Started: 15:56:05.396988 Duration: 247.401 ms Changes: ---------- created: ---------- /dev/vgos/lv2: ---------- Allocated Logical Extents: -1 Allocation Policy: 0 Current Logical Extents Associated: 125 Internal Logical Volume Number: -1 Logical Volume Access: 3 Logical Volume Name: /dev/vgos/lv2 Logical Volume Size: 1024000 Logical Volume Status: 1 Major Device Number: 253 Minor Device Number: 7 Open Logical Volumes: 0 Read Ahead Sectors: -1 Volume Group Name: vgos Output from lvcreate: Logical volume "lv2" created. Summary for stest-app-01.xdc ------------ Succeeded: 2 (changed=2) Failed: 0 ------------ Total states run: 2 Total run time: 592.432 ms ```

But when you run it for the second time it's not fine.

# salt stest-app-01.xdc state.apply test
stest-app-01.xdc:
----------
          ID: testme1
    Function: lvm.lv_present
        Name: lv1
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3.6/site-packages/salt/state.py", line 2154, in call
                  *cdata["args"], **cdata["kwargs"]
                File "/usr/lib/python3.6/site-packages/salt/loader.py", line 2106, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3.6/site-packages/salt/states/lvm.py", line 329, in lv_present
                  size_mb = _convert_to_mb(size)
                File "/usr/lib/python3.6/site-packages/salt/states/lvm.py", line 46, in _convert_to_mb
                  size = int(size)
              ValueError: invalid literal for int() with base 10: '500M'
     Started: 15:57:21.944871
    Duration: 53.015 ms
     Changes:   
----------
          ID: testme2
    Function: lvm.lv_present
        Name: lv2
      Result: True
     Comment: Logical Volume lv2 already present
     Started: 15:57:21.998722
    Duration: 37.764 ms
     Changes:   

Summary for stest-app-01.xdc
------------
Succeeded: 1
Failed:    1
------------
Total states run:     2
Total run time:  90.779 ms
ERROR: Minions returned with non-zero exit code

Expected behavior There should have been no exceptions in the second run.

The documentation for the size parameter says only "The size of the Logical Volume". Which really doesn't specify anything. But since MB worked fine on previous releases all the time and it works in 3002.2 in the first run I would take the liberty of assuming that this is a bug.

Screenshots If applicable, add screenshots to help explain your problem.

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) ``` # salt-minion --versions-report [ERR=64] Salt Version: Salt: 3002.2 Dependency Versions: cffi: Not Installed cherrypy: Not Installed dateutil: Not Installed docker-py: Not Installed gitdb: Not Installed gitpython: Not Installed Jinja2: 2.11.1 libgit2: Not Installed M2Crypto: 0.35.2 Mako: Not Installed msgpack: 0.6.2 msgpack-pure: Not Installed mysql-python: Not Installed pycparser: Not Installed pycrypto: Not Installed pycryptodome: Not Installed pygit2: Not Installed Python: 3.6.8 (default, Apr 2 2020, 13:34:55) python-gnupg: Not Installed PyYAML: 3.13 PyZMQ: 17.0.0 smmap: Not Installed timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.1.4 System Versions: dist: centos 7 Core locale: ISO-8859-2 machine: x86_64 release: 3.10.0-1127.18.2.el7.x86_64 system: Linux version: CentOS Linux 7 Core ```

Additional context Add any other context about the problem here.

[piterpunk]

The valid units are s|S (sectors), m|M (megabytes), g|G (gigabytes), t|T (terabytes) and p|P (petabytes):

    if unit == "s":
        target_size = size / 2048
    elif unit == "m":
        target_size = size
    elif unit == "g":
        target_size = size * 1024
    elif unit == "t":
        target_size = size * 1024 * 1024
    elif unit == "p":
        target_size = size * 1024 * 1024 * 1024

With the absence of b|B (bytes), k|K (kilobytes) and e|e (exabytes) these are almost the same units that the LVM commands should accept (from the man page):

b|B is bytes, s|S is sectors of 512 bytes, k|K is kilobytes, m|M is megabytes, g|G is gigabytes, t|T is terabytes, p|P is petabytes, e|E is exabytes

The 500MB works by chance, because the LVM commands stops the parsing at the first non numeric character:

root@marvin:/srv/salt/ntp# lvcreate -L32M_The_LVM_DONT_CARE_ABOUT_ANY_LETTER_AFTER_THE_FIRST_ONE -n teste1 marvinvg00
  Logical volume "teste1" created.
root@marvin:/srv/salt/ntp# lvs marvinvg00/teste1
  LV     VG         Attr       LSize  Pool Origin Data%  Meta%  Move Log Cpy%Sync Convert
  teste1 marvinvg00 -wi-a----- 32.00m     

While the LVM parser stops at first non-numeric character and ignore the remaining ones, our parser goes from opposite side, it strips the last non numeric character as the unit and assumes the remaining is a valid numeral.

Don't know if this is the case to update documentation, change the state parser to have the same behaviour of LVM parser, force the fail at first state apply or a mix of these options.

IMHO a documentation update should be fine. But as existing states are being broke, maybe the better is to use the same parsing strategy of LVM commands, which is ugly at least.

[dkacar-oradian]

I agree that the documentation update should be enough. Although I personally dislike not using the unit (bytes in this case).

In a similar situation I implemented this:

1. Change the string to upper case
2. If there's 'B' at the end strip it
3. Give the result to the parser which only recognizes size letters (numfmt command, if anyone's interested)

That's an option, too. I'm not asking for it, though (and there is a problem with sectors). I'd really like the documentation to be clear, so that I don't have to deal with the breakage in the future.

[sagetherage]

I have marked for Documentation changes and leaving needs-triage in case there is more to do here - since it isn't pretty

[sagetherage]

@piterpunk this is a breaking change and docs need to be updated, and we would like to know if you can look at a fix for Aluminium point release?

[piterpunk]

Yes, to the point release I can.

[piterpunk]

@sagetherage I just sent the PR with the fix. I guess that all points are addressed: regression reverted, documentation updated and better errors messages.