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

new module: AIX Volume Group creating, resizing, removing #30381

Open
wants to merge 13 commits into
base: devel
from

Conversation

Projects
None yet
7 participants
@kairoaraujo
Contributor

kairoaraujo commented Sep 14, 2017

SUMMARY

It is a module to manage Volume Groups to AIX Logical Volume
Manager. With this module is able to create, resize and
remove a Volume Group.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

aix_lvg

ANSIBLE VERSION
ansible 2.5.0 (aix_lvg bfeb840b21) last updated 2017/09/14 22:05:36 (GMT +200)
  config file = None
  configured module search path = ['/Users/kairoaraujo/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/kairoaraujo/Dev/aix_lvg/ansible/lib/ansible
  executable location = /Users/kairoaraujo/Dev/aix_lvg/ansible/bin/ansible
  python version = 3.6.2 (v3.6.2:5fd33b5926, Jul 16 2017, 20:11:06) [GCC 4.2.1 (Apple Inc. build 5666) (dot 3)]
ADDITIONAL INFORMATION

None

new module: AIX Volume Group creating, resizing, removing
It is a module to manage Volume Groups to AIX Logical Volume
Manager. With this module is able to create, reisize and
remove a Volume Group.
@ansibot

This comment has been minimized.

Contributor

ansibot commented Sep 14, 2017

short_description: Configure LVM volume groups for AIX
description:
- This module creates, removes or resize volume groups on AIX LVM.
version_added: 2.5

This comment has been minimized.

@jtyr

jtyr Sep 14, 2017

Contributor

The value should be quoted.

__metaclass__ = type
ANSIBLE_METADATA = {'metadata_version': '1.1',

This comment has been minimized.

@jtyr

jtyr Sep 14, 2017

Contributor

Please format this like this:

ANSIBLE_METADATA = {
    'metadata_version': '1.1',
    'status': ['preview'],
    'supported_by': 'community'
}
options:
vg:
description:
- Volume Group name

This comment has been minimized.

@jtyr

jtyr Sep 14, 2017

Contributor

Please indent the array items by two characters like you did for example above in the case of the description. The same in all cases bellow.

Also make the Group lowercase to unify it across all documentation. And add period at the end of the sentence.

vg_type:
description:
- Volume Group type.
choices: ["normal", "big", "scalable"]

This comment has been minimized.

@jtyr

jtyr Sep 14, 2017

Contributor

The values in the array should not be quoted.

state:
description:
- Control if the volume group exists.
choices: [ "present", "absent" ]

This comment has been minimized.

@jtyr

jtyr Sep 14, 2017

Contributor

The values in the array should not be quoted.

def _validate_pv(module, pvname):
"""
function to validate if the Physical Volume is not already in use by

This comment has been minimized.

@jtyr

jtyr Sep 14, 2017

Contributor

Capital letter at the beginning and period at the end of the sentence.

required: false
vg_type:
description:
- Volume Group type.

This comment has been minimized.

@jtyr

jtyr Sep 14, 2017

Contributor

Make the Group lowercase to unify it across all documentation.

pvs:
description:
- List of comma-separated devices to use as physical devices in this volume
group. Required when creating or extending ('present' state) the volume

This comment has been minimized.

@jtyr

jtyr Sep 14, 2017

Contributor

present is a value of a parameter so it should be decorated like this: C(present). The same for the absent bellow.

DOCUMENTATION = '''
---
author: "Kairo Araujo (@kairoaraujo)"

This comment has been minimized.

@jtyr

jtyr Sep 14, 2017

Contributor

The value doesn't have to be quoted.

required: false
force:
description:
- Forces volume group creation

This comment has been minimized.

@jtyr

jtyr Sep 14, 2017

Contributor

Missing period at the end of sentence.

@molekuul

This comment has been minimized.

Contributor

molekuul commented Sep 15, 2017

Hi,
Nice to see someone is working on AIX, I thought I was the onlyone :-)

I have some remarks on the module:
Ansible modules are to set a status of a configuration, if you want to extend a volumegroup with a disk that's already in this volumegroup, the module should not fail, but should return changed: False, nothing changed.

When a disk is in use by a volumegroup, you exit the module with a fail,
but when the disk is in use as an ASM disk, you set the return to False, and let the create_extend_vg function handle this, why these different approaches?

If you give a list of disks, the module will fail if even one disk will fail the "usage check" is this wat you want?

Checkmode is a mode in Ansible where you check what will be changed when you run the module with these parameters, without actually doing the action. You didn't incorporate this in the module, however you state that you support checkmode.

There are limits of how many PP's a disk can contain. An AIX system calulates the PP size from the size of the disk if you do not specify this. why not let the system decide the value of the PP-size if not specified in the playbook?

When you are removing a pv from a volumegroup, you do not check if the pv is within this volumegroup, so the command will fail, but the result is correct, the disk is not part of the volumegroup, as you want, you should handle this situation better.

Take into account that a volumegroup must be varyon before taking actions on a volumegroup.

I hope you can do anything with my remarks.

Kind regards,

Joris Weijters

@kairoaraujo

This comment has been minimized.

Contributor

kairoaraujo commented Sep 15, 2017

Hi @molekuul

Yes, as AIX is my main OS I am writing some modules, I have also almost ready to submit modules for filesystem and mksysb. Remarks are always welcome :)
Please, could you also check out my packaging/os/installp PR #30238 ?

I will go through your remarks and see I'll what I can do.

@kairoaraujo

This comment has been minimized.

Contributor

kairoaraujo commented Sep 15, 2017

Working on @molekuul remarks.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Sep 15, 2017

@kairoaraujo This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

kairoaraujo added some commits Sep 15, 2017

fixed suggestion for standards and PEP8
fixed suggestion on PR according with standards and PEP8
Removed blank space in the end of line.
Removed blank space in the end of line.

@kairoaraujo kairoaraujo force-pushed the kairoaraujo:aix_lvg branch to 49682f0 Sep 15, 2017

type: boolean
version_added: 2.5
msg:
description: Return message regarding the action.

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

Please indent it by 2 spaces only.

state=dict(choices=["absent", "present"], default="present"),
force=dict(type="bool", default=False)
),
supports_check_mode=True,

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

You claim support for the check mode but I don't see any implementation here. Please could you explain?

This comment has been minimized.

@kairoaraujo

kairoaraujo Sep 15, 2017

Contributor

I was confused how to implement it but your question clarified :)
I need to implement it.

force = module.params['force']
if state == 'present':

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

I would remove this blank line.

if not pvs:
module.fail_json(msg="pvs is required to state 'present'")

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

I would remove this blank line.

pvs_to_remove.append(line.split()[0])
reduce_msg = "Volume group %s removed" % vg

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

I would remove this blank line.

if state == 'absent':
reduce_vg(module, vg, pvs)

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

Here should be two blank lines.

lspv_cmd = module.get_bin_path('lspv', True)
rc, current_lspv, err = module.run_command('%s ' % lspv_cmd)
if rc != 0:
module.fail_json(msg="Failed executing lspv command.", rc=rc,

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

I kind of don't like this sort of wrapping. What about to use the following style?

        module.fail_json(
            msg="Failed executing lspv command.", rc=rc, err=err)

The same for all similar cases bellow.

This comment has been minimized.

@kairoaraujo

kairoaraujo Sep 15, 2017

Contributor

I can change it if it looks better.

This comment has been minimized.

@kairoaraujo

kairoaraujo Sep 15, 2017

Contributor

@jtyr which one Should I use?

module.fail_json(
            msg='Failed executing lspv command.', rc=rc, err=err)

or

module.fail_json(
            msg = 'Failed executing lspv command.', rc = rc, err = err)

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

The first one is the only correct one. And you should use double quotes as per the above comment.

This comment has been minimized.

@kairoaraujo

kairoaraujo Sep 15, 2017

Contributor

Regarding single/double quotes used across the code, there is non-written convention to use double quotes only for text with placeholders (e.g. "%s ") or longer text messages (e.g. msg="Failed executing lspv command."). When a text contains double quotes, to avoid escaping the quotes, single quotes should be used (e.g. my_var = 'This is "my" variable.'). The rest of cases should use single quotes. Please walk through your code and unify the quotes according this convention.

I'm still confused with this non-written convention to use double quotes and I want to have it clear once I want to send more contributions :)

"The rest of cases should use single quotes."

a. For text with placeholders as "%s" or "{0}".format(variable) use double quotes;
b. For long text/sentences as msg = "This is an error message.";
c. Texts with quotes, to avoid escaping single quotes: my_var = 'This is "my" variable.'
d. In case of text with variables, double quote? msg = "The VG %s was changed"

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

That all sounds correct.

"""
lspv_cmd = module.get_bin_path('lspv', True)
rc, current_lspv, err = module.run_command('%s ' % lspv_cmd)

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

Regarding single/double quotes used across the code, there is non-written convention to use double quotes only for text with placeholders (e.g. "%s ") or longer text messages (e.g. msg="Failed executing lspv command."). When a text contains double quotes, to avoid escaping the quotes, single quotes should be used (e.g. my_var = 'This is "my" variable.'). The rest of cases should use single quotes. Please walk through your code and unify the quotes according this convention.

This comment has been minimized.

@kairoaraujo

kairoaraujo Sep 15, 2017

Contributor

indeed

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

I see did some changes of single quotes to double quotes but you left all double quotes are the were. You should change all other cases than named above to single quotes (e.g. keys in the dict vars - my_dict['my_key']).

force_mode[force], vg, ' '.join(pvs)
))
if rc == 0:
module.exit_json(changed=True, msg="Volume group %s created" % vg)

This comment has been minimized.

@jtyr

jtyr Sep 15, 2017

Contributor

Please could you change the logic the way that this function returns the changed value and you exit at the end of the main() function? The same in for the reduce_vg() function.

@kairoaraujo

This comment has been minimized.

Contributor

kairoaraujo commented Sep 16, 2017

Hi @molekuul

I would like ask somethings regarding your remarks

Ansible modules are to set a status of a configuration, if you want to extend a volumegroup with a disk that's already in this volumegroup, the module should not fail, but should return changed: False, nothing changed.

I agree with that point and I'm implementing it.
My doubt is when the disk is already in use by another volume group, it should fail or also return not changed?

When a disk is in use by a volumegroup, you exit the module with a fail,
but when the disk is in use as an ASM disk, you set the return to False, and let the create_extend_vg function handle this, why these different approaches?
If you give a list of disks, the module will fail if even one disk will fail the "usage check" is this wat you want?

My approach is wrong, should be the same return for ASM and VG use. IMHO If you give a list of disks and one disk is wrong it should fail and forces the administrator correct the pool and avoid wrong resizes. Anyway, I'm opened to opinions.

Checkmode is a mode in Ansible where you check what will be changed when you run the module with these parameters, without actually doing the action. You didn't incorporate this in the module, however you state that you support checkmode.

I want to implement it, but I'm quite confused how to do it.

There are limits of how many PP's a disk can contain. An AIX system calulates the PP size from the size of the disk if you do not specify this. why not let the system decide the value of the PP-size if not specified in the playbook?

Totally agree, I'm changing it.

When you are removing a pv from a volumegroup, you do not check if the pv is within this volumegroup, so the command will fail, but the result is correct, the disk is not part of the volumegroup, as you want, you should handle this situation better.

It is treated by AIX as well, but for sure I do it also.

TASK [Reducing datavg with disk from rootvg] **************************************************************************************
fatal: [172.16.100.24]: FAILED! => {"changed": false, "err": "0516-1782 reducevg: Physical volume hdisk0 is not part of volume group datavg.\n0516-884 reducevg: Unable to remove physical volume hdisk0.\n", "failed": true, "msg": "Unable to remove datavg", "rc": 2}

We will have also this situation when the volume group still having filesystems mounted or open lv. I think is better leave it up to AIX, no?

Take into account that a volumegroup must be varyon before taking actions on a volumegroup.

Including this verification on the top.
About this subject, I will also include C(action): varyonv/varyoff it will be useful ;)

Kind regards,
Kairo Araujo

Improved validations, functions, pep8
- fixed pep8 and non-written conventions;
- fixed RETURN doc indentation for msg;
- fixed and improved the physical volume verification;
- pp_size, when not specified, is followed from AIX side;
- included volume group verification in the top;
- included state varyon and varyoff to be easy in playbooks;
- removed check_mode
msg="Failed executing lspv command.", rc=rc, err=err)
for pv in pvs:
# get pv list

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

Comments should start with a capital letter. The same anywhere bellow.

# check if PV is already in use for the same vg
elif vg == lspv_list[pv]:
msg = ("Physical volume %s is already used by volume group %s."

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

This should be:

                msg = (
                    "Physical volume %s is already used by volume group %s."
                    % (pv, lspv_list[pv]))
return False, msg
else:
module.fail_json(
msg="Physical volume %s is in use by another volume group "

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

This should be:

                module.fail_json(
                    msg=(
                        "Physical volume %s is in use by another volume group "
                        "%s." % (pv, lspv_list[pv])))
def create_extend_vg(module, vg, pvs, pp_size, vg_type, force, vg_validation):
""" creates or extend a volume group"""

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

This should be:

    """ Creates or extend a volume group. """
def main():
module = AnsibleModule(
argument_spec=dict(
vg=dict(required=True),

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

Please order the parameters alphabetically.

- This module creates, removes or resize volume groups on AIX LVM.
version_added: "2.5"
options:
vg:

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

Please order the options alphabetically.

def reduce_vg(module, vg, pvs, vg_validation):

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

You can remove this blank line.

def state_vg(module, vg, state, vg_validation):

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

You can remove this blank line.

description:
- Control if the volume group exists and volume group AIX state varyonvg
C(varyon) or varyoffvg C(varyoff).
choices: [ present, absent, varyon, varyoff ]

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

Remove spaces at the beginning and the end of the square brackets.

@jtyr

This comment has been minimized.

Contributor

jtyr commented Sep 16, 2017

The Check Mode should be implemented the way that if using ansible-play -C site.yaml, the module doesn't perform any real changes, it just reports whether it would perform any change if run without the Check Mode. In your case, you should just check if there already exists VG of that name or parameters and if not, you should not do any changes but just return a flag that a change would be done if running without a Check Mode. Code-wise it would look like this:

def state_vg(...):
    changed = False

    ...

    if state == 'varyon':
        if vg_state is False:
            changed = True

            if not module.check_mode:
                varyonvg_cmd = module.get_bin_path("varyonvg", True)
                rc, varyonvg_out, err = module.run_command(
                    "%s %s" % (varyonvg_cmd, vg))

                if rc != 0:
                    module.failed_json(msg="...", stdout=out, stderr=err)

    ...

    return changed


def main():
    if state == 'present':
        changed = create_extend_vg(...)
    elif state == 'absent':
        changed = reduce_vg(...)
    elif state == 'varyon' or 'varyoff':
         changed = state_vg(...)

    module.exit_json(changed=True, state=state)
if rc == 0:
module.exit_json(changed=True, msg="Volume group %s created." % vg)
else:
module.exit_json(

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

This should be fail_json().

if rc == 0:
module.exit_json(changed=True, msg=reduce_msg)
else:
module.exit_json(

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

This should be fail_json().

rc, varyonvg_out, err = module.run_command(
"%s %s" % (varyonvg_cmd, vg))
if rc != 0:
module.exit_json(changed=False, rc=rc, err=err)

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

This should be fail_json().

rc, varyonvg_out, err = module.run_command(
"%s %s" % (varyonvg_cmd, vg))
if rc != 0:
module.exit_json(changed=False, msg=err)

This comment has been minimized.

@jtyr

jtyr Sep 16, 2017

Contributor

This should be fail_json().

Implemented back module.check_module, pep8, return
Implemented back the module.check_module to permit be used.
Some corrections regarding module.exit_json() and
module.fail_json().
Some pep8 and non-written conventions
changed=True,
msg="Varyon volume group %s completed." % vg)
return changed

This comment has been minimized.

@jtyr

jtyr Sep 17, 2017

Contributor

Why do you return something when you are not doing nothing with it in the main() function? What about to use module.exit_json() only in the main() function? The same in other functions.

This comment has been minimized.

@kairoaraujo

kairoaraujo Sep 18, 2017

Contributor

Could I do it as I did in the comment above?

return changed
else:
module.exit_json(

This comment has been minimized.

@jtyr

jtyr Sep 17, 2017

Contributor

I'm sure this can be on one line.

if not pvs:
module.fail_json(msg='pvs is required to state "present".')
else:
create_extend_vg(module, vg, pvs, pp_size, vg_type, force,

This comment has been minimized.

@jtyr

jtyr Sep 17, 2017

Contributor

This should follow the same formatting principle like above:

            create_extend_vg(
                module, vg, pvs, pp_size, vg_type, force, vg_validation)
if state == 'varyon':
if vg_state is False:
changed = True

This comment has been minimized.

@jtyr

jtyr Sep 17, 2017

Contributor

This should be on the function level to avoid multiple definitions of the same variable on multiple places in the same function.

This comment has been minimized.

@kairoaraujo

kairoaraujo Sep 18, 2017

Contributor

Sorry, maybe I didn't get it.
I tried to follow the example that you gave me previously and I did a mess.
In this case, this block should be like I did below?

    if state == 'varyon':
        if vg_state is False:
            if not module.check_mode:
                varyonvg_cmd = module.get_bin_path("varyonvg", True)
                rc, varyonvg_out, err = module.run_command(
                    "%s %s" % (varyonvg_cmd, vg))
                if rc != 0:
                    module.fail_json(msg="Varyonvg failed.", rc=rc, err=err)
                else:
                    module.exit_json(
                        changed=True,
                        msg="Varyon volume group %s completed." % vg)
            module.exit_json(changed=True)

        else:
            module.exit_json(changed=False, msg=msg)

This comment has been minimized.

@jtyr

jtyr Sep 18, 2017

Contributor

Do not call exit_json() from inside of this function. The function should return the changed value and you should use exit_json() at the end of the main() function.

This comment has been minimized.

@kairoaraujo

kairoaraujo Sep 19, 2017

Contributor

Indeed. In that case I will return changedand msg and use exit_json() in end of main() and I will perform it for all other functions.

Regarding fail_json() Can I keep it into the functions, right? If not, I need to create some standard return like:

# type: fail or exit
# changed Fail or True
return type=fail, changed=False, msg=msg, rc=rc, err=err

This comment has been minimized.

@jtyr

jtyr Sep 19, 2017

Contributor

You can keep the fail_json() in the functions. Only the exit_json() use from the main() only.

def create_extend_vg(module, vg, pvs, pp_size, vg_type, force, vg_validation):
""" Creates or extend a volume group"""

This comment has been minimized.

@jtyr

jtyr Sep 17, 2017

Contributor

This should be:

""" Creates or extend a volume group. """

kairoaraujo added some commits Sep 20, 2017

removed exit_json() from functions and pep8
- ordered option parameters and pep8 compliance
- removed exit_json() from functions and moved to main()
pep8, two spaces before function
pep8, two spaces before function

@ansibot ansibot added the shipit label Oct 18, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 16, 2017

The test ansible-test sanity --test no-underscore-variable [?] failed with the following error:

Command "test/sanity/code-smell/no-underscore-variable.sh" returned exit status 4.
>>> Standard Output
== Underscore used as a variable ==
./lib/ansible/modules/system/aix_lvg.py:            rc, _, err = module.run_command(
./lib/ansible/modules/system/aix_lvg.py:            rc, _, err = module.run_command(
./lib/ansible/modules/system/aix_lvg.py:            rc, _, err = module.run_command(

click here for bot help

@ansibot ansibot added needs_revision and removed shipit labels Nov 16, 2017

no-underscore-variable applied to the code
no-underscore-variable was applied to the code
@maxamillion

This comment has been minimized.

Contributor

maxamillion commented Sep 19, 2018

rebuild_merge

@ansibot ansibot removed the stale_ci label Sep 19, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Sep 19, 2018

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/system/aix_lvg.py:0:0: E307 version_added should be 2.8. Currently 2.5
lib/ansible/modules/system/aix_lvg.py:0:0: E325 argument_spec for "force" defines type="bool" but documentation does not
lib/ansible/modules/system/aix_lvg.py:0:0: E326 Value for "choices" from the argument_spec ([]) for "force" does not match the documentation ([True, False])

click here for bot help

@maxamillion

This comment has been minimized.

Contributor

maxamillion commented Sep 20, 2018

@kairoaraujo Apologies for the lag time on this, but if you don't mind fixing up the sanity tests I'm happy to merge.

@kairoaraujo

This comment has been minimized.

Contributor

kairoaraujo commented Sep 20, 2018

I will take care of it this weekend.

@ansibot ansibot added the stale_ci label Sep 28, 2018

Fixed version and doc
Fixed version and documentation sanity
@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 14, 2018

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/system/aix_lvg.py:0:0: E325 argument_spec for "force" defines type="bool" but documentation does not
lib/ansible/modules/system/aix_lvg.py:0:0: E326 Value for "choices" from the argument_spec ([]) for "force" does not match the documentation ([True, False])

click here for bot help

@ansibot ansibot removed the stale_ci label Oct 14, 2018

@ansibot ansibot added the stale_ci label Oct 25, 2018

@gforster

Looks like all that is needed is to change
force: description: - Forces volume group creation. choices: [True, False] default: "no"

to

force: description: - Forces volume group creation. type: bool default: "no"
If I'm reading the error and the documentation correctly

Fixed documentation for E325, E326
Fixed documentation for E325, E326 to force option
@maxamillion

This comment has been minimized.

Contributor

maxamillion commented Dec 10, 2018

needs_revision

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment