list by isEncrypted

[DaanHoogland]

Description

This PR...

Fixes: #8635

very much a PoC on this.

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)
• [x] 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
• [x] Minor

Bug Severity

• [ ] BLOCKER
• [ ] Critical
• [x] Major
• [ ] Minor
• [ ] Trivial

Screenshots (if appropriate):

How Has This Been Tested?

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

[codecov[bot]]

Codecov Report

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

Comparison is base (9b18243) 30.74% compared to head (e1eeca8) 30.74%. Report is 25 commits behind head on main.

Files	Patch %	Lines
...main/java/com/cloud/storage/dao/VolumeDaoImpl.java	21.42%	11 Missing :warning:
...ain/java/com/cloud/api/query/QueryManagerImpl.java	33.33%	7 Missing and 1 partial :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8643      +/-   ##
============================================
- Coverage     30.74%   30.74%   -0.01%     
+ Complexity    33053    33042      -11     
============================================
  Files          5352     5352              
  Lines        374460   374485      +25     
  Branches      54610    54614       +4     
============================================
- Hits         115123   115122       -1     
- Misses       244068   244089      +21     
- Partials      15269    15274       +5     

Flag	Coverage Δ
simulator-marvin-tests	24.58% <29.62%> (-0.01%)	:arrow_down:
uitests	4.38% <ø> (ø)
unit-tests	16.41% <11.11%> (-0.01%)	:arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[github-actions[bot]]

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

[weizhouapache]

@DaanHoogland maybe better to add the column to volume_view

[DaanHoogland]

@DaanHoogland maybe better to add the column to volume_view

makes sense but the field doesn't exist. I'll refresh my sql-fu to get it in.

[weizhouapache]

@DaanHoogland maybe better to add the column to volume_view

makes sense but the field doesn't exist. I'll refresh my sql-fu to get it in.

with your PR #8647 , you need to add a line to engine/schema/src/main/resources/META-INF/db/views/cloud.volume_view.sql

    `volumes`.`encrypt_format` AS `encrypt_format`,

then add the field in VolumeJoinVO

[DaanHoogland]

@DaanHoogland maybe better to add the column to volume_view

makes sense but the field doesn't exist. I'll refresh my sql-fu to get it in.

with your PR #8647 , you need to add a line to engine/schema/src/main/resources/META-INF/db/views/cloud.volume_view.sql

    `volumes`.`encrypt_format` AS `encrypt_format`,

then add the field in VolumeJoinVO

I thought you meant a field isEncrypted. this is simpler, thanks.

[weizhouapache]

@DaanHoogland maybe better to add the column to volume_view

makes sense but the field doesn't exist. I'll refresh my sql-fu to get it in.

with your PR #8647 , you need to add a line to engine/schema/src/main/resources/META-INF/db/views/cloud.volume_view.sql

    `volumes`.`encrypt_format` AS `encrypt_format`,

then add the field in VolumeJoinVO

I thought you meant a field isEncrypted. this is simpler, thanks.

if need, you can add a method isEncrypted in VolumeJoinVO :smiley:

[vishesh92]

@DaanHoogland Can we revisit this after we merge https://github.com/apache/cloudstack/pull/8321? This will probably result in conflicts with #8321.

[DaanHoogland]

@blueorangutan package

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 14.96%. Comparing base (7aacbcb) to head (f1b5b2d). Report is 6 commits behind head on 4.19.

Files	Patch %	Lines
...ain/java/com/cloud/api/query/QueryManagerImpl.java	0.00%	6 Missing :warning:
...dstack/api/command/user/volume/ListVolumesCmd.java	0.00%	3 Missing :warning:
...apache/cloudstack/api/response/VolumeResponse.java	0.00%	2 Missing :warning:
...ava/com/cloud/api/query/dao/VolumeJoinDaoImpl.java	0.00%	2 Missing :warning:
...main/java/com/cloud/api/query/vo/VolumeJoinVO.java	0.00%	2 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##               4.19    #8643      +/-   ##
============================================
- Coverage     14.96%   14.96%   -0.01%     
- Complexity    10998    10999       +1     
============================================
  Files          5373     5373              
  Lines        469301   469320      +19     
  Branches      61031    57815    -3216     
============================================
- Hits          70224    70219       -5     
- Misses       391302   391328      +26     
+ Partials       7775     7773       -2     

Flag	Coverage Δ
uitests	4.30% <ø> (-0.01%)	:arrow_down:
unittests	15.67% <0.00%> (-0.01%)	:arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[weizhouapache]

Since the query uses VolumeVO now, the fix could be much simple like

        if (cmd.isEncrypted() != null) {
            if (cmd.isEncrypted()) {
                volumeSearchBuilder.and("encryptFormat", volumeSearchBuilder.entity().getEncryptFormat(), SearchCriteria.Op.NNULL);
            } else {
                volumeSearchBuilder.and("encryptFormat", volumeSearchBuilder.entity().getEncryptFormat(), SearchCriteria.Op.NULL);
            }
        }

[weizhouapache]

code looks good

@DaanHoogland a suggestion, can you add encryptformat to the VolumeResponse ? (admin only)

[DaanHoogland]

code looks good

@DaanHoogland a suggestion, can you add encryptformat to the VolumeResponse ? (admin only)

We could do @weizhouapache , but as this is not touching the actual response object but only the selection criteria, it seems to be a separate issue to me. The Cmd type is not know at the time of Response creation. It goes two levels deep onto the stack in Helper classes. ViewResponseHelper::createVolumeResponse(..) > ApiDBUtils::newVolumeResponse(..)/fillVolumeData(..). This unless we want to always add the encryption type of course. In that case it is no effort.

[weizhouapache]

code looks good @DaanHoogland a suggestion, can you add encryptformat to the VolumeResponse ? (admin only)

We could do @weizhouapache , but as this is not touching the actual response object but only the selection criteria, it seems to be a separate issue to me. The Cmd type is not know at the time of Response creation. It goes two levels deep onto the stack in Helper classes. ViewResponseHelper::createVolumeResponse(..) > ApiDBUtils::newVolumeResponse(..)/fillVolumeData(..). This unless we want to always add the encryption type of course. In that case it is no effort.

ok @DaanHoogland it is a little bit difficult to verify if a volume is encrypted in the listVolumes API reponse.

I just checked the volume_view, it does not have encrypt_format so may need to recreate the volume_view let's fix it in another pr.

[DaanHoogland]

@blueorangutan package

[blueorangutan]

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

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

[DaanHoogland]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

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

Test	Result	Time (s)	Test File
test_01_events_resource	Error	421.82	test_events_resource.py
test_05_rvpc_multi_tiers	Failure	543.50	test_vpc_redundant.py
test_05_rvpc_multi_tiers	Error	543.52	test_vpc_redundant.py

[DaanHoogland]

@vishesh92 can you re-re-view?

[vishesh92]

@DaanHoogland it would be good to have this filter in the UI.

[blueorangutan]

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

[DaanHoogland]

@blueorangutan package

[blueorangutan]

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

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

[github-actions[bot]]

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

[kiranchavala]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

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

Test	Result	Time (s)	Test File