cloudstack
cloudstack copied to clipboard
apache
Allow deletion of system VM templates
Description
Currently, ACS does not allow the deletion of system VM templates. However, it might be of interest to the operator to delete templates from older versions that are no longer in use.
A change was applied to remove the restriction on the deletion of system VM templates, allowing operators to delete such templates.
Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] build/CI
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
- [ ] Major
- [x] Minor
How Has This Been Tested?
I applied the changes and successfully deleted old system VM templates
@GaOrtiga If you update the template to USER, then you will be able to remove it. this will prevent some human mistakes
Codecov Report
Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
Project coverage is 30.77%. Comparing base (
1925040) to head (d30510f). Report is 594 commits behind head on main.
| Files | Patch % | Lines |
|---|---|---|
| .../com/cloud/template/HypervisorTemplateAdapter.java | 0.00% | 1 Missing and 1 partial :warning: |
| ...k/api/command/user/template/DeleteTemplateCmd.java | 0.00% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #8556 +/- ##
============================================
- Coverage 30.82% 30.77% -0.05%
+ Complexity 34014 33967 -47
============================================
Files 5341 5341
Lines 375027 375034 +7
Branches 54554 54554
============================================
- Hits 115592 115410 -182
- Misses 244155 244363 +208
+ Partials 15280 15261 -19
| Flag | Coverage Δ | |
|---|---|---|
| simulator-marvin-tests | 24.63% <0.00%> (-0.08%) |
:arrow_down: |
| uitests | 4.39% <ø> (ø) |
|
| unit-tests | 16.49% <0.00%> (-0.01%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@GaOrtiga If you update the template to USER, then you will be able to remove it. this will prevent some human mistakes
@GaOrtiga @DaanHoogland do you agree and close this PR ?
@GaOrtiga If you update the template to USER, then you will be able to remove it. this will prevent some human mistakes
@weizhouapache I understand that there is a way to circumvent this restriction, however, since this would only be possible to Root Admins, I believe deleting it directly should still be an option. To address your concerns about this change, I could make the force flag necessary when the template's type is SYSTEM.
Would this measure be enough to offset the concerns regarding this implementation?
@GaOrtiga as a user wants this convenience. Even when I agree that it is not vital, that would be enough for it to be needed.
- Is there any more pressing objection @weizhouapache?
- Alternatively, is there a modus operandus that would make for a more acceptable change?
@GaOrtiga as a user wants this convenience. Even when I agree that it is not vital, that would be enough for it to be needed.
- Is there any more pressing objection @weizhouapache?
- Alternatively, is there a modus operandus that would make for a more acceptable change?
@DaanHoogland SYSTEM template is very important for cloudstack environments. We need to be very careful to remove it a force flag is not enough to me.
since we already have a way to delete a SYSTEM template (see https://github.com/apache/cloudstack/pull/8556#issuecomment-1906168608), this change is unnecessary.
my question to @GaOrtiga @DaanHoogland is there special use case that we must be able to delete a SYSTEM template in one step ?
@DaanHoogland @GaOrtiga let me explain a bit more users might delete the unused SYSTEM template one or twice every year by average. It takes just few minutes.
However, with this PR, they will be able to delete SYSTEM template by a single API/command. If the SYSTEM template is removed by mistake, it will be a critical issue . can you compare the pros and cons ?
@GaOrtiga @DaanHoogland sorry this PR is not needed in my opinion
Having a workaround should not be a reason to restrict an operation. If the operator does not know that you can circumvent this restriction they may feel obliged to perform the operation directly in the DB, especially since the exception thrown is very vague and editing the template's type to make it deletable is very non-intuitive.
@GaOrtiga as a user wants this convenience. Even when I agree that it is not vital, that would be enough for it to be needed.
- Is there any more pressing objection @weizhouapache?
- Alternatively, is there a modus operandus that would make for a more acceptable change?
@DaanHoogland SYSTEM template is very important for cloudstack environments. We need to be very careful to remove it a force flag is not enough to me.
since we already have a way to delete a SYSTEM template (see #8556 (comment)), this change is unnecessary.
my question to @GaOrtiga @DaanHoogland is there special use case that we must be able to delete a SYSTEM template in one step ?
The use case would be any operator that wants to delete SYSTEM templates and does not know about the workaround, since it's not really something self-explanatory.
The use case would be any operator that wants to delete
SYSTEMtemplates and does not know about the workaround, since it's not really something self-explanatory.
@GaOrtiga this can be fixed by a PR with changes in the error message.
@GaOrtiga @DaanHoogland sorry this PR is not needed in my opinion
Having a workaround should not be a reason to restrict an operation. If the operator does not know that you can circumvent this restriction they may feel obliged to perform the operation directly in the DB, especially since the exception thrown is very vague and editing the template's type to make it deletable is very non-intuitive.
If the solution is not clear, make it clear then. for example
diff --git a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
index b886f0868f6..6456ca452ed 100644
--- a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
+++ b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java
@@ -749,7 +749,7 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase {
TemplateProfile profile = super.prepareDelete(cmd);
VMTemplateVO template = profile.getTemplate();
if (template.getTemplateType() == TemplateType.SYSTEM) {
- throw new InvalidParameterValueException("The DomR template cannot be deleted.");
+ throw new InvalidParameterValueException("The SYSTEM template cannot be deleted. Please update template type to USER and retry.");
}
checkZoneImageStores(profile.getTemplate(), profile.getZoneIdList());
return profile;
What do you think of @weizhouapache 's last sugestion @GaOrtiga ? would this satisfy both your requirements?
@GaOrtiga @DaanHoogland sorry this PR is not needed in my opinion
Having a workaround should not be a reason to restrict an operation. If the operator does not know that you can circumvent this restriction they may feel obliged to perform the operation directly in the DB, especially since the exception thrown is very vague and editing the template's type to make it deletable is very non-intuitive.
If the solution is not clear, make it clear then. for example
diff --git a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java index b886f0868f6..6456ca452ed 100644 --- a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java +++ b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java @@ -749,7 +749,7 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase { TemplateProfile profile = super.prepareDelete(cmd); VMTemplateVO template = profile.getTemplate(); if (template.getTemplateType() == TemplateType.SYSTEM) { - throw new InvalidParameterValueException("The DomR template cannot be deleted."); + throw new InvalidParameterValueException("The SYSTEM template cannot be deleted. Please update template type to USER and retry."); } checkZoneImageStores(profile.getTemplate(), profile.getZoneIdList()); return profile;
@weizhouapache While the proposed implementation provides clarity, it's not intuitive. Since changes are already necessary, instead of merely guiding users through an arbitrary process, the aim should be to enhance overall code understanding and user experience.
The force flag should be more than enough to address any remaining concerns. This flag serves as a clear indicator that the operation requires careful execution and is not intended for routine use.
Besides, deleting the current SYSTEM template does not pose a significant risk to the environment, as it can be reuploaded.
@GaOrtiga @DaanHoogland sorry this PR is not needed in my opinion
Having a workaround should not be a reason to restrict an operation. If the operator does not know that you can circumvent this restriction they may feel obliged to perform the operation directly in the DB, especially since the exception thrown is very vague and editing the template's type to make it deletable is very non-intuitive.
If the solution is not clear, make it clear then. for example
diff --git a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java index b886f0868f6..6456ca452ed 100644 --- a/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java +++ b/server/src/main/java/com/cloud/template/HypervisorTemplateAdapter.java @@ -749,7 +749,7 @@ public class HypervisorTemplateAdapter extends TemplateAdapterBase { TemplateProfile profile = super.prepareDelete(cmd); VMTemplateVO template = profile.getTemplate(); if (template.getTemplateType() == TemplateType.SYSTEM) { - throw new InvalidParameterValueException("The DomR template cannot be deleted."); + throw new InvalidParameterValueException("The SYSTEM template cannot be deleted. Please update template type to USER and retry."); } checkZoneImageStores(profile.getTemplate(), profile.getZoneIdList()); return profile;@weizhouapache While the proposed implementation provides clarity, it's not intuitive. Since changes are already necessary, instead of merely guiding users through an arbitrary process, the aim should be to enhance overall code understanding and user experience.
Can you tell me how frequent you deleted the SYSTEM template?
The
forceflag should be more than enough to address any remaining concerns. This flag serves as a clear indicator that the operation requires careful execution and is not intended for routine use.
force flag is being used by other purpose.
Besides, deleting the current
SYSTEMtemplate does not pose a significant risk to the environment, as it can be reuploaded.
That is not true. Without SYSTEM template, users cannot create new networks and recreate VRs/system vms. Do you think if it is a critical issue if it happens on your production?
@weizhouapache
Can you tell me how frequent you deleted the SYSTEM template?
Even if it's not something that is done frequently, it being purposely less intuitive impacts user experience.
That is not true. Without SYSTEM template, users cannot create new networks and recreate VRs/system vms. Do you think if it is a critical issue if it happens on your production?
While it is true that the environment will not function properly without the SYSTEM template, it can be easily reuploaded as soon as the operator realizes their mistake.
force flag is being used by other purpose.
I could add a new parameter, maybe system to make it very clear on what's the intent of the flag. This would make it so that these templates can be deleted without the need for workarounds, while adding extra security. This would also make it so that unused templates can be deleted only using the system flag, with the exception of the current template being used by the systemvms, which would require the force flag aswell as the system.
@weizhouapache
Can you tell me how frequent you deleted the SYSTEM template?
Even if it's not something that is done frequently, it being purposely less intuitive impacts user experience.
We need to compare the pros and cons ... Comparing with possible unexpected removal of SYSTEM template, the 2 steps removal is a very minor issue.
That is not true. Without SYSTEM template, users cannot create new networks and recreate VRs/system vms. Do you think if it is a critical issue if it happens on your production?
While it is true that the environment will not function properly without the
SYSTEMtemplate, it can be easily reuploaded as soon as the operator realizes their mistake.
Are you operating a production ? If yes, you would not say this ...
force flag is being used by other purpose.
I could add a new parameter, maybe
systemto make it very clear on what's the intent of the flag. This would make it so that these templates can be deleted without the need for workarounds, while adding extra security. This would also make it so that unused templates can be deleted only using thesystemflag, with the exception of the current template being used by the systemvms, which would require theforceflag aswell as thesystem.
IMHO do not make things too complicated...
I could add a new parameter, maybe
systemto make it very clear on what's the intent of the flag. This would make it so that these templates can be deleted without the need for workarounds, while adding extra security. This would also make it so that unused templates can be deleted only using thesystemflag, with the exception of the current template being used by the systemvms, which would require theforceflag aswell as thesystem.
Although I think it is unnecessary to add an API parameter which reduces the steps from 2 to 1, for an operation which run once or twice a year, it looks ok to me as it does not lead to any risk. If you want to implement it, go ahead. cc @GaOrtiga
we have spent too much time on this topic.
@GaOrtiga will you support it on UI ?
API is tested ok
(localcloud) 🐱 > delete template id=d0b0a4db-1125-4a99-8cca-cf7934bbc19b
{
"account": "admin",
"accountid": "75837c93-aff0-11ee-8ad9-1e00940002db",
"cmd": "org.apache.cloudstack.api.command.user.template.DeleteTemplateCmd",
"completed": "2024-01-26T20:25:33+0000",
"created": "2024-01-26T20:25:33+0000",
"domainid": "476c9122-aff0-11ee-8ad9-1e00940002db",
"domainpath": "ROOT",
"jobid": "b9ba7dd4-58a4-4647-936a-44ca0278532f",
"jobinstanceid": "d0b0a4db-1125-4a99-8cca-cf7934bbc19b",
"jobinstancetype": "Template",
"jobprocstatus": 0,
"jobresult": {
"errorcode": 431,
"errortext": "Could not delete template as it is a SYSTEM template and isSystem is set to false."
},
"jobresultcode": 431,
"jobresulttype": "object",
"jobstatus": 2,
"userid": "7584f1d6-aff0-11ee-8ad9-1e00940002db"
}
🙈 Error: async API failed for job b9ba7dd4-58a4-4647-936a-44ca0278532f
(localcloud) 🐱 > delete template id=d0b0a4db-1125-4a99-8cca-cf7934bbc19b issystem=true
{
"success": true
}
@GaOrtiga will you support it on UI ?
@weizhouapache No I will not. In my opinion, adding it to the UI would risk some of the security concerns that you pointed out earlier in the discussion.
Also, I can not think of a use case that would require it as of now, and if one eventually comes up, we can implement it to the UI when needed.
@GaOrtiga will you support it on UI ?
@weizhouapache No I will not. In my opinion, adding it to the UI would risk some of the security concerns that you pointed out earlier in the discussion.
Also, I can not think of a use case that would require it as of now, and if one eventually comes up, we can implement it to the UI when needed.
ok @GaOrtiga so admins have to use api or cmk, right ? this does not help admins a lot.
ok @GaOrtiga so admins have to use api or cmk, right ?
Yes.
this does not help admins a lot.
Since some security concerns have been brought up, I felt that limiting it to the API would be a good compromise in order to implement this functionality while avoiding further security discussions, since, as you mentioned, we have spent too much time on this. However, I would be happy to implement this in the UI if the community sees it as beneficial.
@weizhouapache @DaanHoogland What are your opinions on this?
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8465
I think both sides of the discussion have valid points and this will have to prove itself in production. I am ok with the API only implementation.
I think both sides of the discussion have valid points and this will have to prove itself in production. I am ok with the API only implementation.
thanks @DaanHoogland I am ok with both. that's say, I am ok with this PR.
@rohityadavcloud
- Would this also deletion of systemvmtemplate current in use?
- If that's the case, can we add a check so it won't allow deleting systemvmtemplate in use
It will not allow the deletion of the current template unless both the isSystem and the forced flag are passed.
- Does it need some UI changes, so admins can delete systemvmtemplates (not in use) from the UI?
Given some of the previous discussions regarding this implementation, mainly about security, I believe this PR should focus solely on the backend. If, in the future, we decide that UI changes are beneficial, we can make a new PR to implement it.
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.