CLOUDSTACK-9607: Preventing template deletion when template is in use. #1773
Conversation
There was a problem hiding this comment.
@priyankparihar great small enhancement to ensure the overall system integrity. This seems like it would be a very good addition for LTS users to avoid deleting in-use templates. Therefore, could you please change the base branch of the PR to 4.9?
Also, are their existing Marvin test cases that test attempts to delete in-use templates? If not, could you please create one to verify this PR?
| } | ||
|
|
||
| s_logger.warn(s.substring(0,s.length()-2)); | ||
| throw new InvalidParameterValueException(s.substring(0,s.length()-2)); |
There was a problem hiding this comment.
Lines 1187-1193 should replaced with the following to be DRY and improve clarity:
final String message = String.format("Unable to delete template with id: %1$s because VM instances: [%2$s] are using it.", templateId, Joiner.on(",").join(vmInstanceVOList));
s_logger.warn(message);
throw new InvalidParameterValueException(message);| @@ -52,6 +52,9 @@ | |||
| @Parameter(name = ApiConstants.ZONE_ID, type = CommandType.UUID, entityType = ZoneResponse.class, description = "the ID of zone of the template") | |||
| private Long zoneId; | |||
|
|
|||
| @Parameter(name = ApiConstants.FORCED, type = CommandType.BOOLEAN, required = false, description = "Force delete a template.") | |||
There was a problem hiding this comment.
Please add since to this annotation to indicate that this parameter is available in 4.9+.
priyankparihar
force-pushed
the
CLOUDSTACK-9607
branch
from
cd367dc
to
cb4dd81
Compare
|
Hi @jburwell , |
priyankparihar
force-pushed
the
CLOUDSTACK-9607
branch
from
cb4dd81
to
f460d5a
Compare
|
|
Hi @jburwell , |
|
@priyankparihar I agree with @ustcweizhou . Default behavior should remain forced. And in this case Web UI we should give a warning with "yes and no" if template has deployed VM so users can cancel the operation if they need to. |
|
@priyankparihar @ustcweizhou There seems to be just one case where it will affect resetVM operations. If template is deleted and root disk is migrated to another PS where there is no template copy at destination and the reset VM wont work. It is probably not easy to implement copying a base template from on PS to another as a part of migrate volume command. I think the right approach would be just to have a check in resetVM and if no local template copy is available then find another PS with template copy and reset root disk over there or if at that point there is no PS with template copy gracefully error out resetVM command. |
|
@serg38 yes, your approach is the best solution for now. By the way, I've implemented the copy of template between storage pools on kvm/nfs in our branch, not implemented for other situations yet. The missing template also impact storage migration (live/offline). |
|
I agree with @serg38 and @ustcweizhou that upon deletion user should be displayed with an yes/no dialogue if there are deployed VMs, but a lot of CS user aren't using UI, but just the APIs. If the force option is enabled by default I guess the API call will go and delete the all selected templates without any warning. I think the |
|
Hi @ustcweizhou @serg38 and @borisroman,
If the force option is enabled by default API call will go and delete the all selected templates without any warning so default value of forced is kept 'false'.
I think it will be good to take it in separate PR. |
|
@priyankparihar Then we need to notify hundreds of users to modify their application to add forced=true in deleteTemplate API (Otherwise their application might fail), which is not feasible I think. |
|
Hi @ustcweizhou, |
|
@priyankparihar the default behaviour is forced=true from few years ago. If the template is removed, the user vm is not impacted which is very good. There is indeed an issue when reinstall the vm. For you it could be a very risky issue. For us at least it is not a big issue as we provide some other templates for vm reinstallation. |
|
@ustcweizhou @priyankparihar @borisstoyanov The default behavior was always equivalent of forced=true. We seem to agree that the UI behavior should remain forced=true but with extra warning. We can either change default confirmation "Please confirm that you want to delete this template." e.g. to "Please confirm that you want to delete this template. Features that depends on the template presence might no longer work". But I think the better option will be to have response tag in listTemplate call to indicate if active VMs exists or not. Then warning can be given only to such templates. |
|
@blueorangutan package |
|
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-515 |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Hi @koushik-das @rajesh-battala @devdeep ,
Should i make changes according to @serg38 and @ustcweizhou suggestion ? ( or current code LGT you, Please notify. ) |
|
@priyankparihar I agree with @ustcweizhou regarding the default value of Also, why we permit deletion of a template when it is associated with one or more active volumes? It seems like we are giving the user the means to corrupt their system. |
|
Trillian test result (tid-874)
|
|
Trillian test result (tid-875)
|
|
@jburwell That was default behavior for few years to allow deletion of the template even if active VMs exist. Deletion of the template on secondary doesn't remove the template copy on primary storage so all existing VM function work just fine. |
|
I think changing the default API behaviour for fixing a bug should be ok as long as documented properly. Also in this case the deletion criteria is made more strict, so there is no unwanted data loss. |
priyankparihar
force-pushed
the
CLOUDSTACK-9607
branch
from
f460d5a
to
6df0207
Compare
|
Hi @serg38 @ustcweizhou @koushik-das @jburwell @borisstoyanov , |
|
@blueorangutan package |
|
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Thanks @priyankparihar . We verified in our environment that all is working fine including UI part. |
|
LGTM |
|
@karuturi This seems to be ready for merge (tag:mergeready). |
|
@priyankparihar This PR will break the reset VM feature. I think we should also prevent reset vm if the template is deleted. |
|
Hi @SudharmaJain,
'Force' option is introduced to solve above mentioned problem. |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✖centos6 ✖centos7 ✖debian. JID-1292 |
|
@priyankparihar fix conflicts please. |
….(+ Unit Test and UI)
priyankparihar
force-pushed
the
CLOUDSTACK-9607
branch
from
3111b3e
to
3be0639
Compare
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1498 |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1514 |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1540 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1983)
|
|
Merging this based on code review lgtms and test results. |
Habilitação da funcionalidade de _over-provisioning_ para _primary storages_ do tipo `SharedMountPoint` Closes apache#1773 See merge request scclouds/scclouds!716
For complete description please refer -> CLOUDSTACK-9607