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/03/04 10:34:23 UTC
[GitHub] [cloudstack] Pearl1594 opened a new pull request #4748: Update vm_template table to set template as removed on deletion
Pearl1594 opened a new pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748
### Description
Addresses: https://github.com/apache/cloudstack/issues/4470
This PR fixes the issue noticed while deleting a template in a zone and also adds cleanup, in order to remove such templates left behind. The following is currently observed currently:
In a Zone with only 1 store:
- when a template (private/public) is deleted, the template is marked as Inactive, but not removed
In a Zone with > 1 store:
- when a private template is deleted, the template is marked as Inactive, but not removed. For eg, after deletion, the following is observed:
```
MariaDB [cloud]> select id,type,name,uuid,format,public,size,state,created,removed from vm_template where id =204\G
*************************** 1. row ***************************
id: 204
type: USER
name: template-private-2storesZone
uuid: d899e386-fd23-4c8b-9834-9cac9cd6febe
format: QCOW2
public: 0
size: 52428800
state: Inactive
created: 2021-03-04 06:09:22
removed: NULL
1 row in set (0.00 sec)
MariaDB [cloud]> select id,template_id,store_id,created,state,size,store_role,install_path from template_store_ref where template_id =204\G
*************************** 1. row ***************************
id: 18
template_id: 204
store_id: 1
created: 2021-03-04 06:09:22
state: Destroyed
size: 52428800
store_role: Image
install_path: template/tmpl/2/204/1ba40914-9eba-33de-93b0-8c05ea6fc753.qcow2
1 row in set (0.01 sec)
```
- When a public template is deleted, the templates are set as destroyed on the secondary store (template_store_ref), but it continues to be marked as Active in the vm_template table. For example, On deletion:
```
MariaDB [cloud]> select id,type,name,uuid,format,public,size,state,created,removed from vm_template where id =205\G
*************************** 1. row ***************************
id: 205
type: USER
name: template-public-2storesZone
uuid: 25cbaa26-d114-46d7-8285-cb1180fb5cee
format: QCOW2
public: 1
size: 52428800
state: Active
created: 2021-03-04 06:11:52
removed: NULL
1 row in set (0.00 sec)
MariaDB [cloud]> select id,template_id,store_id,created,state,size,store_role,install_path from template_store_ref where template_id =205\G
*************************** 1. row ***************************
id: 19
template_id: 205
store_id: 2
created: 2021-03-04 06:11:52
state: Destroyed
size: 52428800
store_role: Image
install_path: template/tmpl/2/205/848ef57e-80a3-3b79-a8c9-a7ef7316165a.qcow2
*************************** 2. row ***************************
id: 20
template_id: 205
store_id: 1
created: 2021-03-04 06:11:52
state: Destroyed
size: 52428800
store_role: Image
install_path: template/tmpl/2/205/40db7f18-3d06-3527-b7f3-6db5811a3121.qcow2
2 rows in set (0.01 sec)
```
**Notice template still appears Active when destroyed on secondary store**
Post fix: On deletion of a public template registered in a zone with > 1 image store (here, 2)
```
MariaDB [cloud]> select id,type,name,uuid,format,public,size,state,created,removed from vm_template where id =208\G
*************************** 1. row ***************************
id: 208
type: USER
name: template-public-2storesPostFix
uuid: 2e60aba8-4daf-4dee-9626-d78ccaeca158
format: QCOW2
public: 1
size: 52428800
state: Inactive
created: 2021-03-04 07:14:37
removed: 2021-03-04 07:16:01
1 row in set (0.00 sec)
MariaDB [cloud]> select id,template_id,store_id,created,state,size,store_role,install_path from template_store_ref where template_id =208\G
*************************** 1. row ***************************
id: 24
template_id: 208
store_id: 1
created: 2021-03-04 07:14:37
state: Destroyed
size: 52428800
store_role: Image
install_path: template/tmpl/2/208/38b47a1b-7e92-3851-952b-99b9e898b268.qcow2
*************************** 2. row ***************************
id: 25
template_id: 208
store_id: 2
created: 2021-03-04 07:14:37
state: Destroyed
size: 52428800
store_role: Image
install_path: template/tmpl/2/208/8f30693b-c3a7-3f68-be23-dc3f0ced082e.qcow2
2 rows in set (0.00 sec)
```
With this patch, when the cleanup job is triggered, it also cleans up all templates that are marked as Inactive but not removed (reduced the cleanup interval by modifying the following global settings: `storage.cleanup.interval` and `storage.cleanup.delay`)
```
MariaDB [cloud]> select id,type,name,uuid,format,public,size,state,created,removed from vm_template where id in (202, 203, 204, 205)\G
*************************** 1. row ***************************
id: 202
type: USER
name: template-private
uuid: 195264b5-65e9-41ba-9c65-c992c5f34046
format: QCOW2
public: 0
size: 52428800
state: Inactive
created: 2021-03-04 05:47:30
removed: 2021-03-04 07:07:54
*************************** 2. row ***************************
id: 203
type: USER
name: template-public
uuid: e8039473-25c2-4254-b4fb-25b74ea6ef44
format: QCOW2
public: 1
size: 52428800
state: Inactive
created: 2021-03-04 05:58:26
removed: 2021-03-04 07:07:54
*************************** 3. row ***************************
id: 204
type: USER
name: template-private-2storesZone
uuid: d899e386-fd23-4c8b-9834-9cac9cd6febe
format: QCOW2
public: 0
size: 52428800
state: Inactive
created: 2021-03-04 06:09:22
removed: 2021-03-04 07:07:54
*************************** 4. row ***************************
id: 205
type: USER
name: template-public-2storesZone
uuid: 25cbaa26-d114-46d7-8285-cb1180fb5cee
format: QCOW2
public: 1
size: 52428800
state: Inactive
created: 2021-03-04 06:11:52
removed: 2021-03-04 07:07:54
```
Also tested a scenario where in a template can be present in other zones as well. In such a case, when a public template is destroyed in a specific zone, the template is left as Active - if it present in other zones:
```
MariaDB [cloud]> select * from template_zone_ref where template_id = 212\G
*************************** 1. row ***************************
id: 24
zone_id: 1
template_id: 212
created: 2021-03-04 07:55:08
last_updated: 2021-03-04 07:55:08
removed: 2021-03-04 07:57:11
*************************** 2. row ***************************
id: 25
zone_id: 2
template_id: 212
created: 2021-03-04 07:56:23
last_updated: 2021-03-04 07:56:23
removed: NULL
2 rows in set (0.00 sec)
MariaDB [cloud]> select id,type,name,uuid,format,public,size,state,created,removed from vm_template where id =212\G
*************************** 1. row ***************************
id: 212
type: USER
name: test-multizone
uuid: 0ef1505a-8863-4728-87fd-cbb0c8884cc2
format: QCOW2
public: 1
size: 52428800
state: Active
created: 2021-03-04 07:55:08
removed: NULL
1 row in set (0.00 sec)
MariaDB [cloud]> select id,template_id,store_id,created,state,size,store_role,install_path from template_store_ref where template_id =212\G
*************************** 1. row ***************************
id: 37
template_id: 212
store_id: 3
created: 2021-03-04 07:55:08
state: Destroyed
size: 52428800
store_role: Image
install_path: template/tmpl/2/212/18fe8ef9-565c-3cf0-b418-885bdd6ade55.qcow2
*************************** 2. row ***************************
id: 38
template_id: 212
store_id: 1
created: 2021-03-04 07:55:08
state: Destroyed
size: 52428800
store_role: Image
install_path: template/tmpl/2/212/c4fada96-bf51-356d-b47a-3efb95e71f84.qcow2
2 rows in set (0.01 sec)
```
<!-- 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)
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [X] Major
- [] Minor
- [ ] Trivial
### How Has This Been Tested?
Observations and tests mentioned above
----------------------------------------------------------------
This is an automated message from the 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 #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-802868625
Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 189
--
This is an automated message from the 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] Pearl1594 commented on a change in pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#discussion_r597695132
##########
File path: server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
##########
@@ -585,6 +595,7 @@ public boolean delete(TemplateProfile profile) {
if (iStores == null || iStores.size() == 0) {
// Mark template as Inactive.
template.setState(VirtualMachineTemplate.State.Inactive);
+ templateDao.remove(template.getId());
Review comment:
Done @DaanHoogland
##########
File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java
##########
@@ -1324,6 +1324,11 @@ public void cleanupStorage(boolean recurring) {
s_logger.warn("Unable to destroy uploaded template " + template.getUuid() + ". Error details: " + th.getMessage());
}
}
+ List<VMTemplateVO> vmTemplateVOS = _templateDao.listUnRemovedTemplatesByStates(VirtualMachineTemplate.State.Inactive);
+ for (VMTemplateVO template: vmTemplateVOS) {
+ template.setRemoved(new Date());
+ _templateDao.update(template.getId(), template);
+ }
Review comment:
done @DaanHoogland
##########
File path: server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
##########
@@ -570,6 +572,14 @@ public boolean delete(TemplateProfile profile) {
if (success) {
if ((imageStores != null && imageStores.size() > 1) && (profile.getZoneIdList() != null)) {
//if template is stored in more than one image stores, and the zone id is not null, then don't delete other templates.
+ List<VMTemplateZoneVO> templateZones = templateZoneDao.listByTemplateId(template.getId());
+ List<Long> zoneIds = templateZones.stream().map(VMTemplateZoneVO::getZoneId).collect(Collectors.toList());
+ if (zoneIds.size() > 0) {
+ return success;
+ }
+ template.setRemoved(new Date());
+ template.setState(State.Inactive);
+ templateDao.update(template.getId(), template);
Review comment:
done @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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #4748: Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-790543546
@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] Pearl1594 commented on pull request #4748: Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-790513059
@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 #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-804904041
@Pearl1594 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 #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-804411496
<b>Trillian test result (tid-205)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38360 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4748-t205-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_templates.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 83 look OK, 3 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 70.92 | test_kubernetes_clusters.py
test_03_deploy_vm_wrong_checksum | `Error` | 49.66 | test_templates.py
test_01_migrate_VM_and_root_volume | `Error` | 73.31 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 52.90 | test_vm_life_cycle.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-804879904
@Pearl1594 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 #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-805403552
<b>Trillian test result (tid-234)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38991 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4748-t234-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 84 look OK, 2 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 73.12 | test_kubernetes_clusters.py
test_01_migrate_VM_and_root_volume | `Error` | 69.15 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 51.03 | test_vm_life_cycle.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#discussion_r599561217
##########
File path: test/integration/smoke/test_templates.py
##########
@@ -1332,7 +1332,6 @@ def test_03_deploy_vm_wrong_checksum(self):
domainid=self.account.domainid,
serviceofferingid=self.service_offering.id
)
- self.cleanup.append(tmpl)
Review comment:
i'd say this, `self.cleanup.append(tmpl)` is supposed to go to line 1325,
and this line should read `self.cleanup.append(virtual_machine)`
lines 1339 and 1340 should be removed. As this code is at the moment virtual_machne will be appended to cleanup only if the create fails and `self.fail(...)` is *not* called.
a cleanup should go directly after a create (or register) and only if the remove happens for some other reason it should be removed from the list again (if a double delete would be a problem)
--
This is an automated message from the 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 #4748: Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-790544143
@Pearl1594 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 #4748: Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-790540420
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2864
----------------------------------------------------------------
This is an automated message from the 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 merged pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748
--
This is an automated message from the 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] Pearl1594 commented on pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-804903557
@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] Pearl1594 commented on pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-803884018
@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 a change in pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#discussion_r588420148
##########
File path: engine/schema/src/main/java/com/cloud/storage/dao/VMTemplateDaoImpl.java
##########
@@ -430,6 +431,11 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
ParentTemplateIdSearch.and("state", ParentTemplateIdSearch.entity().getState(), SearchCriteria.Op.EQ);
ParentTemplateIdSearch.done();
+ InactiveUnremovedTmpltSearch = createSearchBuilder();
+ InactiveUnremovedTmpltSearch.and("state", InactiveUnremovedTmpltSearch.entity().getState(), SearchCriteria.Op.IN);
+ InactiveUnremovedTmpltSearch.and("removed", InactiveUnremovedTmpltSearch.entity().getRemoved(), SearchCriteria.Op.NULL);
+ InactiveUnremovedTmpltSearch.done();
Review comment:
is this used anywhere?
----------------------------------------------------------------
This is an automated message from the 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 #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-804924283
Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 213
--
This is an automated message from the 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 #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-802850052
@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Pearl1594 commented on pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-804932429
@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 #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-804901270
Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 210
--
This is an automated message from the 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] Pearl1594 commented on pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-802849719
@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] Pearl1594 commented on pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-804879115
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#discussion_r597480536
##########
File path: server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
##########
@@ -570,6 +572,14 @@ public boolean delete(TemplateProfile profile) {
if (success) {
if ((imageStores != null && imageStores.size() > 1) && (profile.getZoneIdList() != null)) {
//if template is stored in more than one image stores, and the zone id is not null, then don't delete other templates.
+ List<VMTemplateZoneVO> templateZones = templateZoneDao.listByTemplateId(template.getId());
+ List<Long> zoneIds = templateZones.stream().map(VMTemplateZoneVO::getZoneId).collect(Collectors.toList());
+ if (zoneIds.size() > 0) {
+ return success;
+ }
+ template.setRemoved(new Date());
+ template.setState(State.Inactive);
+ templateDao.update(template.getId(), template);
Review comment:
`delete()` ranges from line 454 to line 621. It deserves its own worker class. I won't ask that (hint hint, wink wink) but can you at least extract your new bit to a separate method?
##########
File path: server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
##########
@@ -585,6 +595,7 @@ public boolean delete(TemplateProfile profile) {
if (iStores == null || iStores.size() == 0) {
// Mark template as Inactive.
template.setState(VirtualMachineTemplate.State.Inactive);
+ templateDao.remove(template.getId());
Review comment:
There's a `templateDao` member in this class and a `_tmpltDao` in `TemplateAdapterBase`, and though I like the first name better, its presence is pollution/technical debt. I think you should use the one from the based class.
##########
File path: server/src/main/java/com/cloud/storage/StorageManagerImpl.java
##########
@@ -1324,6 +1324,11 @@ public void cleanupStorage(boolean recurring) {
s_logger.warn("Unable to destroy uploaded template " + template.getUuid() + ". Error details: " + th.getMessage());
}
}
+ List<VMTemplateVO> vmTemplateVOS = _templateDao.listUnRemovedTemplatesByStates(VirtualMachineTemplate.State.Inactive);
+ for (VMTemplateVO template: vmTemplateVOS) {
+ template.setRemoved(new Date());
+ _templateDao.update(template.getId(), template);
+ }
Review comment:
Can you extract this? method already reaches from l1156-1339. ( a `class StorageCleaner` required :| )
--
This is an automated message from the 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 #4748: Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-790513885
@Pearl1594 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 #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-790980013
<b>Trillian test result (tid-3649)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36750 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4748-t3649-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_templates.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 82 look OK, 4 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_04_rvpc_privategw_static_routes | `Failure` | 237.79 | test_privategw_acl.py
ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 64.57 | test_kubernetes_clusters.py
test_03_deploy_vm_wrong_checksum | `Error` | 48.67 | test_templates.py
test_01_migrate_VM_and_root_volume | `Error` | 59.05 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 51.14 | test_vm_life_cycle.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-804933059
@Pearl1594 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 #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#issuecomment-803884886
@Pearl1594 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] Pearl1594 commented on a change in pull request #4748: Template cleanup : Update vm_template table to set template as removed on deletion
Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #4748:
URL: https://github.com/apache/cloudstack/pull/4748#discussion_r597404882
##########
File path: engine/schema/src/main/java/com/cloud/storage/dao/VMTemplateDaoImpl.java
##########
@@ -430,6 +431,11 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
ParentTemplateIdSearch.and("state", ParentTemplateIdSearch.entity().getState(), SearchCriteria.Op.EQ);
ParentTemplateIdSearch.done();
+ InactiveUnremovedTmpltSearch = createSearchBuilder();
+ InactiveUnremovedTmpltSearch.and("state", InactiveUnremovedTmpltSearch.entity().getState(), SearchCriteria.Op.IN);
+ InactiveUnremovedTmpltSearch.and("removed", InactiveUnremovedTmpltSearch.entity().getRemoved(), SearchCriteria.Op.NULL);
+ InactiveUnremovedTmpltSearch.done();
Review comment:
Thanks @DaanHoogland . It was supposed to be referenced at listUnRemovedTemplatesByStates() - fixed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org