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

cs instance root_disk size update resizes the root volume #43817

Merged
merged 4 commits into from
Aug 28, 2018

Conversation

greut
Copy link
Contributor

@greut greut commented Aug 8, 2018

SUMMARY

CloudStack's resizeVolume on the ROOT volume of a VM based on the root_disk_size.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

cloud / cloudstack / cs_instance

ANSIBLE VERSION
ansible 2.7.0.dev0 (cs-instance-root-disk-size c9a9a5d39a) last updated 2018/08/08 11:47:28 (GMT +200)
  config file = None
  configured module search path = ['/home/yoan/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/yoan/soft/ansible/lib/ansible
  executable location = /home/yoan/soft/ansible/venv36/bin/ansible
  python version = 3.6.6 (default, Aug  6 2018, 17:27:47) [GCC 8.2.0]
ADDITIONAL INFORMATION

Assuming, you created an instance with a root_disk_size or based on a template supporting it. Changing its value will triggers a resizeVolume call applying the changes. shrinkok is an extra option.

      cs_instance:
        template: Linux Debian 9 64-bit
        name: test
        state: present
        root_disk_size: 14
        shrinkok: true
        force: true

Signed-off-by: Yoan Blanc <yoan.blanc@exoscale.ch>
Reviewed-by: Marc-Aurèle Brothier <m@brothier.org>
@greut greut changed the title Cs instance root disk size cs instance root_disk size update resizes the root volume Aug 8, 2018
@ansibot
Copy link
Contributor

ansibot commented Aug 8, 2018

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 cloud cloudstack community_review In order to be merged, this PR must follow the community review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. labels Aug 8, 2018
Signed-off-by: Yoan Blanc <yoan.blanc@exoscale.ch>
@ansibot
Copy link
Contributor

ansibot commented Aug 8, 2018

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

lib/ansible/modules/cloud/cloudstack/cs_instance.py:0:0: E309 version_added for new option (shrinkok) should be 2.7. Currently 0.0
lib/ansible/modules/cloud/cloudstack/cs_instance.py:0:0: E323 "shrinkok" is listed in DOCUMENTATION.options, but not accepted by the module

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Aug 8, 2018
Signed-off-by: Yoan Blanc <yoan.blanc@exoscale.ch>
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Aug 8, 2018

if root_disk_size is not None:
res = self.query_api('listVolumes', type='ROOT', virtualmachineid=instance['id'])
[volume] = res['volume']
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we catch a just in case missing volume in the response to be cleaner?:

volume = res.get('volume')
if volume:
  ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defensive programming doesn't help figuring it out what's wrong, in case something is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

True

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 8, 2018
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Aug 9, 2018
@resmo
Copy link
Contributor

resmo commented Aug 9, 2018

Thanks for the contribution. code looks good to me. One thing I don't like that much is the param name shrinkok, what about allow_shrink or allow_root_disk_shrink?

@marcaurele
Copy link
Contributor

You're right we need something more meaningful to relate to the root disk of the VM since we are in the instance module. I'd prefer the allow_root_disk_shrink or simply root_disk_shrink.

Signed-off-by: Yoan Blanc <yoan.blanc@exoscale.ch>
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Aug 10, 2018
if shrinkok:
args_volume_update['shrinkok'] = shrinkok

root_disk_size_changed = root_disk_size != size
Copy link
Contributor

@resmo resmo Aug 18, 2018

Choose a reason for hiding this comment

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

one more question, this would also be "true" for enlargement of the disk, would this work as expected? if yes, allow_root_disk_shrink would be a misleading name because it would not only resize to smaller, but also resize to bigger volume --> allow_root_disk_resize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's really only for _shrinkagethat a special flag needs to be sent.allow_root_disk_resize` would be misleading as the nothing special is required for enlarging a disk.

http://cloudstack.apache.org/api/apidocs-4.4/user/resizeVolume.html

Copy link
Contributor

Choose a reason for hiding this comment

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

@resmo the shrink option is only for changing the disk size to a smaller partition. It requires more actions from the user if it should success:

Copy link
Contributor

Choose a reason for hiding this comment

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

so, that means, we should change this condition to the following, right?

root_disk_size_changed = root_disk_size < size

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you can resize the volume up or down, but if you want to resize it down you must specify the flag shrink to ensure you know what you are doing since it requires some preparation of the volume.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, got it! 👍

Copy link
Contributor

@resmo resmo left a comment

Choose a reason for hiding this comment

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

needs_info

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 18, 2018
@resmo
Copy link
Contributor

resmo commented Aug 28, 2018

rebuild_merge

@resmo
Copy link
Contributor

resmo commented Aug 28, 2018

bot_status

@ansibot
Copy link
Contributor

ansibot commented Aug 28, 2018

Components

lib/ansible/modules/cloud/cloudstack/cs_instance.py
support: community
maintainers: dpassante resmo

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 0
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainer or core team member): []
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 28, 2018
@resmo
Copy link
Contributor

resmo commented Aug 28, 2018

rebuild_merge

@ansibot ansibot merged commit cf61510 into ansible:devel Aug 28, 2018
@greut greut deleted the cs-instance-root-disk-size branch December 5, 2018 14:41
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 cloud cloudstack community_review In order to be merged, this PR must follow the community review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants