You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/07/16 03:14:48 UTC
[GitHub] [cloudstack] GabrielBrascher opened a new pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
GabrielBrascher opened a new pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212
## Description
<!--- Describe your changes in detail -->
This PR adds global settings parameter for setting a strategy for stopping or migrating VMs with local storage when putting a host in maintenance.
Global settings name: `host.maintenance.local.storage.strategy`
Description: _Defines the strategy towards VMs with volumes on local storage when putting a host in maintenance. The default strategy is 'Error', preventing maintenance in such a case. To migrate away VMs running on local storage choose 'Migrating' strategy. To stop VMs, choose 'Stopping' strategy._
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
- Strategy **Error**:
1. put a host into maintenance
2. Maintenance fails and the host gets into ErrorInMaintenance state
- Strategy **Stopping**:
1. put a host into maintenance
2. VMs with volumes on shared storage are migrated, VMs in local storage are stopped, the host gets into Maintenance
- Strategy **Migrating**:
1. put a host into maintenance
2. VMs with volumes on shared storage are migrated, VMs in local storage are stopped, the host gets into Maintenance
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-700497763
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r456015927
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
@wido I think that I answered the _wrong_ question on previous comments ...
Regarding the debug line, personally, I would remove it. The idea behind it would be to avoid costly String operations and therefore optimize CloudStack processing. However, I think that there is a little optimization, and most of the cases CloudStack already runs in Debug mode.
I think that I saw @DaanHoogland and @rafaelweingartner discussing about it on the past.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-659139922
Packaging result: ✔centos7 ✔debian. JID-1584
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-789927145
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r456015927
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
@wido I think that I answered the _wrong_ question on previous comments ... I think that you mentioned the `isDebugEnabled()`, right?
Regarding the `isDebugEnabled()`, personally, I would remove it. The idea behind it would be to avoid costly String operations and therefore optimize CloudStack processing. However, I think that there is a little optimization, and most of the cases CloudStack already runs in Debug mode.
I think that I saw @DaanHoogland and @rafaelweingartner discussing it in the past.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] andrijapanicsb commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-688294482
that sounds good @GabrielBrascher - thx for confirming.
Logic/behaviour is LGTM - but I haven't tested it.
How does it behave with other hypervisors? I trust you've tested KVM only?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-724946078
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-712788860
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r455928072
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
That is a good point. It depends on the strategy configured at the ResourceManager, I will take a look at the best approaches to adapt it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-791901093
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-725591747
<b>Trillian test result (tid-3151)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32709 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4212-t3151-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 84 look OK, 1 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
ContextSuite context=TestKubernetesSupportedVersion>:setup | `Error` | 0.00 | test_kubernetes_supported_versions.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher edited a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher edited a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-669467544
@DaanHoogland I did not answer your Marvin question, sorry for the delay.
To be honest, I had no plans on creating Marvin tests for this. I can take a look at it, though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-796815120
Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 94
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-659132312
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-724946654
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-689500939
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-762407562
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r456015927
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
@wido I think that I answered the _wrong_ question on previous comments ... Did you mention the `isDebugEnabled()`, or the debug message content?
Regarding the `isDebugEnabled()`, personally, I would remove it. The idea behind it would be to avoid costly String operations and therefore optimize CloudStack processing. However, I think that there is a little optimization, and most of the cases CloudStack already runs in Debug mode.
I think that I saw @DaanHoogland and @rafaelweingartner discussing it in the past.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-689535627
Packaging result: ✔centos7 ✔centos8 ✖debian. JID-1940
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-725434055
@DaanHoogland I added your contribution, that log message is better indeed. Also, I fixed the conflicts with the master.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r465954322
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
@DaanHoogland I changed the log level from debug to error on those cases.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-713059649
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-725284605
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2363
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-788561939
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2852
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-689501628
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
rhtyd removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-700497320
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-788550432
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-789928062
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-789733436
@DaanHoogland I am still checking the python changes on the system VM. I should be soon giving a definitive answer (good or "bad"). Good to see that all tests are green.
The failing tests (test_01_migrate_VM_and_root_volume and test_02_migrate_VM_with_two_data_disks) are failing on other PRs as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-819586785
Considering the this implementation only touches at `doMaintaint`and `prepareForMaintenance` methods I think that the test failures at `test_01_migrate_VM_and_root_volume` and `test_02_migrate_VM_with_two_data_disks` are not related to any of the added commits in this PR.
Thanks to all the reviewers @DaanHoogland @andrijapanicsb @rhtyd @wido @RodrigoDLopez; this one looks ready for merge. Are there any concerns that are missing my attention?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-725464413
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2370
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-725269825
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r521213403
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5609,9 +5609,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
}
if (!isOnSupportedHypevisorForMigration(vm)) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(vm + " is not XenServer/VMware/KVM/Ovm/Hyperv, cannot migrate this VM form hypervisor type " + vm.getHypervisorType());
- }
+ s_logger.error(vm + " is not XenServer/VMware/KVM/Ovm/Hyperv, cannot migrate this VM form hypervisor type " + vm.getHypervisorType());
Review comment:
nit picking: the target host can be specified and small spello
```suggestion
s_logger.error(vm + " is not XenServer/VMware/KVM/Ovm/Hyperv, cannot migrate this VM from hypervisor type " + vm.getHypervisorType() + " to hypervisor type " + destinationHost.getHypervisorType());
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-789291139
<b>Trillian test result (tid-3639)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33275 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4212-t3639-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_migrate_VM_and_root_volume | `Error` | 60.50 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 51.58 | test_vm_life_cycle.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-725270259
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-789951120
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2862
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] wido commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
wido commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r455888563
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
Is this debug line still valid?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-713058314
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-762420514
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2547
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r456015927
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
@wido I think that I answered the _wrong_ question on previous comments ... I think that you mentioned the `isDebugEnabled()`, right?
Regarding the debug line, personally, I would remove it. The idea behind it would be to avoid costly String operations and therefore optimize CloudStack processing. However, I think that there is a little optimization, and most of the cases CloudStack already runs in Debug mode.
I think that I saw @DaanHoogland and @rafaelweingartner discussing it in the past.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-762407134
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-724946078
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-725295736
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-724946654
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-788550786
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher edited a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher edited a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-819586785
Considering that this implementation only touches at `doMaintaint`and `prepareForMaintenance` methods I think that the test failures at `test_01_migrate_VM_and_root_volume` and `test_02_migrate_VM_with_two_data_disks` are not related to any of the added commits in this PR.
Thanks to all the reviewers @DaanHoogland @andrijapanicsb @rhtyd @wido @RodrigoDLopez; this one looks ready for merge. Are there any concerns that are missing my attention?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-796798819
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-700497320
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-789660874
@GabrielBrascher sounds like you are not ready to have this merged, do you?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-712788518
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-791900993
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-688280897
Ping for review @rhtyd, @nvazquez, @andrijapanicsb, @borisstoyanov, @DaanHoogland, @svenvogel, @weizhouapache, @RodrigoDLopez, @svenvogel, @kiwiflyer, and others :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-659132111
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-659132312
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
rhtyd removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-689500939
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-669467544
@DaanHoogland I did not answer your Marvin question, sorry for the delay.
I added Unit tests but, to be honest, I had no plans on creating Marvin tests for this. I can take a look at it, though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r456011835
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
@wido I did check it again and noticed that the log message as it is makes sense.
The `migrateVirtualMachine` method does not work with volume migration. When a VM with volumes, e.g. when migrating VMs with local storage, is migrated it is used `migrateWithStorage` method. Therefore, we need to keep it that way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-788906116
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-700530767
Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2098
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-780326627
@GabrielBrascher conflict here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r455928072
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
That is a good point. It depends on the strategy configured at the ResourceService, I will take a look at the best approaches to adapt it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-724974576
Packaging result: ✖centos7 ✔centos8 ✖debian. JID-2357
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-659132111
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r462954509
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
@DaanHoogland your €0,03 is very appreciated ;)
I will change the log level from debug to error (maybe warn, but it looks more like an error).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-688290306
@andrijapanicsb normally VMs are (live) migrated when a host is put into Maintenance; however, if the host has VMs with local storage the host is placed in the "`ErrorInMaintenance`" state; the ADMIN then needs to manually stop/migrate these VMs and then re-try the Maintenance.
Following the same concern that you raised, this implementation keeps the current behavior by default, therefore does not cause backward compatibility to any deployed Zone.
To change the behavior the Root Admin needs to configure the global settings parameter `host.maintenance.local.storage.strategy` on its own risk.
This parameter defines the strategy towards VMs with volumes on local storage when putting a host in maintenance.
1. The default strategy is 'Error', preventing maintenance in such a case.
2. To migrate away VMs running on local storage it is necessary to set `Migrating` strategy.
3. To stop VMs with local storage instead of placing the host into `ErrorInMaintenance`, it is necessary to change the strategy to `Stopping`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-712863170
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2225
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-788307614
Code has been updated adding the option to Stop VM with force stop or a simple stop. Strategies are the following:
1. The default strategy is **'Error'**, preventing maintenance in such a case
2. Choose **'Migration'** strategy to migrate away VMs running on local storage
3. To stop VMs, choose **'Stop'** strategy : `_haMgr.scheduleStop(vm, hostId, WorkType.Stop);`
4. To force-stop VMs, choose **'ForceStop'** strategy: `_haMgr.scheduleStop(vm, hostId, WorkType.ForceStop);`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-791904830
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2875
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#discussion_r462174163
##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5619,7 +5619,7 @@ public VirtualMachine migrateVirtualMachine(Long vmId, Host destinationHost) thr
throw new InvalidParameterValueException("Unsupported Hypervisor Type for User VM migration, we support XenServer/VMware/KVM/Ovm/Hyperv/Ovm3 only");
}
- if (isVMUsingLocalStorage(vm)) {
+ if (isVMUsingLocalStorage(vm.getId())) {
if (s_logger.isDebugEnabled()) {
s_logger.debug(vm + " is using Local Storage, cannot migrate this VM.");
Review comment:
1. As for the message, looks like an extra check for the strategy is in order, but the flow might already guarantee it's validity.
2. As for the second interpretation of the question, as a matter of principle it is always good to check the logging level as parameters are always evaluated before passing and you never know how often the code is being executed.
3. As for the level, this is an obvious mis-use of debug. An error or at least warning or info situation is occurring here; the user might expect a migration but there is something impairing it. This is not a message for a developer, hence not a debug level message.
my €0,03 ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-688302602
@andrijapanicsb I tested with KVM only indeed.
I just re-checked the maintenance behavior with VMs running on shared storage. The zone holds one of the recent commits at the master (not older than one week).
Tested on KVM node with NFS as primary storage:
1. the host has only VMs running with Root disk placed on shared storage
2. call `prepareHostForMaintenance` API command
3. all VMs are live migrated to another host
4. host gets into Maintenance state
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-689501628
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-725295982
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-796798108
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-725434490
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-713058314
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-725434171
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-712788518
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland merged pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-788905377
@blueorangutan test keepEnv
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-788887444
Running tests. The strategy added for Stop is failing; StopForce works fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-797693398
<b>[S] Trillian test result (tid-102)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35571 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4212-t102-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_migrate_VM_and_root_volume | `Error` | 77.42 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 53.18 | test_vm_life_cycle.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-712788860
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] andrijapanicsb commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-688282872
@GabrielBrascher what was the behaviour so far? I believe we were stopping all VMs, right?
I would be happy to see the this new feature to implement the old behaviour as the defaul behaviour - to keep backward compatibility, if that makes sense
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-789912301
Code has been updated reverting the change to allow choosing between Stop or ForceStop. It turns out that the implementation got a bit tricky when HA management calls a VM `Stop` at the maintenance workflow. For now, keeping only VM `ForceStop`.
Fitting the "graceful" Stop into the current maintenance workflow might be a good enhancement for another PR, but for now, keeping the strategy feature as it is:
1. The default strategy is 'Error', preventing maintenance in such a case
2. Choose 'Migration' strategy to migrate away VMs running on local storage
3. To force-stop VMs, choose the 'ForceStop' strategy
Manual tests are looking good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher removed a comment on pull request #4212: Migrate/Stop VMs with local storage when preparing host for maintenance
Posted by GitBox <gi...@apache.org>.
GabrielBrascher removed a comment on pull request #4212:
URL: https://github.com/apache/cloudstack/pull/4212#issuecomment-789733436
@DaanHoogland I am still checking the python changes on the system VM. I should be soon giving a definitive answer (good or "bad"). Good to see that all tests are green.
The failing tests (test_01_migrate_VM_and_root_volume and test_02_migrate_VM_with_two_data_disks) are failing on other PRs as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org