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

Addition of Spot instance support for VM and VMSS #559

Merged
merged 18 commits into from
Aug 6, 2021

Conversation

ElPrincidente
Copy link
Contributor

SUMMARY

Fixes #397

Addition of Spot VM instance for both azure_rm_virtualmachine and azure_rm_virtualmachinescaleset by adding a priority field. Azure python SDK was already supporting the feature.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

azure_rm_virtualmachine
azure_rm_virtualmachinescaleset

ADDITIONAL INFORMATION

By default Azure API will create a "Regular" VM if priority is not set. If priority is set to "Spot", it will look at eviction_policy and max_price to setup the spot instance.

@Fred-sun
Copy link
Collaborator

Fred-sun commented Jun 7, 2021

@ElPrincidente Thanks for your contribution! Would you add test case to tests/integration/targets/azure_rm_virmachine*/, It will help to merge this PR!

@Fred-sun Fred-sun added medium_priority Medium priority new_feature New feature requirments work in In trying to solve, or in working with contributors labels Jun 7, 2021
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachinescaleset.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachinescaleset.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachinescaleset.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
@ElPrincidente
Copy link
Contributor Author

@ElPrincidente Thanks for your contribution! Would you add test case to tests/integration/targets/azure_rm_virmachine*/, It will help to merge this PR!

@Fred-sun I've incorporated that text markup suggested. Right now I have a few roadblock running integration test with my MSDN Azure Subscription (obsolete SKU, unavailable image size, etc). Will commit the new integration test as soon as possible.

@Fred-sun
Copy link
Collaborator

@ElPrincidente Can these two newly added parameters be updated? If it is possible to update, please help to add the update logic judgment. If not, please indicate that it is not possible to update as in line 1091 and 1092. Thank you very much!

@ElPrincidente
Copy link
Contributor Author

@Fred-sun added update logic to fail if any of the spot parameters (priority, eviction_policy, max_price) are changed. Integration test were also update to test spot VM/VMSS. I had to change the VMSS integration test image since CoreOs has reached EOL and image were removed from Azure.

The only outstanding item is the "eviction_policy" default value, see my comment above

plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachinescaleset.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachinescaleset.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachinescaleset.py Outdated Show resolved Hide resolved
@Fred-sun
Copy link
Collaborator

@ElPrincidente Could you please help to authorize me to update this PR? Thank you very much!

@ElPrincidente
Copy link
Contributor Author

@ElPrincidente Could you please help to authorize me to update this PR? Thank you very much!

@Fred-sun you should be able to modify this PR. I've push the latest change (removing the default value + whitespace formatting comment from above) Thank you !

@Fred-sun
Copy link
Collaborator

Fred-sun commented Jul 6, 2021

@ElPrincidente Thank you. I will help complete this PR.

@Fred-sun
Copy link
Collaborator

@ElPrincidente Could you please help to authorize me to update this PR? Thank you very much!

@Fred-sun you should be able to modify this PR. I've push the latest change (removing the default value + whitespace formatting comment from above) Thank you !

@ElPrincidente I'm sorry, but I still don't have your authorization. Thank you very much!

@ElPrincidente
Copy link
Contributor Author

ElPrincidente commented Jul 12, 2021

@ElPrincidente Could you please help to authorize me to update this PR? Thank you very much!

@Fred-sun you should be able to modify this PR. I've push the latest change (removing the default value + whitespace formatting comment from above) Thank you !

@ElPrincidente I'm sorry, but I still don't have your authorization. Thank you very much!

@Fred-sun I apologize for the confusion, first time collaborating on GH (more used to my company's GHE flow). I've added you as a collaborator to my fork, please advise me if there anything else I need to do in order to authorize you for modifying this PR.

@Fred-sun Fred-sun added ready_for_review The PR has been modified and can be reviewed and merged and removed work in In trying to solve, or in working with contributors labels Jul 12, 2021
@Fred-sun
Copy link
Collaborator

Fred-sun commented Aug 2, 2021

@Fred-sun I apologize for the confusion, first time collaborating on GH (more used to my company's GHE flow). I've added you as a collaborator to my fork, please advise me if there anything else I need to do in order to authorize you for modifying this PR.

@ElPrincidente It‘s OK, I will move forward with the merger as soon as possible. Thank you very much!

@xuzhang3 xuzhang3 merged commit 3c41298 into ansible-collections:dev Aug 6, 2021
Fred-sun added a commit to Fred-sun/ansible_collections_azure that referenced this pull request Aug 11, 2021
…s#559)

* Addition of Spot instance support for VM and VMSS

* PR comment relative to formatting

* Addition of spot instance test for azure_rm_virtualmachine and  vmss

* Changed default priority to None. Added validation for Spot param update

* Removed default values for spot instances parameters + formatting

* small change

Co-authored-by: Vincent Prince <vincent.prince@cae.com>
Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
Co-authored-by: Fred-sun <xiuxi.sun@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority new_feature New feature requirments ready_for_review The PR has been modified and can be reviewed and merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for spot instances
3 participants