You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/05/11 12:20:53 UTC
[GitHub] [cloudstack] ravening opened a new pull request #4078: CLSTACK-7189 - Cleanup download urls when SSVM destroyed
ravening opened a new pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078
## Description
1. When SSVM is destroyed the downloaded urls of
template and volumes are not cleaned and because
of that we cant download the templates/volumes
using the download_url link
2. cleanup volumes in secondary storage when ssvm is destroyed
3. It should clean up the download urls for the templates belonging to the zone where ssvm is running
Steps to test
Before the fix
(1) download template (check download_url in template_store_ref table).
(2) destroy ssvm
(3) download template again
(4) It will fail to download
After the fix
1. Download template
2. Destroy ssvm
3. This will cleanup the download_url in template_store_ref table
4. Try to download template again
5. Download will be successful
Test case 2
Before the fix
1. create a vm
2. stop a vm
3. download volume
Volume stored in
https://10.1.1.2/userdata/2dc15926-eaa4-49f8-9d3c-e71bdbe9d39f.qcow2
4. start/stop ssvm
5. start/stop vm
6. download volume
Volume stored in
https://10.1.1.2/userdata/2dc15926-eaa4-49f8-9d3c-e71bdbe9d39f.qcow2
Expected result
volumes in step3 and step 5 are different
Actual result
volumes in step 3 and 5 are same.
After the fix
volumes in step 3 and 5 are different
step 3: http://10.11.1.2/userdata/7237c056-2a45-426b-b12e-13da9adab644.qcow2
Step 5: http://10.11.1.2/userdata/9da05f2a-2abc-42b8-9adc-c0e41f36f0c4.qcow2
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [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)
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-684740723
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-717935827
@ravening can you answer @rhtyd 's question
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r426501969
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3237,13 +3245,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
Review comment:
Yes but I didnt pass it purposefully because in case in future we need to do further processing with the systemvm, we will have entire data. If I just pass data center id, we dont have reference to systemvm
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718044253
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-729478108
> > @ravening can you deal with the comment or should we move to a later milestone?
>
> @DaanHoogland there is no need for functional change. It's a different opinion on coding style.
I am not talking about extracting that block of code, I asked you to. Both @rhtyd and @sureshanaparti raised concerns.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r524126130
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3237,13 +3245,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
Review comment:
ping @ravening. Can you answer @sureshanaparti here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-694923730
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4078: CLSTACK-7189 - Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r425099692
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3237,13 +3245,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
Review comment:
@ravening DatacenterId should be ok here, to cleanup urls as per the method implementation. But, in case of multiple SSVMs in the zone, this would delete all the download urls in that zone.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r525988804
##########
File path: services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
##########
@@ -744,9 +743,6 @@ private void allocCapacity(long dataCenterId, SecondaryStorageVm.Role role) {
if (_allocLock.lock(ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_SYNC)) {
try {
secStorageVm = startNew(dataCenterId, role);
- for (UploadVO upload : _uploadDao.listAll()) {
Review comment:
@rhtyd @DaanHoogland I tested by uploading volumes from local as well as from url but `upload` tables were still empty. So looks like uploaded volume/template entries are not stored in this table.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-689535605
Packaging result: ✔centos7 ✔centos8 ✖debian. JID-1939
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-690418467
<b>Trillian test result (tid-2713)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 58784 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4078-t2713-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 81 look OK, 4 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_add_delete_kubernetes_supported_version | `Error` | 126.09 | test_kubernetes_supported_versions.py
test_03_vpc_privategw_restart_vpc_cleanup | `Failure` | 333.03 | test_privategw_acl.py
test_04_rvpc_privategw_static_routes | `Failure` | 414.73 | test_privategw_acl.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 498.10 | test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 429.57 | test_vpc_redundant.py
test_hostha_kvm_host_fencing | `Error` | 205.26 | test_hostha_kvm.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-653730801
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718045057
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-697142155
Test LGTM, this is ready for merge but I've not tested this. @ravening can you explain the one comment I've left? Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-665057020
@sureshanaparti do you approve the changes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-695811065
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-653810726
<b>Trillian test result (tid-1979)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45555 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4078-t1979-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Smoke tests completed. 83 look OK, 0 have error(s)
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-729643872
> > > > > > @ravening can you deal with the comment or should we move to a later milestone?
> > > > >
> > > > >
> > > > > @DaanHoogland there is no need for functional change. It's a different opinion on coding style.
> > > >
> > > >
> > > > I am not talking about extracting that block of code, I asked you to. Both @rhtyd and @sureshanaparti raised concerns.
> > >
> > >
> > > @DaanHoogland will look into it today
> >
> >
> > while you're at it, can you extract that code block, please?
>
> @DaanHoogland @sureshanaparti made changes. Please review them again
changes LGTM. @ravening can you respond to the comment on multiple SSVMs scenario
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-717834591
@ravening can you address conflicts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-717936296
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r514088345
##########
File path: services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
##########
@@ -744,9 +743,6 @@ private void allocCapacity(long dataCenterId, SecondaryStorageVm.Role role) {
if (_allocLock.lock(ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_SYNC)) {
try {
secStorageVm = startNew(dataCenterId, role);
- for (UploadVO upload : _uploadDao.listAll()) {
Review comment:
@ravening can you answer this question, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-695811223
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4078: CLSTACK-7189 - Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r425099692
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3237,13 +3245,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
Review comment:
@ravening DatacenterId should be ok here, to cleanup urls as per the method implementation. But, in case of multiple SSVMs in the zone, this would delete all the download urls in that zone. May be better to cleanup by SSVM's public ip in that zone.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-717981706
Packaging result: ✖centos7 ✖centos8 ✔debian. JID-2300
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r526046675
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3286,13 +3294,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
+ // clean download URLs when destroying ssvm
+ // clean only the volumes and templates of the zone
+ // to which ssvm belongs to
+ for (VolumeDataStoreVO volume :_volumeStoreDao.listVolumeDownloadUrlsByZoneId(systemVm.getDataCenterId())) {
Review comment:
@sureshanaparti yes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718045057
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-729599393
> > > > > @ravening can you deal with the comment or should we move to a later milestone?
> > > >
> > > >
> > > > @DaanHoogland there is no need for functional change. It's a different opinion on coding style.
> > >
> > >
> > > I am not talking about extracting that block of code, I asked you to. Both @rhtyd and @sureshanaparti raised concerns.
> >
> >
> > @DaanHoogland will look into it today
>
> while you're at it, can you extract that code block, please?
@DaanHoogland @sureshanaparti made changes. Please review them again
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r524145235
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3237,13 +3245,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
Review comment:
@ravening the download urls are related to zone, so I recommend to go with zone id, which is relavent. Passing _systemVm_ (VMInstanceVO of SSVM) is irrelevant here. Not sure what will be the future double work you are pointing to.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-653730876
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-630109012
> So when we destroy the SSVM, we clean up the download_url link so that it will generate a new download_url link when template/volume is downloaded again.
right, I was confusing `vm_template.url` with `template_store_ref.download_url`
totally sensible fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-689500169
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718029256
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2302
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-729144571
> @ravening can you deal with the comment or should we move to a later milestone?
@DaanHoogland there is no need for functional change. It's a different opinion on coding style.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-689870436
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r524136266
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3237,13 +3245,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
Review comment:
> ping @ravening , please update on this. thanks.
@sureshanaparti i would rather prefer to keep the method definition as is rather than changing it again future which I double work
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-684743509
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r524126188
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3237,13 +3245,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
Review comment:
ping @ravening , please update on this. thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-687650182
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-694951845
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2046
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-717936296
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718005312
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-684593568
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r524004638
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3237,13 +3245,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
Review comment:
@ravening in case, any data is required for further processing in future in this method, the method definition/implementation can be updated based on the requirement in future. As the datacenter is only being used to cleanup the urls, so better use the datacenter id for the cleanup purpose here, and update the method name accordingly (suggestion below).
```suggestion
private void cleanupDownloadUrlsInZone(long zoneId) {
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-694924405
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-689532960
Packaging result: ✔centos7 ✖centos8 ✖debian. JID-1937
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-650242479
@DaanHoogland @rhtyd please review it when you get time
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-653724565
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-696434834
<b>Trillian test result (tid-2809)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 51476 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4078-t2809-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_pvlan.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 82 look OK, 3 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_create_pvlan_network | `Error` | 0.04 | test_pvlan.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 371.50 | test_vpc_redundant.py
test_hostha_kvm_host_fencing | `Error` | 165.97 | test_hostha_kvm.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718156699
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-717981706
Packaging result: ✖centos7 ✖centos8 ✔debian. JID-2300
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-727867437
> @rhtyd @sureshanaparti are your concerns met?
@DaanHoogland waiting for update on cleanup method definition, expect that rest all changes LGTM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-729520097
> > > @ravening can you deal with the comment or should we move to a later milestone?
> >
> >
> > @DaanHoogland there is no need for functional change. It's a different opinion on coding style.
>
> I am not talking about extracting that block of code, I asked you to. Both @rhtyd and @sureshanaparti raised concerns.
@DaanHoogland will look into it today
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: CLSTACK-7189 - Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-628484658
@ravening, I have seen problems with re-downloads of templates and the main issue was not the URL but the checksum stored. Did you encounter this? I don't see how the URL of a template would be the problem, unless I don't understand your description.
Also CLSTACK-7189 is an internal issue for you guys isn't it? Can you create a github issue describing the issue and rename the PR? (using the hashtag-issue contruct to point to the public github issue)
finally, you are speaking of both volumes and templates in the PR but only of templates in the title. Just a remark, i'm sure the source will make it clear, but not everybody will read the source :(
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718157314
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-687655565
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-1903
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-696434834
<b>Trillian test result (tid-2809)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 51476 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4078-t2809-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_pvlan.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 82 look OK, 3 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_create_pvlan_network | `Error` | 0.04 | test_pvlan.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 371.50 | test_vpc_redundant.py
test_hostha_kvm_host_fencing | `Error` | 165.97 | test_hostha_kvm.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-689870087
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718044253
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-689501467
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-689501197
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-630071057
> @ravening, I have seen problems with re-downloads of templates and the main issue was not the URL but the checksum stored. Did you encounter this? I don't see how the URL of a template would be the problem, unless I don't understand your description.
> Also CLSTACK-7189 is an internal issue for you guys isn't it? Can you create a github issue describing the issue and rename the PR? (using the hashtag-issue contruct to point to the public github issue)
> finally, you are speaking of both volumes and templates in the PR but only of templates in the title. Just a remark, i'm sure the source will make it clear, but not everybody will read the source :(
@dahn, When we download volume/template, it generates a download_url link in the table. When we destroy the secondary storage vm, the download_url still exists in the table but it is not valid anymore as we cant download the template/volume from the same link.
So when we destroy the SSVM, we clean up the download_url link so that it will generate a new download_url link when template/volume is downloaded again.
This applies for both volumes and templates.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-653730276
Packaging result: ✔centos7 ✔debian. JID-1523
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4078: CLSTACK-7189 - Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r425099692
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3237,13 +3245,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
Review comment:
@ravening DatacenterId should be ok here, to cleanup urls.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r526027502
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3286,13 +3294,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
+ // clean download URLs when destroying ssvm
+ // clean only the volumes and templates of the zone
+ // to which ssvm belongs to
+ for (VolumeDataStoreVO volume :_volumeStoreDao.listVolumeDownloadUrlsByZoneId(systemVm.getDataCenterId())) {
Review comment:
> then the entries will be cleared.
ok @ravening, so all the zone url entries are deleted irrespective of the SSVM ip in the url, and a new url is generated whenever user/operator requests download. am i correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd closed pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd closed pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-684591718
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland merged pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718006129
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-695811223
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-724925355
@rhtyd @sureshanaparti are your concerns met?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-695811065
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-717918701
> @ravening can you address conflicts?
@rhtyd done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-684679448
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-1864
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r524157670
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3286,13 +3294,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
+ // clean download URLs when destroying ssvm
+ // clean only the volumes and templates of the zone
+ // to which ssvm belongs to
+ for (VolumeDataStoreVO volume :_volumeStoreDao.listVolumeDownloadUrlsByZoneId(systemVm.getDataCenterId())) {
Review comment:
@ravening What if when one of the SSVM is destroyed (multiple SSVMs are created for a zone), which is created to meet the storage command sessions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-687650357
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r461600605
##########
File path: services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java
##########
@@ -307,12 +307,15 @@ public Answer handleDeleteEntityDownloadURLCommand(DeleteEntityDownloadURLComman
//We just need to remove the UUID.vhd
String extractUrl = cmd.getExtractUrl();
- command.add("unlink /var/www/html/userdata/" + extractUrl.substring(extractUrl.lastIndexOf(File.separator) + 1));
- String result = command.execute();
- if (result != null) {
- // FIXME - Ideally should bail out if you cant delete symlink. Not doing it right now.
- // This is because the ssvm might already be destroyed and the symlinks do not exist.
- s_logger.warn("Error in deleting symlink :" + result);
+ String result;
+ if (extractUrl != null) {
+ command.add("unlink /var/www/html/userdata/" + extractUrl.substring(extractUrl.lastIndexOf(File.separator) + 1));
+ result = command.execute();
+ if (result != null) {
+ // FIXME - Ideally should bail out if you cant delete symlink. Not doing it right now.
+ // This is because the ssvm might already be destroyed and the symlinks do not exist.
+ s_logger.warn("Error in deleting symlink :" + result);
Review comment:
hm, that would be an easy check, wonder if that is all.
##########
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##########
@@ -2698,9 +2701,40 @@ public String extractVolume(ExtractVolumeCmd cmd) {
}
// Check if the url already exists
- VolumeDataStoreVO volumeStoreRef = _volumeStoreDao.findByVolume(volumeId);
+ SearchCriteria<VolumeDataStoreVO> sc = _volumeStoreDao.createSearchCriteria();
+ sc.addAnd("state", SearchCriteria.Op.EQ, ObjectInDataStoreStateMachine.State.Ready.toString());
+ sc.addAnd("volumeId", SearchCriteria.Op.EQ, volumeId);
+ sc.addAnd("destroyed", SearchCriteria.Op.EQ, false);
+ // the volume should not change (attached/detached, vm not updated) after created
+ if (volume.getVolumeType() == Volume.Type.ROOT) { // for ROOT disk
+ VMInstanceVO vm = _vmInstanceDao.findById(volume.getInstanceId());
+ sc.addAnd("updated", SearchCriteria.Op.GTEQ, vm.getUpdateTime());
+ } else if (volume.getVolumeType() == Volume.Type.DATADISK && volume.getInstanceId() == null) { // for not attached DATADISK
+ sc.addAnd("updated", SearchCriteria.Op.GTEQ, volume.getUpdated());
+ } else { // for attached DATA DISK
+ VMInstanceVO vm = _vmInstanceDao.findById(volume.getInstanceId());
+ sc.addAnd("updated", SearchCriteria.Op.GTEQ, vm.getUpdateTime());
+ sc.addAnd("updated", SearchCriteria.Op.GTEQ, volume.getUpdated());
+ }
+ Filter filter = new Filter(VolumeDataStoreVO.class, "created", false, 0L, 1L);
+ List<VolumeDataStoreVO> volumeStoreRefs = _volumeStoreDao.search(sc, filter);
+ VolumeDataStoreVO volumeStoreRef = null;
+ if (volumeStoreRefs != null && !volumeStoreRefs.isEmpty()) {
+ volumeStoreRef = volumeStoreRefs.get(0);
+ }
if (volumeStoreRef != null && volumeStoreRef.getExtractUrl() != null) {
return volumeStoreRef.getExtractUrl();
+ } else if (volumeStoreRef != null) {
+ s_logger.debug("volume " + volumeId + " is already installed on secondary storage, install path is " +
+ volumeStoreRef.getInstallPath());
+ ImageStoreEntity secStore = (ImageStoreEntity) dataStoreMgr.getDataStore(volumeStoreRef.getDataStoreId(), DataStoreRole.Image);
+ String extractUrl = secStore.createEntityExtractUrl(volumeStoreRef.getInstallPath(), volume.getFormat(), null);
+ volumeStoreRef = _volumeStoreDao.findByVolume(volumeId);
+ volumeStoreRef.setExtractUrl(extractUrl);
+ volumeStoreRef.setExtractUrlCreated(DateUtil.now());
+ _volumeStoreDao.update(volumeStoreRef.getId(), volumeStoreRef);
+ return extractUrl;
Review comment:
can you extract these two blocks in methods with good descriptive names 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r525990535
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3286,13 +3294,29 @@ protected void runInContext() {
}
}
+ private void cleanupDownloadUrls(VMInstanceVO systemVm) {
+ // clean download URLs when destroying ssvm
+ // clean only the volumes and templates of the zone
+ // to which ssvm belongs to
+ for (VolumeDataStoreVO volume :_volumeStoreDao.listVolumeDownloadUrlsByZoneId(systemVm.getDataCenterId())) {
Review comment:
then the entries will be cleared.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-729530067
> > > > @ravening can you deal with the comment or should we move to a later milestone?
> > >
> > >
> > > @DaanHoogland there is no need for functional change. It's a different opinion on coding style.
> >
> >
> > I am not talking about extracting that block of code, I asked you to. Both @rhtyd and @sureshanaparti raised concerns.
>
> @DaanHoogland will look into it today
while you're at it, can you extract that code block, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718363811
<b>Trillian test result (tid-3103)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33175 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4078-t3103-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_03_deploy_and_upgrade_kubernetes_cluster | `Failure` | 249.53 | 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-718005312
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on a change in pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#discussion_r493205410
##########
File path: services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
##########
@@ -744,9 +743,6 @@ private void allocCapacity(long dataCenterId, SecondaryStorageVm.Role role) {
if (_allocLock.lock(ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_SYNC)) {
try {
secStorageVm = startNew(dataCenterId, role);
- for (UploadVO upload : _uploadDao.listAll()) {
Review comment:
@ravening would that cause a regression, to not expunge uploaded volumes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-729143729
@ravening can you deal with the comment or should we move to a later milestone?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4078: Cleanup download urls when SSVM destroyed
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4078:
URL: https://github.com/apache/cloudstack/pull/4078#issuecomment-653724455
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org