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 2021/12/06 08:44:08 UTC
[GitHub] [cloudstack] DaanHoogland opened a new pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
DaanHoogland opened a new pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750
### Description
This PR fails a migrateData command if any of the files fail to migrate.
In addition it tries to calculate capacity based on the actual size on disk the template takes, and not the virtual size.
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- 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: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [x] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### 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. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1004833560
> @sureshanaparti I had some trouble testing this because of faulty states in template_store_ref that might need attention, however the functionality here all works. cc @Pearl1594 @nvazquez
>
> The problem was that a systemtemplate had three entries in the table two of which were "Migrating" which seems like an unrelated issue as it was before any test from that store was done. before I had succesfully migrated amongst two other stores and to the one at fault without problems.
>
> I think we are ready to merge, given enough review.
thanks @DaanHoogland , will check 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006343505
@blueorangutan test matrix
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778706887
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: %s", template.getState()));
}
List<SnapshotDataStoreVO> snapshots = snapshotDataStoreDao.listByStoreId(srcDataStoreId, DataStoreRole.Image);
for (SnapshotDataStoreVO snapshot : snapshots) {
- isReady &= (Arrays.asList(validStates).contains(snapshot.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(snapshot.getState()));
+ LOGGER.trace(String.format("template state: %s", template.getState()));
}
List<VolumeDataStoreVO> volumes = volumeDataStoreDao.listByStoreId(srcDataStoreId);
for (VolumeDataStoreVO volume : volumes) {
- isReady &= (Arrays.asList(validStates).contains(volume.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(volume.getState()));
+ LOGGER.trace(String.format("template state: %s", template.getState()));
Review comment:
```suggestion
LOGGER.trace(String.format("volume state: %s", volume.getState()));
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006797378
<b>Trillian test result (tid-2797)</b>
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36234 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2797-xenserver-71.zip
Smoke tests completed. 91 look OK, 1 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_sys_vm_start | `Failure` | 0.11 | test_secondary_storage.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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006343626
@sureshanaparti a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1003356993
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778700552
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: ", template.getState()));
Review comment:
:facepalm: you are right :shootingmyslefinthehead:
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778693589
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: ", template.getState()));
}
List<SnapshotDataStoreVO> snapshots = snapshotDataStoreDao.listByStoreId(srcDataStoreId, DataStoreRole.Image);
for (SnapshotDataStoreVO snapshot : snapshots) {
- isReady &= (Arrays.asList(validStates).contains(snapshot.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(snapshot.getState()));
+ LOGGER.trace(String.format("snapshot state: ", snapshot.getState()));
Review comment:
```suggestion
LOGGER.trace(String.format("snapshot state: %s", snapshot.getState().toString()));
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006509770
> From what I understood, the concern raised by other reviewers was ruled out of the table. (am I right?) Code LGTM.
ruled out and/or mitigated @GabrielBrascher
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1001945506
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-989880589
I created and am testing 5760, which takes care of the failing when not all is migrated. Renaming this one.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1001945506
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-987281273
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1002240589
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778695512
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: ", template.getState()));
Review comment:
this is automatic @sureshanaparti, the State has toString() method.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1003376555
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986560168
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986902300
this solves #5655 for sure (verified) , but we'll have to split the PR to verify #5654 (as the easiest solution to set up a proper test) @sureshanaparti @Pearl1594
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778698334
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: ", template.getState()));
Review comment:
ignore toString() part, the state is not logged in the trace, check other similar logs 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r768315208
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -215,7 +219,20 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
return handleResponse(futures, migrationPolicy, message, success);
}
- protected Pair<String, Boolean> migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List<DataObject> files, MigrationPolicy migrationPolicy) {
+ private String getObjectType(DataObject dataObject) {
+ if (dataObject instanceof VolumeInfo) {
+ return "volume";
Review comment:
Can return string from enum types here:
https://github.com/apache/cloudstack/blob/51d4e5475f3a461b7f1aef39aded5e09773759dd/api/src/main/java/com/cloud/agent/api/to/DataObjectType.java#L22
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r773871451
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -215,7 +219,20 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
return handleResponse(futures, migrationPolicy, message, success);
}
- protected Pair<String, Boolean> migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List<DataObject> files, MigrationPolicy migrationPolicy) {
+ private String getObjectType(DataObject dataObject) {
+ if (dataObject instanceof VolumeInfo) {
+ return "volume";
Review comment:
wouldn't dataObject.getType().getName() make more sense? and do away with the stacked if-statements completely, @sureshanaparti and @Pearl1594 ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1003357525
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r762889878
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -35,6 +35,7 @@
import javax.inject.Inject;
import javax.naming.ConfigurationException;
+import com.cloud.storage.Volume;
Review comment:
@DaanHoogland Pkging fails due to this unused import ^^^
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986732428
@blueorangutan test matrix
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778698334
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: ", template.getState()));
Review comment:
the state is not logged in the trace, check other similar logs 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778693333
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: ", template.getState()));
Review comment:
```suggestion
LOGGER.trace(String.format("template state: %s", template.getState().toString()));
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1003376452
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778693740
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: ", template.getState()));
}
List<SnapshotDataStoreVO> snapshots = snapshotDataStoreDao.listByStoreId(srcDataStoreId, DataStoreRole.Image);
for (SnapshotDataStoreVO snapshot : snapshots) {
- isReady &= (Arrays.asList(validStates).contains(snapshot.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(snapshot.getState()));
+ LOGGER.trace(String.format("snapshot state: ", snapshot.getState()));
}
List<VolumeDataStoreVO> volumes = volumeDataStoreDao.listByStoreId(srcDataStoreId);
for (VolumeDataStoreVO volume : volumes) {
- isReady &= (Arrays.asList(validStates).contains(volume.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(volume.getState()));
+ LOGGER.trace(String.format("volume state: ", volume.getState()));
Review comment:
```suggestion
LOGGER.trace(String.format("volume state: %s", volume.getState().toString()));
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778685136
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -194,8 +197,9 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
destDatastoreId = orderedDS.get(1);
}
- if (chosenFileForMigration.getSize() > storageCapacities.get(destDatastoreId).first()) {
- s_logger.debug("file: " + chosenFileForMigration.getId() + " too large to be migrated to " + destDatastoreId);
+ if (chosenFileForMigration.getPhysicalSize() > storageCapacities.get(destDatastoreId).first()) {
+ s_logger.debug(String.format("%s: %s too large to be migrated to %s", getObjectType(chosenFileForMigration), chosenFileForMigration.getUuid(), destDatastoreId));
+ skipped += 1;
continue;
Review comment:
I tried moving one of the child templates of a multi-disk template to another store and modified the store_id details in the template_store_ref table and noticed a failure during deletion of the template, where it complained about not being able to find the template. cc @DaanHoogland @nvazquez
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r773873424
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -194,8 +197,9 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
destDatastoreId = orderedDS.get(1);
}
- if (chosenFileForMigration.getSize() > storageCapacities.get(destDatastoreId).first()) {
- s_logger.debug("file: " + chosenFileForMigration.getId() + " too large to be migrated to " + destDatastoreId);
+ if (chosenFileForMigration.getPhysicalSize() > storageCapacities.get(destDatastoreId).first()) {
+ s_logger.debug(String.format("%s: %s too large to be migrated to %s", getObjectType(chosenFileForMigration), chosenFileForMigration.getUuid(), destDatastoreId));
+ skipped += 1;
continue;
Review comment:
I wouldn't know for sure, but I think not. templates are gotten from their store and copied to primary for usage. no reason for a multi-disk template to be on one store asfaics @Pearl1594 @sureshanaparti .
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1003356993
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1001994783
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2032
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1003366976
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006322621
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1005688392
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2094
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-987227570
<b>Trillian test result (tid-2625)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30252 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2625-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986733047
@sureshanaparti a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-987300034
<b>Trillian test result (tid-2624)</b>
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34421 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2624-xenserver-71.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778700958
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: ", template.getState()));
Review comment:
```suggestion
LOGGER.trace(String.format("template state: %s", template.getState()));
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778896975
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -194,8 +197,9 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
destDatastoreId = orderedDS.get(1);
}
- if (chosenFileForMigration.getSize() > storageCapacities.get(destDatastoreId).first()) {
- s_logger.debug("file: " + chosenFileForMigration.getId() + " too large to be migrated to " + destDatastoreId);
+ if (chosenFileForMigration.getPhysicalSize() > storageCapacities.get(destDatastoreId).first()) {
+ s_logger.debug(String.format("%s: %s too large to be migrated to %s", getObjectType(chosenFileForMigration), chosenFileForMigration.getUuid(), destDatastoreId));
+ skipped += 1;
continue;
Review comment:
@Pearl1594 I've tested with your last patch and indeed all mulidisk parts go to the same store (migrating a store with four multidisk templates to two destinations, the load gets balanced but templates don't get split)
cc @nvazquez @sureshanaparti
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006797259
<b>Trillian test result (tid-2799)</b>
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36223 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2799-vmware-65u2.zip
Smoke tests completed. 89 look OK, 3 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_DeployVmAntiAffinityGroup | `Error` | 46.94 | test_affinity_groups.py
test_DeployVmAntiAffinityGroup_in_project | `Error` | 112.96 | test_affinity_groups_projects.py
test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 41.14 | test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r768331931
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -194,8 +197,9 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
destDatastoreId = orderedDS.get(1);
}
- if (chosenFileForMigration.getSize() > storageCapacities.get(destDatastoreId).first()) {
- s_logger.debug("file: " + chosenFileForMigration.getId() + " too large to be migrated to " + destDatastoreId);
+ if (chosenFileForMigration.getPhysicalSize() > storageCapacities.get(destDatastoreId).first()) {
+ s_logger.debug(String.format("%s: %s too large to be migrated to %s", getObjectType(chosenFileForMigration), chosenFileForMigration.getUuid(), destDatastoreId));
+ skipped += 1;
continue;
Review comment:
You are right @sureshanaparti cc @DaanHoogland, we have a utility method getFileSize(), that gets the cumulative size. Since we are on that topic, I do have a doubt now, currently only snapshot chains are migrated together, is there any constraint on templates too, i.e., do multi-disk templates also need to go to the same store?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1001945898
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006343028
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2105
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778706206
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: %s", template.getState()));
}
List<SnapshotDataStoreVO> snapshots = snapshotDataStoreDao.listByStoreId(srcDataStoreId, DataStoreRole.Image);
for (SnapshotDataStoreVO snapshot : snapshots) {
- isReady &= (Arrays.asList(validStates).contains(snapshot.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(snapshot.getState()));
+ LOGGER.trace(String.format("template state: %s", template.getState()));
Review comment:
```suggestion
LOGGER.trace(String.format("snapshot state: %s", snapshot.getState()));
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1005866758
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006754837
<b>Trillian test result (tid-2798)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32981 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2798-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1003376555
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986559901
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986687722
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986568582
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986733047
@sureshanaparti a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r774309466
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -215,7 +219,20 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
return handleResponse(futures, migrationPolicy, message, success);
}
- protected Pair<String, Boolean> migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List<DataObject> files, MigrationPolicy migrationPolicy) {
+ private String getObjectType(DataObject dataObject) {
+ if (dataObject instanceof VolumeInfo) {
+ return "volume";
Review comment:
> wouldn't dataObject.getType().getName() make more sense? and do away with the stacked if-statements completely, @sureshanaparti and @Pearl1594 ?
+1 @DaanHoogland
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1004659800
@sureshanaparti I had some trouble testing this because of faulty states in template_store_ref that might need attention, however the functionality here all works. cc @Pearl1594 @nvazquez
The problem was that a systemtemplate had three entries in the table two of which were "Migrating" which seems like an unrelated issue as it was before any test from that store was done. before I had succesfully migrated amongst two other stores and to the one at fault without problems.
I think we are ready to merge, given enough review.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1005006306
<b>Trillian test result (tid-2777)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31119 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2777-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1004596861
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986559901
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986686997
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986719858
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1841
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1002240589
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986732428
@blueorangutan test matrix
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1001945898
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986687722
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti merged pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti merged pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778921363
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -194,8 +197,9 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
destDatastoreId = orderedDS.get(1);
}
- if (chosenFileForMigration.getSize() > storageCapacities.get(destDatastoreId).first()) {
- s_logger.debug("file: " + chosenFileForMigration.getId() + " too large to be migrated to " + destDatastoreId);
+ if (chosenFileForMigration.getPhysicalSize() > storageCapacities.get(destDatastoreId).first()) {
+ s_logger.debug(String.format("%s: %s too large to be migrated to %s", getObjectType(chosenFileForMigration), chosenFileForMigration.getUuid(), destDatastoreId));
+ skipped += 1;
continue;
Review comment:
Thanks for testing @DaanHoogland.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r768311812
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -233,6 +250,10 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
}
} else {
message = "Migration completed";
+ if (migrationPolicy == MigrationPolicy.COMPLETE && skipped != 0) {
Review comment:
```suggestion
if (migrationPolicy == MigrationPolicy.COMPLETE && skipped > 0) {
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-992328586
#5760 tested successfully, let's merge
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1003366976
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_multiplication_x: suse15. SL-JID 2056
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1004384890
Packaging result: :heavy_multiplication_x: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2077
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1004432627
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2078
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1004596861
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1004597422
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006300000
> Trillian test result (tid-2794) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 33518 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2794-kvm-centos7.zip Smoke tests completed. 80 look OK, 0 have errors Only failed tests results shown below:
> Test Result Time (s) Test File
LGTM , but in the last test run I see only 80 tests have run as opposed to 91 - which seems a bit odd.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-986568582
Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1840
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: fail 'COMPLETE' Migration if not all files where moved.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-987281273
<b>Trillian test result (tid-2626)</b>
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33567 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2626-vmware-65u2.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r775791908
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -194,8 +197,9 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
destDatastoreId = orderedDS.get(1);
}
- if (chosenFileForMigration.getSize() > storageCapacities.get(destDatastoreId).first()) {
- s_logger.debug("file: " + chosenFileForMigration.getId() + " too large to be migrated to " + destDatastoreId);
+ if (chosenFileForMigration.getPhysicalSize() > storageCapacities.get(destDatastoreId).first()) {
+ s_logger.debug(String.format("%s: %s too large to be migrated to %s", getObjectType(chosenFileForMigration), chosenFileForMigration.getUuid(), destDatastoreId));
+ skipped += 1;
continue;
Review comment:
@nvazquez can you comment on this discussion, please?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r768318332
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -194,8 +197,9 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
destDatastoreId = orderedDS.get(1);
}
- if (chosenFileForMigration.getSize() > storageCapacities.get(destDatastoreId).first()) {
- s_logger.debug("file: " + chosenFileForMigration.getId() + " too large to be migrated to " + destDatastoreId);
+ if (chosenFileForMigration.getPhysicalSize() > storageCapacities.get(destDatastoreId).first()) {
+ s_logger.debug(String.format("%s: %s too large to be migrated to %s", getObjectType(chosenFileForMigration), chosenFileForMigration.getUuid(), destDatastoreId));
+ skipped += 1;
continue;
Review comment:
Does the files migrated have any relation (eg. template info and template file)? Is it ok to migrate other files, if one large file is skipped in a group of related files? or Is it better to check with the cumulative size of all the related files?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r773885629
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -215,7 +219,20 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
return handleResponse(futures, migrationPolicy, message, success);
}
- protected Pair<String, Boolean> migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List<DataObject> files, MigrationPolicy migrationPolicy) {
+ private String getObjectType(DataObject dataObject) {
+ if (dataObject instanceof VolumeInfo) {
+ return "volume";
Review comment:
Yes @DaanHoogland - agree
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1002240762
@sureshanaparti 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1003376452
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1003460240
<b>Trillian test result (tid-2761)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31247 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2761-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
nvazquez commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r777467778
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java
##########
@@ -194,8 +197,9 @@ public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatasto
destDatastoreId = orderedDS.get(1);
}
- if (chosenFileForMigration.getSize() > storageCapacities.get(destDatastoreId).first()) {
- s_logger.debug("file: " + chosenFileForMigration.getId() + " too large to be migrated to " + destDatastoreId);
+ if (chosenFileForMigration.getPhysicalSize() > storageCapacities.get(destDatastoreId).first()) {
+ s_logger.debug(String.format("%s: %s too large to be migrated to %s", getObjectType(chosenFileForMigration), chosenFileForMigration.getUuid(), destDatastoreId));
+ skipped += 1;
continue;
Review comment:
Afaik multi-disk templates wouldn't necessarily need to go to the same store
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1002390906
<b>Trillian test result (tid-2749)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32248 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2749-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006514201
Merging based on approvals, smoke test results and tests performed by author.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006322813
@sureshanaparti 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#discussion_r778701420
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: ", template.getState()));
}
List<SnapshotDataStoreVO> snapshots = snapshotDataStoreDao.listByStoreId(srcDataStoreId, DataStoreRole.Image);
for (SnapshotDataStoreVO snapshot : snapshots) {
- isReady &= (Arrays.asList(validStates).contains(snapshot.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(snapshot.getState()));
+ LOGGER.trace(String.format("snapshot state: ", snapshot.getState()));
}
List<VolumeDataStoreVO> volumes = volumeDataStoreDao.listByStoreId(srcDataStoreId);
for (VolumeDataStoreVO volume : volumes) {
- isReady &= (Arrays.asList(validStates).contains(volume.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(volume.getState()));
+ LOGGER.trace(String.format("volume state: ", volume.getState()));
Review comment:
```suggestion
LOGGER.trace(String.format("template state: %s", template.getState()));
```
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java
##########
@@ -87,19 +92,22 @@
* the migration is terminated.
*/
private boolean filesReadyToMigrate(Long srcDataStoreId) {
- String[] validStates = new String[]{"Ready", "Allocated", "Destroying", "Destroyed", "Failed"};
+ State[] validStates = {State.Ready, State.Allocated, State.Destroying, State.Destroyed, State.Failed};
boolean isReady = true;
List<TemplateDataStoreVO> templates = templateDataStoreDao.listByStoreId(srcDataStoreId);
for (TemplateDataStoreVO template : templates) {
- isReady &= (Arrays.asList(validStates).contains(template.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(template.getState()));
+ LOGGER.trace(String.format("template state: ", template.getState()));
}
List<SnapshotDataStoreVO> snapshots = snapshotDataStoreDao.listByStoreId(srcDataStoreId, DataStoreRole.Image);
for (SnapshotDataStoreVO snapshot : snapshots) {
- isReady &= (Arrays.asList(validStates).contains(snapshot.getState().toString()));
+ isReady &= (Arrays.asList(validStates).contains(snapshot.getState()));
+ LOGGER.trace(String.format("snapshot state: ", snapshot.getState()));
Review comment:
```suggestion
LOGGER.trace(String.format("template state: %s", template.getState()));
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1004597422
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1006223200
<b>Trillian test result (tid-2794)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33518 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2794-kvm-centos7.zip
Smoke tests completed. 80 look OK, 0 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1005006306
<b>Trillian test result (tid-2777)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31119 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5750-t2777-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1005867645
@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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #5750: use physical size instead of virtual size for migration.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #5750:
URL: https://github.com/apache/cloudstack/pull/5750#issuecomment-1004432627
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2078
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org