-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
CLOUDSTACK-9707: While using hostid parameter, vm gets deployed on an… #1868
Conversation
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests: Skipped tests: Passed test suits: |
Code changes LGTM, also verified that deployment is not retried in case of failure if host is specified |
Do we need to add a global setting to determine whether deploy to other hosts if the specified host is out of capacity ? |
@ustcweizhou, In my opinion we should not do that as that way we are giving them option of silently deploying VM on some host which he might not be aware of. As after specifying host he is expecting that VM will be deployed on that host. If he is ok with deploying VM on different host then he can try deploying VM in next attempt without specifying hostid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM code changes.
@anshul1886 |
@ustcweizhou, Actually I am restoring to old behaviour of 3.x release which got changed to current behaviour because of bug. |
@ustcweizhou, Is it looking good to you now? |
@anshul1886 |
376a1c9
to
b7cc8fa
Compare
@ustcweizhou Added the global setting allow.deploy.vm.if.deploy.on.given.host.fails to control the behaviour. |
@anshul1886 good, I will test it. |
@ustcweizhou I think default value should be false. As it tells the user to make informed decision instead of misleading according to original purpose of parameter. @sateesh-chodapuneedi @koushik-das What do you guys think? |
@blueorangutan package |
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-579 |
@blueorangutan test |
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Trillian test result (tid-942)
|
The point of specifying a host is in deploy request itself means inclination towards using that host for instance. Hence I think default value should be false. |
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests:
Skipped tests: Passed test suits: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM based on code review, manual verification and test results.
tag:mergeready |
…other if the host given is running out of capacity. If host id is specified the deployment should happen on the given host and it should fail if the host is out of capacity. We are retrying deployment on the entire zone without the given host id if we fail once. The retry, which will retry on other hosts, should only be attempted if host id isn't given. Also, introduces global setting allow.deploy.vm.if.deploy.on.given.host.fails with which old behaviour can be restored
fe4a974
to
f52719a
Compare
…other if the host
given is running out of capacity. If host id is specified the deployment should happen
on the given host and it should fail if the host is out of capacity. We are retrying
deployment on the entire zone without the given host id if we fail once. The retry,
which will retry on other hosts, should only be attempted if host id isn't given.