Skip to content

Conversation

@shwstppr
Copy link
Contributor

@shwstppr shwstppr commented Jan 11, 2022

Description

Fixes #5696

  • Find suitable custom disk offering for volume upload based on the zone.
  • In UI, added diskoffering option in Upload volume from local.

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):

Screenshot from 2022-01-28 13-47-08

How Has This Been Tested?

  • Deployed an env with 2 zones
  • Tested volume upload local/url without passing diskoffering
  • Deleted default custom disk offering, created two separate custom disk offerings for the two zones and then:
  1. Tested local/url volume upload to both zones, one by one without passing offering
  2. Tested local/url volume upload to both zones, one by one while passing offering
  3. Tested local/url volume upload to first zone while passing offering offering from second zone

Fixes apache#5696

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@shwstppr
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

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

@sureshanaparti
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@DaanHoogland
Copy link
Contributor

code looks good so far, @shwstppr How far is this from done?

@shwstppr
Copy link
Contributor Author

@DaanHoogland I was not able to test a multi-zone setup yet. Also, I was not sure if it will correct to use a different custom offering as default other than one with unique_name=Cloud.com-Custom. So would like other's opinion.

@DaanHoogland
Copy link
Contributor

The offering name should not matter.
I would like to see the user have the opportunity to choose an offering and maybe even create one during the upload process. For now this seems a bit out of scope. Just something to think about.

Let me know when this is ready for review.

@blueorangutan
Copy link

Trillian test result (tid-2862)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31305 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5852-t2862-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test Result Time (s) Test File

@shwstppr shwstppr marked this pull request as ready for review January 17, 2022 11:28
@DaanHoogland DaanHoogland self-assigned this Jan 19, 2022
@shwstppr
Copy link
Contributor Author

The offering name should not matter. I would like to see the user have the opportunity to choose an offering and maybe even create one during the upload process. For now this seems a bit out of scope. Just something to think about.

Let me know when this is ready for review.

@DaanHoogland uploadVolume API does have a param to specify diskoffering. Creating a new offering during volume upload itself in the UI won't make a lot of sense as disk offering creation by default is not allowed to default User role.
Sorry forgot to ping you after marking the PR ready for review but I guess you already noticed.

@shwstppr shwstppr linked an issue Jan 24, 2022 that may be closed by this pull request
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@shwstppr
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

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

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

tested LGTM

@apache apache deleted a comment from sureshanaparti Jan 29, 2022
@shwstppr
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

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

@sureshanaparti
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

Trillian test result (tid-3054)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37416 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5852-t3054-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test Result Time (s) Test File

@sureshanaparti
Copy link
Contributor

@shwstppr Upload volume from URL lists disk offerings with name, whereas Upload volume from Local lists disk offerings with display name, please make them consistent. and When selecting other zone's disk offering in Upload volume from URL, it is not throwing access permission error (which it shown earlier).

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@shwstppr
Copy link
Contributor Author

shwstppr commented Feb 1, 2022

@sureshanaparti

Made UI change for your first issue
Not able reproduce second issue. Error shown when offering from a different zone is selected while upload from URL,

Reverted access check

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@apache apache deleted a comment from blueorangutan Feb 1, 2022
@sureshanaparti
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@shwstppr
Copy link
Contributor Author

shwstppr commented Feb 1, 2022

@sureshanaparti @DaanHoogland I've refactored code changes to use checkAccess so a valid offering can be found using both zone and owner's domain.

@blueorangutan package

@blueorangutan
Copy link

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

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

@sureshanaparti
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

Trillian test result (tid-3105)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34843 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5852-t3105-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test Result Time (s) Test File

@apache apache deleted a comment from blueorangutan Feb 2, 2022
@sureshanaparti
Copy link
Contributor

@sureshanaparti

Made UI change for your first issue Not able reproduce second issue. Error shown when offering from a different zone is selected while upload from URL,

Reverted access check

Verified the UI changes, looks good.

@sureshanaparti
Copy link
Contributor

Tested upload from local and url with user and admin accounts with latest changes. Also, verified the access restriction on other zone's disk offering while uploading volume from url. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Uploaded volumes use the first custom disk offering

4 participants