Add `active` and `inactive` states to the lvol module #1974

Merged
merged 7 commits into from Jul 28, 2016

Projects

None yet

4 participants

@insom
Contributor
insom commented Apr 6, 2016
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lvol

ANSIBLE VERSION

2.1

SUMMARY

lvol lacks the ability to enable or disable a logical volume. My use case is having a VG persist across machine rebuilds - attaching that disk to a "new" machine means its volumes need to be activating before being used. My work around is to use the command module, but it seemed reasonable to add this to lvol itself.

The active and inactive states will do the right thing with 'changed', depending on the previous state of the volume.

TASK [lvol lv=test9-root state=active size=21G vg=containers] ****************
ok: [127.0.0.1]
@gregdek
Contributor
gregdek commented Apr 6, 2016

Thanks @insom. To the current maintainers, @jhoekx, @abulimov please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit' or 'needs_revision' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

@bcoca bcoca and 1 other commented on an outdated diff Apr 7, 2016
system/lvol.py
@@ -45,11 +45,11 @@
Float values must begin with a digit.
Resizing using percentage values was not supported prior to 2.1.
state:
@bcoca
bcoca Apr 7, 2016 Member

instead of overloading state, we could add an active=true/false option

@insom
insom Apr 7, 2016 Contributor

Yeah. I'd originally thought overloading state would be following on from how apt does it, but now that you mention it another variable is more flexible: you could create a new, disabled, lv with state=present active=false. There's no way in my current version you could do that with just one action. Will update the PR.

@gregdek
Contributor
gregdek commented Apr 28, 2016

Thanks @insom for this PR. Unfortunately, it is not mergeable in its current state due to merge conflicts. Please rebase your PR. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

@insom
Contributor
insom commented Apr 30, 2016

ready_for_review

@gregdek
Contributor
gregdek commented May 1, 2016

Thanks @insom. To the current maintainers, @jhoekx, @abulimov please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit', 'needs_revision' or 'close_me' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

@abulimov
Contributor

Thank you @insom! Could you please add usage examples?

@abulimov abulimov commented on the diff May 12, 2016
system/lvol.py
@@ -382,6 +395,22 @@ def main():
else:
module.fail_json(msg="Unable to resize %s to %s%s" % (lv, size, size_unit), rc=rc, err=err)
+ if this_lv is not None:
@abulimov
abulimov May 12, 2016 Contributor

Looks like if we want to activate/deactivate already present volume without specifying size,
like lvol: vg=firefly lv=test active=no, we won't get there because of check at line 289. Could you please tell me if I'm missing something, or this is intended?

@insom
Contributor
insom commented May 12, 2016

@abulimov Thanks for the review. I've added some usage examples, and you're right, there was a code path which didn't fire correctly if you didn't specify size. I've added a commit to change this.

Here' the output of a run an accompanying YAML:

---
- hosts: all
  become: true
  tasks:
  - lvol: lv=aaron vg=pgift-vs1 size=10G
  - lvol: lv=aaron vg=pgift-vs1 active=false
  - lvol: lv=aaron vg=pgift-vs1 active=true
  - lvol: lv=aaron vg=pgift-vs1 active=true
(venv)iweb@pgift-vs1:~/ansible$ ansible-playbook -i localhost, test.yml

PLAY [all] *********************************************************************

TASK [setup] *******************************************************************
ok: [localhost]

TASK [lvol] ********************************************************************
changed: [localhost]

TASK [lvol] ********************************************************************
changed: [localhost]

TASK [lvol] ********************************************************************
changed: [localhost]

TASK [lvol] ********************************************************************
ok: [localhost]

PLAY RECAP *********************************************************************
localhost                  : ok=5    changed=3    unreachable=0    failed=0

So, changed is being toggled correctly now, too.

@abulimov abulimov commented on an outdated diff May 12, 2016
system/lvol.py
@@ -353,6 +367,9 @@ def main():
else:
module.fail_json(msg="Unable to resize %s to %s%s" % (lv, size, size_unit), rc=rc, err=err)
+ elif not size:
@abulimov
abulimov May 12, 2016 Contributor

The if/elif/else order here looks a bit fragile, as it depends on default size_opt = 'L' on line 209. Maybe we should check if size if specified before size_opt == 'l' check on line 330?

@insom
Contributor
insom commented May 12, 2016

@abulimov Good call, I've moved it up so that it bails from this if much earlier, and the test YAML will works just fine.

@abulimov
Contributor

Thank you @insom, everything looks good for me now.

shipit!

@gregdek
Contributor
gregdek commented May 12, 2016

Thanks again to @insom for this PR, and thanks @jhoekx, @abulimov for reviewing. Marking for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek gregdek added shipit and removed community_review labels May 12, 2016
@gregdek
Contributor
gregdek commented May 13, 2016

Thanks @insom for this PR. Unfortunately, it is not mergeable in its current state due to merge conflicts. Please rebase your PR. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek gregdek added needs_revision and removed shipit labels May 13, 2016
@insom
Contributor
insom commented May 13, 2016

ready_for_review

@gregdek
Contributor
gregdek commented May 13, 2016

Thanks @insom. To the current maintainers, @jhoekx, @abulimov please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit', 'needs_revision' or 'close_me' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

@abulimov
Contributor

shipit!

@bcoca bcoca merged commit 3533ae2 into ansible:devel Jul 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@calamityman calamityman added a commit to calamityman/ansible-modules-extras that referenced this pull request Jul 28, 2016
@calamityman Aaron Brady + calamityman Add `active` and `inactive` states to the lvol module (#1974)
* Add `active` and `inactive` states to the lvol module

* Honor the previous state of the changed variable

* Move active/inactive states to active boolean parameter

* Bump version_added to make Travis happy

* Avoid bailing early is size isn't specified

* Add invocation examples

* Move "no size" up for code clarity
a1f08df
@woz5999 woz5999 pushed a commit to woz5999/ansible-modules-extras that referenced this pull request Aug 2, 2016
Aaron Brady + Jeff Wozniak Add `active` and `inactive` states to the lvol module (#1974)
* Add `active` and `inactive` states to the lvol module

* Honor the previous state of the changed variable

* Move active/inactive states to active boolean parameter

* Bump version_added to make Travis happy

* Avoid bailing early is size isn't specified

* Add invocation examples

* Move "no size" up for code clarity
f20761e
@defionscode defionscode pushed a commit to defionscode/ansible-modules-extras that referenced this pull request Aug 12, 2016
Aaron Brady + Jonathan Davila Add `active` and `inactive` states to the lvol module (#1974)
* Add `active` and `inactive` states to the lvol module

* Honor the previous state of the changed variable

* Move active/inactive states to active boolean parameter

* Bump version_added to make Travis happy

* Avoid bailing early is size isn't specified

* Add invocation examples

* Move "no size" up for code clarity
88be43c
@defionscode defionscode pushed a commit to defionscode/ansible-modules-extras that referenced this pull request Aug 12, 2016
Aaron Brady + Jonathan Davila Add `active` and `inactive` states to the lvol module (#1974)
* Add `active` and `inactive` states to the lvol module

* Honor the previous state of the changed variable

* Move active/inactive states to active boolean parameter

* Bump version_added to make Travis happy

* Avoid bailing early is size isn't specified

* Add invocation examples

* Move "no size" up for code clarity
d9d2fc3
@slaingnials slaingnials added a commit to slaingnials/ansible-modules-extras that referenced this pull request Oct 24, 2016
@slaingnials Aaron Brady + slaingnials Add `active` and `inactive` states to the lvol module (#1974)
* Add `active` and `inactive` states to the lvol module

* Honor the previous state of the changed variable

* Move active/inactive states to active boolean parameter

* Bump version_added to make Travis happy

* Avoid bailing early is size isn't specified

* Add invocation examples

* Move "no size" up for code clarity
c42ce52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment