Skip to content

Create API to reassign volume - #6938

Merged
DaanHoogland merged 10 commits into
apache:mainfrom
scclouds:create-api-to-reassign-volume
Jan 27, 2023
Merged

Create API to reassign volume#6938
DaanHoogland merged 10 commits into
apache:mainfrom
scclouds:create-api-to-reassign-volume

Conversation

@JoaoJandre

Copy link
Copy Markdown
Contributor

Description

ACS has an API to change the account that owns a VM; however, it is not possible to perform the same type of operation for volumes. Therefore, we created the API assignVolume to allow the transition of volumes between accounts. For this, the user executing the operation needs the following:

  • permission to the assignVolume API
  • must be able to access the account that will receive the volume
  • must be able to access the volume being moved to a new account

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

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

N Test Result Expected behavior?
1 Call the API to change a volume's owner between two different accounts the volume's owner was changed yes
2 Call the API to change the volume's owner to its own account Exception yes
3 Call the API to change the volume's owner to a disabled account Exception yes
4 Call the API to change the volume's owner to a locked account Exception yes
5 Call the API to change the volume's owner while the volume is attached Exception yes
6 Call the API to change the volume's owner while the volume has a snapshot Exception yes
7 Call the API to change a volume's owner between two different accounts, while the volume is on the secondary storage The volume was moved from the original accounts secondary storage directory to the new owner's secondary storage directory and then was reassigned yes

@sonarqubecloud

sonarqubecloud Bot commented Dec 1, 2022

Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

29.0% 29.0% Coverage
0.0% 0.0% Duplication

@codecov

codecov Bot commented Dec 1, 2022

Copy link
Copy Markdown

Codecov Report

Merging #6938 (690d2c1) into main (d74f64a) will increase coverage by 0.01%.
The diff coverage is 47.82%.

@@             Coverage Diff              @@
##               main    #6938      +/-   ##
============================================
+ Coverage     11.78%   11.79%   +0.01%     
- Complexity     7673     7685      +12     
============================================
  Files          2505     2505              
  Lines        246161   246252      +91     
  Branches      38390    38401      +11     
============================================
+ Hits          29003    29041      +38     
- Misses       213385   213438      +53     
  Partials       3773     3773              
Impacted Files Coverage Δ
...ck/engine/subsystem/api/storage/VolumeService.java 0.00% <ø> (ø)
...tack/storage/endpoint/DefaultEndPointSelector.java 0.00% <0.00%> (ø)
...e/cloudstack/storage/volume/VolumeServiceImpl.java 2.28% <0.00%> (-0.05%) ⬇️
...in/java/com/cloud/server/ManagementServerImpl.java 5.72% <0.00%> (-0.01%) ⬇️
...n/java/com/cloud/storage/VolumeApiServiceImpl.java 14.81% <70.96%> (+1.52%) ⬆️
...dstack/network/contrail/model/ModelObjectBase.java 21.15% <0.00%> (-7.70%) ⬇️
...apache/cloudstack/syslog/AlertsSyslogAppender.java 56.49% <0.00%> (-2.26%) ⬇️
...c/main/java/com/cloud/projects/ProjectManager.java 100.00% <0.00%> (+100.00%) ⬆️

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

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks generally good, but it is good convention to guard log statements of info level and below if they do parameter expansion. Please do so.

@JoaoJandre

Copy link
Copy Markdown
Contributor Author

looks generally good, but it is good convention to guard log statements of info level and below if they do parameter expansion. Please do so.

As there are no processes being executed only to log the data (the data is already loaded) and we do not have a convention in using is...Enabled for a simple String construction, such the ones I added, I would rather not add it and improve the code readability.

@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@DaanHoogland

Copy link
Copy Markdown
Contributor

@JoaoJandre can you look at the conflicts?
And please have a look at #6973 to see if you find that to be an acceptable compromise?

@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@JoaoJandre

Copy link
Copy Markdown
Contributor Author

Hello @DaanHoogland, I just solved the conflicts. Regarding your proposal (#6973), I will take a look at it.

@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@JoaoJandre

Copy link
Copy Markdown
Contributor Author

@DaanHoogland , I took a look at your proposal, and as we are working on the Log4j upgrade, as @GutoVeronezi commented (#6973 (comment)), I would rather not applying an intermediate step.

@github-actions

github-actions Bot commented Jan 4, 2023

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@JoaoJandre

Copy link
Copy Markdown
Contributor Author

Sorry for the delay @DaanHoogland , I fixed it :)

@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@DaanHoogland

Copy link
Copy Markdown
Contributor

and yet again @JoaoJandre . sorry, during this aheavy merge period we get conflicts a lot. hope you have time for this.

@JoaoJandre

Copy link
Copy Markdown
Contributor Author

@DaanHoogland No problem :) done

@DaanHoogland

Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@DaanHoogland a 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

Copy link
Copy Markdown

Packaging result: ✔️ el7 ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 5370

@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@DaanHoogland

Copy link
Copy Markdown
Contributor

I feel for you @JoaoJandre , yet another set of conflicts.

@sonarqubecloud

Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

29.2% 29.2% Coverage
0.0% 0.0% Duplication

@DaanHoogland

Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@DaanHoogland a 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

Copy link
Copy Markdown

Packaging result: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5381

@DaanHoogland

Copy link
Copy Markdown
Contributor

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

Test Result Time (s) Test File
test_01_vpc_site2site_vpn_multiple_options Failure 461.52 test_vpc_vpn.py

@DaanHoogland

Copy link
Copy Markdown
Contributor

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

Test Result Time (s) Test File
test_08_upgrade_kubernetes_ha_cluster Failure 651.63 test_kubernetes_clusters.py

@vladimirpetrov vladimirpetrov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM based on manual testing.

@DaanHoogland DaanHoogland added this to the 4.18.0.0 milestone Jan 27, 2023
@DaanHoogland
DaanHoogland merged commit 61a7225 into apache:main Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants