Adding vmId as part of error response when vm create fails. by anniejili · Pull Request #8484 · apache/cloudstack

anniejili · 2024-01-09T22:42:12Z

Description

This PR fixes the difficulty in cleaning up failed VM resources.
If there is a failure during VM create after vm entity is persisted, Vm UUID will be included as part of the error response.

{
"deployvirtualmachineresponse": {
"uuidList": [
{
"serialVersionUID": -7514266713085362000,
"uuid": "f1a9a209-20a8-42ce-8f5c-457f2f560e95",
"description": "vmId"
}
],
"errorcode": 530,
"cserrorcode": 4250,
"errortext": “Error text”
}
}

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

DeployVm API response without fix:

{
"deployvirtualmachineresponse": {
"uuidList": [],
"errorcode": 530,
"cserrorcode": 4250,
"errortext": “Error text”
}
}

After Fix:
{
"deployvirtualmachineresponse": {
"uuidList": [
{
"serialVersionUID": -7514266713085362000,
"uuid": "f1a9a209-20a8-42ce-8f5c-457f2f560e95",
"description": "vmId"
}
],
"errorcode": 530,
"cserrorcode": 4250,
"errortext": “Error text”
}
}

How Has This Been Tested?

Change was tested in my own development environment.

anniejili · 2024-01-09T22:43:05Z

@blueorangutan package

blueorangutan · 2024-01-09T22:58:03Z

@anniejili 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.

blueorangutan · 2024-01-10T00:01:43Z

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

server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

codecov · 2024-01-10T04:54:11Z

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (399bd0a) 30.90% compared to head (ed03888) 30.78%.
Report is 7 commits behind head on main.

Files	Patch %	Lines
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	48.27%	13 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8484      +/-   ##
============================================
- Coverage     30.90%   30.78%   -0.12%     
+ Complexity    34142    34014     -128     
============================================
  Files          5353     5353              
  Lines        375900   375935      +35     
  Branches      54629    54633       +4     
============================================
- Hits         116158   115749     -409     
- Misses       244428   244975     +547     
+ Partials      15314    15211     -103

Flag	Coverage Δ
simulator-marvin-tests	`24.50% <48.27%> (-0.21%)`	⬇️
uitests	`4.38% <ø> (ø)`
unit-tests	`16.62% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

DaanHoogland

@anniejili , it makes function sense what you are trying to do. I have some requests though.

can you make methods for the new code, especially try to avoid having nested try-catch blocks in a single method?
as @harikrishna-patnala requested, please remove commented code? We have source control and can always get the code back if we need to.
you submitted and closed another PR for a similar attempt. There is no reason to open a new one, you can push --force to the same branch to update the PR with your new method of implementation. This will aid in keeping track of discussions on the subject.

That all said, thanks you for you effort and welcome ;) . once again it makes total sense and also seems to do what you suggested on the PR description.

DaanHoogland · 2024-01-10T08:26:17Z

@anniejili , please have a look at https://github.com/apache/cloudstack/actions/runs/7467774820/job/20329156779?pr=8484#step:6:9822 (some line endings were disapproved of by the robots ruling our lives)

anniejili · 2024-01-10T17:06:31Z

Ack.

anniejili · 2024-01-10T17:13:02Z

@blueorangutan package

blueorangutan · 2024-01-10T17:14:03Z

@anniejili 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.

anniejili · 2024-01-10T17:51:55Z

@anniejili , it makes function sense what you are trying to do. I have some requests though.

can you make methods for the new code, especially try to avoid having nested try-catch blocks in a single method?

@DaanHoogland Hi Dahn, Thank you for reviewing my code. As far as I can tell, there is no nested try-catch introduced by this change. Can you please double check?

as @harikrishna-patnala requested, please remove commented code? We have source control and can always get the code back if we need to.

Done. Thank you @harikrishna-patnala!

you submitted and closed another PR for a similar attempt. There is no reason to open a new one, you can push --force to the same branch to update the PR with your new method of implementation. This will aid in keeping track of discussions on the subject.

My bad, will make sure it won't happen next time.

That all said, thanks you for you effort and welcome ;) . once again it makes total sense and also seems to do what you suggested on the PR description.

blueorangutan · 2024-01-10T18:15:26Z

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

server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

DaanHoogland · 2024-01-11T08:38:39Z

@blueorangutan test

blueorangutan · 2024-01-11T08:40:03Z

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

DaanHoogland · 2024-01-11T08:46:01Z

@anniejili , it makes function sense what you are trying to do. I have some requests though.

can you make methods for the new code, especially try to avoid having nested try-catch blocks in a single method?

@DaanHoogland Hi Dahn, Thank you for reviewing my code. As far as I can tell, there is no nested try-catch introduced by this change. Can you please double check?

You are right,I cannot see that nesting of try-catch blocks anymore, maybe I was confusing different PRs. The request remains however; can you make methods for the new code? for readability sake, verbs and nouns are easier to read (given proper method naming) than curls and braces. Also this commitUserVm method is now 150 lines long (not your fault) so let's reduce that a bit.

In no way is this a deprecation of your code, just a suggestion for improvement. (not a 👎 !!)

Co-authored-by: dahn <daan.hoogland@gmail.com>

anniejili · 2024-01-11T19:38:00Z

@blueorangutan package

blueorangutan · 2024-01-11T19:40:03Z

@anniejili 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.

…m_fails

DaanHoogland · 2024-01-12T07:54:49Z

@anniejili , I accidenntly updated your PR. Hope you don't mind.

anniejili · 2024-01-12T16:58:32Z

@anniejili , I accidenntly updated your PR. Hope you don't mind.

No worries.

anniejili · 2024-01-12T16:59:29Z

@DaanHoogland Does the PR look okay now? Thank you!

DaanHoogland · 2024-01-12T19:06:01Z

@DaanHoogland Does the PR look okay now? Thank you!

code looks good @anniejili , but I would still request that you extract the new code into a new method with a clear name.

anniejili · 2024-01-13T00:50:21Z

@DaanHoogland Does the PR look okay now? Thank you!

code looks good @anniejili , but I would still request that you extract the new code into a new method with a clear name.

It is moved to a new method line #4582.

DaanHoogland

clgtm, thanks @anniejili

yadvr · 2024-02-07T06:24:01Z

Conflicts resolving, re kicking packaging for a final check

@blueorangutan package

blueorangutan · 2024-02-07T06:26:03Z

@rohityadavcloud 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.

blueorangutan · 2024-02-07T07:27:53Z

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

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

yadvr · 2024-02-07T11:26:14Z

Thanks @weizhouapache I've addressed some of the cosmetics changes, cc @anniejili
@blueorangutan package

blueorangutan · 2024-02-07T11:28:03Z

@rohityadavcloud 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.

weizhouapache

code lgtm

blueorangutan · 2024-02-07T12:31:28Z

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

weizhouapache · 2024-02-07T12:53:55Z

@blueorangutan test

blueorangutan · 2024-02-07T13:06:04Z

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

blueorangutan · 2024-02-08T01:25:16Z

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

Test	Result	Time (s)	Test File

) * Adding vmId as part of error response when vm create fails. * Removed unneeded comments. * Fixed code review comments. * Update server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java Co-authored-by: dahn <daan.hoogland@gmail.com> * Fixed code review comments. * Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java * Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java * Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java * Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java --------- Co-authored-by: Annie Li <ji_li@apple.com> Co-authored-by: dahn <daan.hoogland@gmail.com> Co-authored-by: dahn <daan@onecht.net> Co-authored-by: Rohit Yadav <rohit.yadav@shapeblue.com> Co-authored-by: Rohit Yadav <rohityadav89@gmail.com>

Adding vmId as part of error response when vm create fails.

3fdee64

harikrishna-patnala reviewed Jan 10, 2024

View reviewed changes

server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java Outdated Show resolved Hide resolved

server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java Outdated Show resolved Hide resolved

DaanHoogland reviewed Jan 10, 2024

View reviewed changes

Removed unneeded comments.

e91c70b

Fixed code review comments.

a4509ea

DaanHoogland reviewed Jan 11, 2024

View reviewed changes

server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java Show resolved Hide resolved

anniejili and others added 2 commits January 11, 2024 09:34

Update server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java

36bc2e2

Co-authored-by: dahn <daan.hoogland@gmail.com>

Fixed code review comments.

8635ffa

Merge branch 'main' into Adding_uuid_as_part_of_error_response_when_v…

4475589

…m_fails

DaanHoogland approved these changes Jan 15, 2024

View reviewed changes

github-actions bot removed the status:has-conflicts label Feb 7, 2024

yadvr changed the base branch from main to 4.19 February 7, 2024 07:11

boring-cyborg bot added component:agent component:build component:integration component:networking labels Feb 7, 2024

yadvr changed the base branch from 4.19 to main February 7, 2024 07:11

weizhouapache reviewed Feb 7, 2024

View reviewed changes

yadvr modified the milestones: 4.19.1.0, 4.20.0.0 Feb 7, 2024

yadvr reviewed Feb 7, 2024

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated Show resolved Hide resolved

Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

20ecf6f

yadvr reviewed Feb 7, 2024

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated Show resolved Hide resolved

Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

d8277b1

yadvr reviewed Feb 7, 2024

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated Show resolved Hide resolved

Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

f29d5ed

yadvr reviewed Feb 7, 2024

View reviewed changes

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated Show resolved Hide resolved

Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

ed03888

weizhouapache approved these changes Feb 7, 2024

View reviewed changes

yadvr merged commit 4de2f38 into apache:main Feb 8, 2024

Conversation

anniejili commented Jan 9, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

anniejili commented Jan 9, 2024

Uh oh!

blueorangutan commented Jan 9, 2024

Uh oh!

blueorangutan commented Jan 10, 2024

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 10, 2024 • 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!

DaanHoogland commented Jan 10, 2024

Uh oh!

anniejili commented Jan 10, 2024

Uh oh!

anniejili commented Jan 10, 2024

Uh oh!

blueorangutan commented Jan 10, 2024

Uh oh!

anniejili commented Jan 10, 2024

Uh oh!

blueorangutan commented Jan 10, 2024

Uh oh!

Uh oh!

DaanHoogland commented Jan 11, 2024

Uh oh!

blueorangutan commented Jan 11, 2024

Uh oh!

DaanHoogland commented Jan 11, 2024

Uh oh!

anniejili commented Jan 11, 2024

Uh oh!

blueorangutan commented Jan 11, 2024

Uh oh!

DaanHoogland commented Jan 12, 2024

Uh oh!

anniejili commented Jan 12, 2024

Uh oh!

anniejili commented Jan 12, 2024

Uh oh!

DaanHoogland commented Jan 12, 2024

Uh oh!

anniejili commented Jan 13, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Feb 7, 2024

Uh oh!

blueorangutan commented Feb 7, 2024

Uh oh!

blueorangutan commented Feb 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yadvr commented Feb 7, 2024

Uh oh!

blueorangutan commented Feb 7, 2024

Uh oh!

weizhouapache left a comment

codecov bot commented Jan 10, 2024 •

edited

Loading