server: fix url check for storages without a valid url by shwstppr · Pull Request #8353 · apache/cloudstack

shwstppr · 2023-12-13T12:18:52Z

Description

Fixes #8352
Some managed storages may not need a valid URL to be passed. We can skip check and extraction of host or path from url of such storages.

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)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Fixes apache#8352 Some managed storages may not need a valid URL to be passed. We can skip check and extraction of host or path from url of such storages Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

slavkap · 2023-12-13T12:24:48Z

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

-    private Map<String,String> extractUriParamsAsMap(String url){
+    protected Map<String,String> extractUriParamsAsMap(String url, boolean managed) {
        Map<String,String> uriParams = new HashMap<>();
        UriUtils.UriInfo uriInfo = UriUtils.getUriInfo(url);


@shwstppr, it will fail here if the URL is not valid. Also, the Storpool plugin doesn't use the managed tag

May check the first characters of url instead of the managed parameter

@slavkap @weizhouapache thanks for the pointers. I'll make the change

codecov · 2023-12-13T12:26:55Z

Codecov Report

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

Comparison is base (82f7abd) 28.91% compared to head (cb2442e) 30.74%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
...ain/java/com/cloud/storage/StorageManagerImpl.java	33.33%	21 Missing and 7 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8353      +/-   ##
============================================
+ Coverage     28.91%   30.74%   +1.82%     
- Complexity    31419    33884    +2465     
============================================
  Files          5329     5341      +12     
  Lines        373594   374775    +1181     
  Branches      54319    54515     +196     
============================================
+ Hits         108027   115220    +7193     
+ Misses       250807   244288    -6519     
- Partials      14760    15267     +507

Flag	Coverage Δ
simulator-marvin-tests	`24.65% <14.28%> (-0.07%)`	⬇️
uitests	`4.40% <ø> (-0.03%)`	⬇️
unit-tests	`16.44% <26.19%> (+1.60%)`	⬆️

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.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

slavkap · 2023-12-13T13:04:37Z

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

    public PrimaryDataStoreInfo createPool(CreateStoragePoolCmd cmd) throws ResourceInUseException, IllegalArgumentException, UnknownHostException, ResourceUnavailableException {
        String providerName = cmd.getStorageProviderName();
-        Map<String,String> uriParams = extractUriParamsAsMap(cmd.getUrl());
+        Map<String,String> uriParams = extractUriParamsAsMap(cmd.getUrl(), cmd.isManaged());


@shwstppr, sorry github doesn't allow you to comment on lines without any changes. On lines 825 and 897 there will be null pointer if the uriParams is empty.

825 ScopeType scopeType = uriParams.get("scheme").toString().equals("file") ? ScopeType.HOST : ScopeType.CLUSTER; 897 if (params.get("scheme").toString().equals("file"))

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

slavkap

thanks @shwstppr, code LGTM
tested with NFS, StorPool and Ceph primary storage

weizhouapache

code lgtm

weizhouapache · 2023-12-13T20:00:01Z

@shwstppr ready for review and merge?

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2023-12-14T06:11:40Z

@shwstppr ready for review and merge?

@weizhouapache it should be ready now. Add some more changes to check for a possible NPE

harikrishna-patnala

code LGTM

shwstppr · 2023-12-14T06:46:24Z

@blueorangutan package

blueorangutan · 2023-12-14T06:48:03Z

@shwstppr 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 · 2023-12-14T07:52:53Z

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

shwstppr · 2023-12-14T07:55:20Z

@blueorangutan test

blueorangutan · 2023-12-14T07:56:03Z

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

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

blueorangutan · 2023-12-14T20:45:28Z

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

Test	Result	Time (s)	Test File

shwstppr · 2023-12-15T09:32:10Z

@blueorangutan package

blueorangutan · 2023-12-15T09:36:03Z

@shwstppr 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 · 2023-12-15T10:39:29Z

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

Fixes apache#8352 Some managed storages may not need a valid URL to be passed. We can skip check and extraction of host or path from url of such storages.

server: fix url check for managed storages

a3c545c

Fixes apache#8352 Some managed storages may not need a valid URL to be passed. We can skip check and extraction of host or path from url of such storages Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

boring-cyborg bot added the component:storage label Dec 13, 2023

shwstppr mentioned this pull request Dec 13, 2023

Cannot create primary on StorPool #8352

Closed

slavkap reviewed Dec 13, 2023

View reviewed changes

remove unnecessary imports

ec1da3c

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

slavkap reviewed Dec 13, 2023

View reviewed changes

shwstppr added 2 commits December 13, 2023 22:37

refactor

33785a8

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

fix

af9a284

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

slavkap approved these changes Dec 13, 2023

View reviewed changes

weizhouapache approved these changes Dec 13, 2023

View reviewed changes

checks

e8e0d31

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr marked this pull request as ready for review December 14, 2023 06:11

shwstppr changed the title ~~server: fix url check for managed storages~~ server: fix url check for storages without a valid url Dec 14, 2023

harikrishna-patnala approved these changes Dec 14, 2023

View reviewed changes

DaanHoogland reviewed Dec 14, 2023

View reviewed changes

server/src/main/java/com/cloud/storage/StorageManagerImpl.java Outdated Show resolved Hide resolved

shwstppr modified the milestones: 4.19.1.0, 4.19.0.0 Dec 14, 2023

refactor

cb2442e

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr merged commit de095ba into apache:main Dec 15, 2023

DaanHoogland deleted the fix-storagepool-urlcheck branch December 15, 2023 11:35

Comments

Conversation

shwstppr commented Dec 13, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

slavkap Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

weizhouapache Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

shwstppr Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

slavkap Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

slavkap left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Dec 13, 2023

Uh oh!

shwstppr commented Dec 14, 2023

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Dec 14, 2023

Uh oh!

blueorangutan commented Dec 14, 2023

Uh oh!

blueorangutan commented Dec 14, 2023

Uh oh!

shwstppr commented Dec 14, 2023

Uh oh!

blueorangutan commented Dec 14, 2023

Uh oh!

Uh oh!

blueorangutan commented Dec 14, 2023

Uh oh!

shwstppr commented Dec 15, 2023

Uh oh!

blueorangutan commented Dec 15, 2023

Uh oh!

blueorangutan commented Dec 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Dec 13, 2023 •

edited

Loading