Default value of force should be false for template delete operation #7731

harikrishna-patnala · 2023-07-07T07:09:16Z

Description

Default value of force should be false for the template delete operation

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)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

harikrishna-patnala · 2023-07-07T07:14:36Z

@blueorangutan package

blueorangutan · 2023-07-07T07:16:03Z

@harikrishna-patnala a [SF] 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.

blueorangutan · 2023-07-07T08:07:54Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6434

codecov · 2023-07-07T08:14:22Z

Codecov Report

Merging #7731 (7b3cc99) into 4.18 (29c7b31) will increase coverage by 0.00%.
Report is 68 commits behind head on 4.18.
The diff coverage is 15.37%.

@@             Coverage Diff             @@
##               4.18    #7731     +/-   ##
===========================================
  Coverage     13.02%   13.02%             
- Complexity     9032     9222    +190     
===========================================
  Files          2720     2720             
  Lines        257080   262616   +5536     
  Branches      40088    42312   +2224     
===========================================
+ Hits          33476    34211    +735     
- Misses       219400   224114   +4714     
- Partials       4204     4291     +87

Files	Coverage Δ
.../main/java/com/cloud/network/IpAddressManager.java	`100.00% <100.00%> (ø)`
...ava/com/cloud/network/as/AutoScaleVmProfileVO.java	`80.20% <100.00%> (+11.66%)`	⬆️
.../api/command/admin/ratelimit/ResetApiLimitCmd.java	`0.00% <ø> (ø)`
...ava/com/cloud/api/commands/StopNetScalerVMCmd.java	`0.00% <ø> (ø)`
...tungsten/api/command/ListTungstenFabricTagCmd.java	`0.00% <ø> (ø)`
...e/datastore/util/ListElastistorVolumeResponse.java	`0.00% <ø> (ø)`
...dstack/api/response/LdapConfigurationResponse.java	`0.00% <ø> (ø)`
...dstack/api/response/LinkAccountToLdapResponse.java	`93.33% <ø> (ø)`
...udstack/api/response/LinkDomainToLdapResponse.java	`85.71% <ø> (ø)`
...loud/api/auth/DefaultLoginAPIAuthenticatorCmd.java	`0.00% <ø> (ø)`
... and 37 more

... and 25 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DaanHoogland

clgtm

rohityadavcloud · 2023-07-07T17:29:56Z

@blueorangutan test

blueorangutan · 2023-07-07T17:32:04Z

@rohityadavcloud a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2023-07-08T05:59:26Z

[SF] Trillian test result (tid-7025)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43285 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7731-t7025-kvm-centos7.zip
Smoke tests completed. 106 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_03_deploy_vm_wrong_checksum	`Error`	40.69	test_templates.py
test_09_list_templates_download_details	`Failure`	0.05	test_templates.py
test_01_migrate_VM_and_root_volume	`Error`	79.89	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	52.63	test_vm_life_cycle.py

DaanHoogland · 2023-07-10T07:50:18Z

@harikrishna-patnala can you add some testing information to the description?

rohityadavcloud · 2023-07-10T18:04:44Z

@blueorangutan package

blueorangutan · 2023-07-10T18:06:03Z

@rohityadavcloud a [SF] 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.

blueorangutan · 2023-07-10T18:57:56Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6455

DaanHoogland · 2023-07-11T05:59:12Z

@harikrishna-patnala can you have a look if it does make sense for the test_templates tests to now fail?

harikrishna-patnala · 2023-07-14T06:11:26Z

yes @DaanHoogland it seems to be related.
'Unable to delete template with id: 267 because VM instances: [VM instance {"id":205,"instanceName":"i-119-205-VM","type":"User","uuid":"810e3e54-aeb1-444d-bb9b-9b224df2bd08"}] are using it.'

I'll fix the tests too

weizhouapache · 2023-07-21T06:15:35Z

this parameter was introduced in PR #1773

changing the default value to false makes sense. However, it will break backwards compatibility.
once this is merged, we need to add a note to to release note (e.g. in Breaking changes section)

shwstppr

Code LGTM

weizhouapache · 2023-07-25T03:00:28Z

a similar issue:
due to a new flag introduced by #4617,
ansible needs to be updated: ngine-io/ansible-collection-cloudstack#122

kiranchavala

LGTM , tested manually with the api call

Also tested with project

Before the fix

2023-08-02 10:25:41,346 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (qtp1239807799-18:ctx-6c52ee9f ctx-71da430f ctx-dbb08cbb) (logid:2828153a) submit async job-113, details: AsyncJobVO: {id:113, userId: 2, accountId: 2, instanceType: Template, instanceId: 215, cmd: org.apache.cloudstack.api.command.user.template.DeleteTemplateCmd, cmdInfo: {"apiKey":"LIN6rqXuaJwMPfGYFh13qDwYz5VNNz1J2J6qIOWcd3oLQOq0WtD4CwRundBL6rzXToa3lQOC_vKjI3nkHtiD8Q","signature":"YiZXnlLsEqPiSiiMrEBLTiSJAlo\u003d","response":"json","ctxUserId":"2","httpmethod":"GET","ctxStartEventId":"267","id":"44bb4d22-c43c-4ce5-a9c0-fcf61aeb1775","ctxDetails":"{\"interface com.cloud.template.VirtualMachineTemplate\":\"44bb4d22-c43c-4ce5-a9c0-fcf61aeb1775\"}","ctxAccountId":"2","uuid":"44bb4d22-c43c-4ce5-a9c0-fcf61aeb1775","cmdEventType":"TEMPLATE.DELETE"}, cmdVersion: 0, status: IN_PROGRESS, processStatus: 0, resultCode: 0, result: null, initMsid: 32986556793923, completeMsid: null, lastUpdated: null, lastPolled: null, created: null, removed: null}

After the fix


2023-08-02 10:21:24,306 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-41:ctx-f860df56 job-73) (logid:1cb54178) Executing AsyncJobVO: {id:73, userId: 2, accountId: 2, instanceType: Template, instanceId: 205, cmd: org.apache.cloudstack.api.command.user.template.DeleteTemplateCmd, cmdInfo: {"apiKey":"LIN6rqXuaJwMPfGYFh13qDwYz5VNNz1J2J6qIOWcd3oLQOq0WtD4CwRundBL6rzXToa3lQOC_vKjI3nkHtiD8Q","signature":"bdb7Zdkui9CZUoFAr7RMvEoIpm8\u003d","response":"json","ctxUserId":"2","httpmethod":"GET","ctxStartEventId":"161","id":"2bc0c528-55f8-4860-9b00-8f1b0f2acff8","ctxDetails":"{\"interface com.cloud.template.VirtualMachineTemplate\":\"2bc0c528-55f8-4860-9b00-8f1b0f2acff8\"}","ctxAccountId":"2","uuid":"2bc0c528-55f8-4860-9b00-8f1b0f2acff8","cmdEventType":"TEMPLATE.DELETE"}, cmdVersion: 0, status: IN_PROGRESS, processStatus: 0, resultCode: 0, result: null, initMsid: 32988351955670, completeMsid: null, lastUpdated: null, lastPolled: null, created: null, removed: null}

2023-08-02 10:21:24,306 DEBUG [c.c.a.ApiServlet] (qtp1239807799-13:ctx-a0ba720f ctx-e324f833 ctx-89287e07) (logid:9ba795bc) ===END===  10.0.3.251 -- GET  apiKey=LIN6rqXuaJwMPfGYFh13qDwYz5VNNz1J2J6qIOWcd3oLQOq0WtD4CwRundBL6rzXToa3lQOC_vKjI3nkHtiD8Q&command=deleteTemplate&id=2bc0c528-55f8-4860-9b00-8f1b0f2acff8&response=json&signature=bdb7Zdkui9CZUoFAr7RMvEoIpm8%3D

2023-08-02 10:21:24,315 WARN  [c.c.t.TemplateManagerImpl] (API-Job-Executor-41:ctx-f860df56 job-73 ctx-89c8cbb6) (logid:1cb54178) Unable to delete template with id: 205 because VM instances: [VM instance {"id":8,"instanceName":"i-2-8-VM","type":"User","uuid":"3dbd61ea-6608-4bf7-9144-ac510eec8358"}] are using it.

2023-08-02 10:21:24,320 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-41:ctx-f860df56 job-73) (logid:1cb54178) Complete async job-73, jobStatus: FAILED, resultCode: 431, result: org.apache.cloudstack.api.response.ExceptionResponse/null/{"uuidList":[],"errorcode":"431","errortext":"Unable to delete template with id: 205 because VM instances: [VM instance {"id":8,"instanceName":"i-2-8-VM","type":"User","uuid":"3dbd61ea-6608-4bf7-9144-ac510eec8358"}] are using it."

weizhouapache · 2023-08-10T13:59:09Z

@rohityadavcloud @harikrishna-patnala @DaanHoogland @shwstppr @kiranchavala
what should we proceed with this PR ?
it looks I am the only one disagree to merge this.

weizhouapache · 2023-08-15T11:56:37Z

moved to 4.18.2.0 milestone

rohityadavcloud

LGTM didn't test it

harikrishna-patnala · 2023-08-28T04:20:07Z

@blueorangutan package

blueorangutan · 2023-08-28T04:22:03Z

@harikrishna-patnala a [SF] 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.

blueorangutan · 2023-08-28T05:13:45Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6907

harikrishna-patnala · 2023-08-28T05:15:56Z

@blueorangutan test

blueorangutan · 2023-08-28T05:18:03Z

@harikrishna-patnala a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2023-08-28T16:53:41Z

[SF] Trillian test result (tid-7588)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40241 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7731-t7588-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_03_deploy_vm_wrong_checksum	`Error`	40.63	test_templates.py
test_09_list_templates_download_details	`Failure`	0.06	test_templates.py

DaanHoogland · 2023-10-02T13:50:29Z

@harikrishna-patnala any resolution on this? cc @shwstppr

DaanHoogland · 2023-10-05T13:05:34Z

this parameter was introduced in PR #1773

changing the default value to false makes sense. However, it will break backwards compatibility. once this is merged, we need to add a note to to release note (e.g. in Breaking changes section)

@shwstppr , I think we can merge this if we take @weizhouapache 's remark into account. What do you think?

shwstppr · 2023-10-06T12:39:02Z

@DaanHoogland I'm okay to merge this.
@harikrishna-patnala if you're okay please take this out of draft

harikrishna-patnala · 2023-10-09T07:35:13Z

@DaanHoogland I'm okay to merge this. @harikrishna-patnala if you're okay please take this out of draft

yes @DaanHoogland This can be merged cc @shwstppr

DaanHoogland · 2023-10-09T09:30:00Z

@harikrishna-patnala
can you add a doc PR for this please?

harikrishna-patnala · 2023-10-10T11:01:11Z

@DaanHoogland I get that I'm breaking backward compatibility, in my opinion documenting this is not needed as we are not breaking anything that is correct or expected behaviour by someone. On the other hand I also dont know where exactly to document this if you can suggest that helps please.

DaanHoogland · 2023-10-10T11:03:42Z

as @weizhouapache sugested, it should be in the release notes in the breaking changes section. humans may expect this , but tools like ansible and terraform will not.

harikrishna-patnala · 2023-10-10T11:05:27Z

ah we have a breaking changes section, nice. Thanks @DaanHoogland I'll create one.

…pache#7731) * default value of force should be false * Added force flag in tests (cherry picked from commit a9f3af8) Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

boring-cyborg bot added the component:api label Jul 7, 2023

DaanHoogland approved these changes Jul 7, 2023

View reviewed changes

rohityadavcloud approved these changes Jul 7, 2023

View reviewed changes

rohityadavcloud added this to the 4.18.1.0 milestone Jul 7, 2023

harikrishna-patnala marked this pull request as draft July 14, 2023 06:11

shwstppr approved these changes Jul 21, 2023

View reviewed changes

weizhouapache added the status:needs-review label Jul 26, 2023

kiranchavala approved these changes Aug 7, 2023

View reviewed changes

weizhouapache modified the milestones: 4.18.1.0, 4.18.2.0 Aug 15, 2023

rohityadavcloud approved these changes Aug 22, 2023

View reviewed changes

harikrishna-patnala added 2 commits August 28, 2023 09:48

default value of force should be false

6290f42

Added force flag in tests

7b3cc99

harikrishna-patnala force-pushed the DontForceDeleteTemplate branch from 29ac938 to 7b3cc99 Compare August 28, 2023 04:19

boring-cyborg bot added component:integration-test Python Warning... Python code Ahead! labels Aug 28, 2023

DaanHoogland added the Severity:Major label Oct 5, 2023

harikrishna-patnala marked this pull request as ready for review October 9, 2023 07:34

DaanHoogland closed this Oct 9, 2023

DaanHoogland reopened this Oct 9, 2023

rohityadavcloud merged commit a9f3af8 into apache:4.18 Oct 10, 2023

rohityadavcloud deleted the DontForceDeleteTemplate branch October 10, 2023 10:20

DaanHoogland mentioned this pull request Oct 30, 2023

Allow deleting used without "force" #8157

Closed

shwstppr mentioned this pull request Feb 5, 2024

4.19 release docs apache/cloudstack-documentation#381

Merged

JoaoJandre mentioned this pull request Mar 14, 2024

Do not retrieve VM's stats on normal VM listing #8782

Merged

13 tasks

Default value of force should be false for template delete operation #7731

Default value of force should be false for template delete operation #7731

Uh oh!

Conversation

harikrishna-patnala commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

harikrishna-patnala commented Jul 7, 2023

Uh oh!

blueorangutan commented Jul 7, 2023

Uh oh!

blueorangutan commented Jul 7, 2023

Uh oh!

codecov bot commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Jul 7, 2023

Uh oh!

blueorangutan commented Jul 7, 2023

Uh oh!

blueorangutan commented Jul 8, 2023

Uh oh!

DaanHoogland commented Jul 10, 2023

Uh oh!

rohityadavcloud commented Jul 10, 2023

Uh oh!

blueorangutan commented Jul 10, 2023

Uh oh!

blueorangutan commented Jul 10, 2023

Uh oh!

DaanHoogland commented Jul 11, 2023

Uh oh!

harikrishna-patnala commented Jul 14, 2023

Uh oh!

weizhouapache commented Jul 21, 2023

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Jul 25, 2023

Uh oh!

kiranchavala left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Aug 10, 2023

Uh oh!

weizhouapache commented Aug 15, 2023

Uh oh!

rohityadavcloud left a comment

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala commented Aug 28, 2023

Uh oh!

blueorangutan commented Aug 28, 2023

Uh oh!

blueorangutan commented Aug 28, 2023

Uh oh!

harikrishna-patnala commented Aug 28, 2023

Uh oh!

blueorangutan commented Aug 28, 2023

Uh oh!

blueorangutan commented Aug 28, 2023

Uh oh!

DaanHoogland commented Oct 2, 2023

Uh oh!

DaanHoogland commented Oct 5, 2023

Uh oh!

shwstppr commented Oct 6, 2023

Uh oh!

harikrishna-patnala commented Oct 9, 2023

Uh oh!

harikrishna-patnala commented Jul 7, 2023 •

edited

Loading

codecov bot commented Jul 7, 2023 •

edited

Loading