Allow deletion of system VM templates#8556
Conversation
|
@GaOrtiga |
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@GaOrtiga @DaanHoogland |
@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 Would this measure be enough to offset the concerns regarding this implementation? |
| if (template.getTemplateType() == TemplateType.SYSTEM) { | ||
| throw new InvalidParameterValueException("The DomR template cannot be deleted."); | ||
| } |
There was a problem hiding this comment.
if (template.getTemplateType() == TemplateType.SYSTEM && ! cmd.isForced()) {
throw new InvalidParameterValueException("The DomR template cannot be deleted.");
}
is what you mean @GaOrtiga ?
There was a problem hiding this comment.
Yes, and I will also update the message in the exception.
There was a problem hiding this comment.
if (template.getTemplateType() == TemplateType.SYSTEM && ! cmd.isForced()) { throw new InvalidParameterValueException("The DomR template cannot be deleted."); }is what you mean @GaOrtiga ?
@DaanHoogland @weizhouapache given that this operation can only be carried out by Root Admins, could we proceed with this proposed implementation?
|
@GaOrtiga @DaanHoogland |
|
@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.
|
@DaanHoogland since we already have a way to delete a SYSTEM template (see #8556 (comment)), this change is unnecessary. my question to @GaOrtiga @DaanHoogland |
|
@DaanHoogland @GaOrtiga However, with this PR, they will be able to delete SYSTEM template by a single API/command. |
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. |
The use case would be any operator that wants to delete |
@GaOrtiga |
If the solution is not clear, make it clear then. for example |
|
What do you think of @weizhouapache 's last sugestion @GaOrtiga ? would this satisfy both your requirements? |
@weizhouapache The Besides, deleting the current |
Can you tell me how frequent you deleted the SYSTEM template?
force flag is being used by other purpose.
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? |
Even if it's not something that is done frequently, it being purposely less intuitive impacts user experience.
While it is true that the environment will not function properly without the
I could add a new parameter, maybe |
We need to compare the pros and cons ...
Are you operating a production ? If yes, you would not say this ...
IMHO do not make things too complicated... |
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. we have spent too much time on this topic. |
|
@GaOrtiga API is tested ok |
@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. |
api/src/main/java/org/apache/cloudstack/api/command/user/template/DeleteTemplateCmd.java
Outdated
Show resolved
Hide resolved
ok @GaOrtiga |
Yes.
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? |
|
@blueorangutan package |
yadvr
left a comment
There was a problem hiding this comment.
- 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
- Does it need some UI changes, so admins can delete systemvmtemplates (not in use) from the UI?
It will not allow the deletion of the current template unless both the
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. |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8686 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-9263)
|
|
@blueorangutan package |
|
@JoaoJandre 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 9279 |
|
@DaanHoogland @sureshanaparti @rohityadavcloud @shwstppr could we run the CI here? |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-9858)
|
|
@blueorangutan package |
|
@JoaoJandre 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 10461 |
|
@blueorangutan package |
|
@JoaoJandre 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]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10638 |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10652 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-11074)
|
|
Merging based on approvals, CI result and manual tests (#8556 (comment)) |
* allow delete system VM templates * Add isSystem parameter * add authorized --------- Co-authored-by: Gabriel <gabriel.fernandes@scclouds.com.br>
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
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
I applied the changes and successfully deleted old system VM templates