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.

Sign up for GitHub

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

Jump to bottom

vmware_resource_pool: Add new allocation shares options for cpu and memory #461

Merged
merged 5 commits into from
Oct 28, 2020
Merged

vmware_resource_pool: Add new allocation shares options for cpu and memory #461

merged 5 commits into from
Oct 28, 2020
+151 −2

Conversation

sky-joker
Copy link
Collaborator

SUMMARY

This PR is to add new allocation shares options for cpu and memory to the vmware_resource_pool module.

fixes: #444

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/modules/vmware_resource_pool.py
tests/integration/targets/vmware_resource_pool/tasks/main.yml

ADDITIONAL INFORMATION

tested on vCenter/ESXi 7.0

@sky-joker sky-joker changed the title [WIP] Add new allocation shares options for cpu and memory Add new allocation shares options for cpu and memory Oct 24, 2020
@sky-joker sky-joker mentioned this pull request Oct 24, 2020
Closed
@sky-joker
Copy link
Collaborator Author

recheck

9 similar comments
@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@Akasurde Akasurde changed the title Add new allocation shares options for cpu and memory vmware_resource_pool: Add new allocation shares options for cpu and memory Oct 26, 2020
Akasurde
Akasurde requested changes Oct 26, 2020
View reviewed changes
changelogs/fragments/461_vmware_resource_pool.yml Outdated
@@ -0,0 +1,2 @@
minor_changes:
- vmware_resource_pool - Add new allocation shares options for cpu and memory(https://github.com/ansible-collections/community.vmware/pull/461).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- vmware_resource_pool - Add new allocation shares options for cpu and memory(https://github.com/ansible-collections/community.vmware/pull/461).
- vmware_resource_pool - add new allocation shares options for cpu and memory(https://github.com/ansible-collections/community.vmware/pull/461).

plugins/modules/vmware_resource_pool.py Show resolved Hide resolved
plugins/modules/vmware_resource_pool.py Show resolved Hide resolved
plugins/modules/vmware_resource_pool.py Outdated
@@ -251,15 +263,24 @@ def state_add_rp(self):
cpu_alloc.limit = int(self.cpu_limit)
cpu_alloc.reservation = int(self.cpu_reservation)
cpu_alloc_shares = vim.SharesInfo()
cpu_alloc_shares.level = self.cpu_shares
if self.cpu_shares == 'custom':
cpu_alloc_shares.level = self.cpu_shares
Copy link
Member

Choose a reason for hiding this comment

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

if level is common for both if block, why not move them out of if.

plugins/modules/vmware_resource_pool.py Show resolved Hide resolved
plugins/modules/vmware_resource_pool.py Outdated
@@ -294,19 +315,25 @@ def main():
resource_pool=dict(required=True, type='str'),
mem_shares=dict(type='str', default="normal", choices=[
'high', 'custom', 'normal', 'low']),
mem_allocation_shares=dict(type='int'),
Copy link
Member

Choose a reason for hiding this comment

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

Can we set some default value just like other parameters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added to the default value based on the normal value on vCenter 7.

tests/integration/targets/vmware_resource_pool/tasks/main.yml
@@ -97,3 +97,42 @@
assert:
that:
- "{{ resource_result_0004.changed == false }}"

# Testcase 0005: Add Resource pool with the mem_allocation_shares and cpu_allocation_shares parameters
- name: add resource pool
Copy link
Member

Choose a reason for hiding this comment

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

Can you check idempotency here?

@sky-joker
Copy link
Collaborator Author

Thank you @Akasurde for reviewing this PR.

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

@Akasurde Done!

Akasurde
Akasurde approved these changes Oct 27, 2020
View reviewed changes
Copy link
Member

@Akasurde Akasurde left a comment

Choose a reason for hiding this comment

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

LGTM

mariolenz
mariolenz reviewed Oct 27, 2020
View reviewed changes
Copy link
Collaborator

@mariolenz mariolenz left a comment

Choose a reason for hiding this comment

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

I'm not sure, but it looks like the module returns change=False even if cpu_allocation_shares or mem_allocation_shares (or, in fact, anything else) differs from the current resource pool configuration. Is this correct? That would be an accident waiting to happen... but I'm not sure if this really needs to be fixed in this PR. Maybe we should open a new issue for this.

Otherwise, the code LGTM.

@sky-joker
Copy link
Collaborator Author

sky-joker commented Oct 28, 2020 •
edited
Loading

@mariolenz

I'm not sure, but it looks like the module returns change=False even if cpu_allocation_shares or mem_allocation_shares (or, in fact, anything else) differs from the current resource pool configuration. Is this correct?

You're right.
Current supported at this module is the only creation of a new pool and removing a pool.
This module hasn't the changing function of a resource pool config value.
So the resource pool config value will not break.
I was going to create a new issue about it after merging this PR.
First, I thought necessary to add the new options.

@sky-joker
Copy link
Collaborator Author

sky-joker commented Oct 28, 2020 •
edited
Loading

I'm thinking I'm going to work on this issue(#467) after merging this PR.
But should I handle the issue in this PR?
What do you think?

@mariolenz
Copy link
Collaborator

I'm thinking I'm going to work on this issue(#467) after merging this PR.
But should I handle the issue in this PR?
What do you think?

I think your plan to implement new allocation shares options for cpu and memory in this PR and work on changing config values in another one makes sense.

@Akasurde You asked for some idempotency tests. This hasn't been "officially" resolved, but since you said that the code looks good to you after the update the integration test commit, I'll add the gate label.

@mariolenz mariolenz added the gate label Oct 28, 2020
@ansible-zuul ansible-zuul bot merged commit cc33a1d into ansible-collections:main Oct 28, 2020
@sky-joker
Copy link
Collaborator Author

Thank you @mariolenz @Akasurde

@sky-joker sky-joker deleted the fix_444 branch October 29, 2020 01:42
@Akasurde Akasurde mentioned this pull request Oct 29, 2020
Closed
@sky-joker sky-joker mentioned this pull request Nov 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariolenz mariolenz mariolenz left review comments

@Akasurde Akasurde Akasurde approved these changes

@goneri goneri Awaiting requested review from goneri

Assignees
No one assigned
Labels
has_issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vmware_resource_pool can't set custom shares
3 participants
@sky-joker @mariolenz @Akasurde