list by isEncrypted by DaanHoogland · Pull Request #8643 · apache/cloudstack

DaanHoogland · 2024-02-13T09:08:26Z

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

codecov · 2024-02-13T09:16:33Z

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 ⚠️
...ain/java/com/cloud/api/query/QueryManagerImpl.java	33.33%	7 Missing and 1 partial ⚠️

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%)`	⬇️
uitests	`4.38% <ø> (ø)`
unit-tests	`16.41% <11.11%> (-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.

github-actions · 2024-02-19T09:19:33Z

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

weizhouapache · 2024-02-19T09:29:10Z

@DaanHoogland
maybe better to add the column to volume_view

DaanHoogland · 2024-02-19T13:01:50Z

@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 · 2024-02-19T14:39:54Z

@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 · 2024-02-20T09:13:52Z

@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 · 2024-02-20T10:51:34Z

@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
😃

vishesh92 · 2024-02-21T14:36:20Z

@DaanHoogland Can we revisit this after we merge #8321? This will probably result in conflicts with #8321.

DaanHoogland · 2024-04-26T10:52:24Z

@blueorangutan package

codecov-commenter · 2024-04-26T10:52:30Z

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 ⚠️
...dstack/api/command/user/volume/ListVolumesCmd.java	0.00%	3 Missing ⚠️
...apache/cloudstack/api/response/VolumeResponse.java	0.00%	2 Missing ⚠️
...ava/com/cloud/api/query/dao/VolumeJoinDaoImpl.java	0.00%	2 Missing ⚠️
...main/java/com/cloud/api/query/vo/VolumeJoinVO.java	0.00%	2 Missing ⚠️

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%)`	⬇️
unittests	`15.67% <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.

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

weizhouapache · 2024-04-26T11:45:06Z

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 · 2024-04-30T10:59:34Z

code looks good

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

DaanHoogland · 2024-04-30T11:27:18Z

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 · 2024-04-30T11:39:45Z

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 · 2024-05-22T12:29:47Z

@blueorangutan package

blueorangutan · 2024-05-22T12:32:04Z

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

weizhouapache

code lgtm

blueorangutan · 2024-05-22T13:36:41Z

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

DaanHoogland · 2024-05-22T13:44:18Z

@blueorangutan test

blueorangutan · 2024-05-22T13:46:05Z

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

blueorangutan · 2024-05-23T13:08:59Z

[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

blueorangutan · 2024-06-07T13:46:07Z

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

DaanHoogland · 2024-06-10T13:29:04Z

@blueorangutan package

blueorangutan · 2024-06-10T13:30:03Z

@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 · 2024-06-10T14:25:24Z

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

github-actions · 2024-06-11T06:27:08Z

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

kiranchavala

LGTM, Tested the API and it works fine

Create a encrypted custom disk offering
Create a data disk with the disk offering mentioned in step 1
attach it to a running vm
Execute the list volume with isencyrpted parameter


(cmk) 🐱 > list volumes isencrypted=true
{
  "count": 1,
  "volume": [
    {
      "account": "admin",
      "clusterid": "fe78444f-224d-442d-ac45-a476151854e2",
      "clustername": "p1-c1",
      "created": "2024-06-11T07:14:48+0000",
      "destroyed": false,
      "deviceid": 1,
      "diskioread": 0,
      "diskiowrite": 0,
      "diskkbsread": 0,
      "diskkbswrite": 0,
      "diskofferingdisplaytext": "encrypt",
      "diskofferingid": "9d7cf65d-003c-4372-ba4e-af94e535cec6",
      "diskofferingname": "encrypt",
      "displayvolume": true,
      "domain": "ROOT",
      "domainid": "ee255cb8-27ba-11ef-9848-1e00210001ab",
      "encryptformat": "luks",
      "hasannotations": false,
      "hypervisor": "KVM",
      "id": "179b9925-1867-4b0f-abbc-40ebe964575e",
      "isextractable": true,
      "name": "DATA-3",
      "path": "179b9925-1867-4b0f-abbc-40ebe964575e",
      "podid": "40c6e7f0-a613-49ae-847c-3fe8c2d3623f",
      "podname": "Pod1",
      "provisioningtype": "thin",
      "quiescevm": false,
      "size": 10737418240,
      "state": "Ready",
      "storage": "ref-trl-6792-k-Mol8-kiran-chavala-kvm-pri1",
      "storageid": "230486b2-6c8f-3338-978e-9147ba94829a",
      "storagetype": "shared",
      "supportsstoragesnapshot": false,
      "tags": [],
      "type": "DATADISK",
      "virtualmachineid": "4c300cb2-d3b2-489f-841e-546e98a3021a",
      "vmdisplayname": "vm1",
      "vmname": "vm1",
      "vmstate": "Running",
      "vmtype": "User",
      "zoneid": "424ee83b-ded2-428e-a8e5-8a4a1b124cb9",
      "zonename": "ref-trl-6792-k-Mol8-kiran-chavala"
    }
  ]
}

kiranchavala · 2024-06-11T07:40:46Z

@blueorangutan test

blueorangutan · 2024-06-11T07:42:04Z

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

blueorangutan · 2024-06-11T20:41:38Z

[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

api/src/main/java/org/apache/cloudstack/api/command/user/volume/ListVolumesCmd.java

…e/ListVolumesCmd.java Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>

DaanHoogland · 2024-06-12T05:52:55Z

@blueorangutan package

blueorangutan · 2024-06-12T05:54:04Z

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

sureshanaparti

clgtm

blueorangutan · 2024-06-12T06:55:55Z

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

sureshanaparti · 2024-06-12T09:40:28Z

Merging this based on the reviews & test results.

* list by isEncrypted * use filter on VO and cleanup * add encryption type to volume response * Update api/src/main/java/org/apache/cloudstack/api/command/user/volume/ListVolumesCmd.java Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>

boring-cyborg bot added component:api component:storage labels Feb 13, 2024

github-actions bot added the status:has-conflicts label Feb 19, 2024

github-actions bot removed the status:has-conflicts label Apr 26, 2024

DaanHoogland requested a review from vishesh92 April 26, 2024 10:52

vishesh92 reviewed Apr 26, 2024

View reviewed changes

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java Outdated Show resolved Hide resolved

DaanHoogland force-pushed the ghi8635_listEncryptedVolumes branch from 66ed7a3 to 01bb5c4 Compare April 30, 2024 11:03

DaanHoogland mentioned this pull request Apr 30, 2024

add encryptionformat to volume reponse for admins #9014

Closed

weizhouapache approved these changes May 22, 2024

View reviewed changes

DaanHoogland force-pushed the ghi8635_listEncryptedVolumes branch from 01bb5c4 to 225d9e5 Compare May 24, 2024 12:01

DaanHoogland added this to the 4.20.0.0 milestone May 31, 2024

DaanHoogland force-pushed the ghi8635_listEncryptedVolumes branch from 225d9e5 to ace138e Compare June 10, 2024 13:27

DaanHoogland changed the base branch from main to 4.19 June 10, 2024 13:28

DaanHoogland added 2 commits June 11, 2024 08:09

list by isEncrypted

2534840

use filter on VO and cleanup

d561e13

github-actions bot added the status:has-conflicts label Jun 11, 2024

add encryption type to volume response

d4f99cc

DaanHoogland force-pushed the ghi8635_listEncryptedVolumes branch from ace138e to d4f99cc Compare June 11, 2024 06:33

github-actions bot removed the status:has-conflicts label Jun 11, 2024

kiranchavala approved these changes Jun 11, 2024

View reviewed changes

sureshanaparti reviewed Jun 12, 2024

View reviewed changes

api/src/main/java/org/apache/cloudstack/api/command/user/volume/ListVolumesCmd.java Outdated Show resolved Hide resolved

Update api/src/main/java/org/apache/cloudstack/api/command/user/volum…

f1b5b2d

…e/ListVolumesCmd.java Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>

sureshanaparti approved these changes Jun 12, 2024

View reviewed changes

sureshanaparti merged commit 4de975f into apache:4.19 Jun 12, 2024

sureshanaparti modified the milestones: 4.20.0.0, 4.19.1.0 Jun 12, 2024

DaanHoogland deleted the ghi8635_listEncryptedVolumes branch June 12, 2024 09:41

Comments

Conversation

DaanHoogland commented Feb 13, 2024

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!

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

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Feb 19, 2024

Uh oh!

weizhouapache commented Feb 19, 2024

Uh oh!

DaanHoogland commented Feb 19, 2024

Uh oh!

weizhouapache commented Feb 19, 2024

Uh oh!

DaanHoogland commented Feb 20, 2024

Uh oh!

weizhouapache commented Feb 20, 2024

Uh oh!

vishesh92 commented Feb 21, 2024

Uh oh!

DaanHoogland commented Apr 26, 2024

Uh oh!

codecov-commenter commented Apr 26, 2024 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

weizhouapache commented Apr 26, 2024

Uh oh!

weizhouapache commented Apr 30, 2024

Uh oh!

DaanHoogland commented Apr 30, 2024

Uh oh!

weizhouapache commented Apr 30, 2024

Uh oh!

DaanHoogland commented May 22, 2024

Uh oh!

blueorangutan commented May 22, 2024

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented May 22, 2024

Uh oh!

DaanHoogland commented May 22, 2024

Uh oh!

blueorangutan commented May 22, 2024

Uh oh!

blueorangutan commented May 23, 2024

Uh oh!

blueorangutan commented Jun 7, 2024

Uh oh!

DaanHoogland commented Jun 10, 2024

Uh oh!

blueorangutan commented Jun 10, 2024

Uh oh!

blueorangutan commented Jun 10, 2024

Uh oh!

github-actions bot commented Jun 11, 2024

Uh oh!

kiranchavala left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiranchavala commented Jun 11, 2024

Uh oh!

blueorangutan commented Jun 11, 2024

Uh oh!

blueorangutan commented Jun 11, 2024

Uh oh!

codecov bot commented Feb 13, 2024 •

edited

Loading

codecov-commenter commented Apr 26, 2024 •

edited by codecov bot

Loading

kiranchavala left a comment •

edited

Loading