Skip to content

Conversation

@shwstppr
Copy link
Contributor

@shwstppr shwstppr commented Jan 20, 2025

Description

Fixes #9660

Retrieving results from snapshot_view when only a single id passed was always returning Primary role entry (seen in the 3rd query below. To overcome this all entries for the single ID are fetched and sorted in the JAVA code.

mysql> SELECT id, store_role FROM snapshot_view ORDER BY snapshot_view.snapshot_store_pair ASC;
+----+------------+
| id | store_role |
+----+------------+
|  1 | Image      |
|  1 | Primary    |
|  2 | Image      |
|  2 | Primary    |
+----+------------+
4 rows in set (0.01 sec)

mysql> SELECT id,store_role FROM snapshot_view WHERE snapshot_view.id IN (1,2) GROUP BY snapshot_view.id ORDER BY snapshot_view.snapshot_store_pair ASC;
+----+------------+
| id | store_role |
+----+------------+
|  1 | Image      |
|  2 | Image      |
+----+------------+
2 rows in set (0.00 sec)

mysql> SELECT id,store_role FROM snapshot_view WHERE snapshot_view.id IN (1) GROUP BY snapshot_view.id ORDER BY snapshot_view.snapshot_store_pair ASC;
+----+------------+
| id | store_role |
+----+------------+
|  1 | Primary    |
+----+------------+
1 row in set (0.00 sec)

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
  • test (unit or integration test code)

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?

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

Signed-off-by: Abhishek Kumar <[email protected]>
@codecov
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Project coverage is 15.14%. Comparing base (9967bb3) to head (9c837f7).
Report is 40 commits behind head on 4.19.

Files with missing lines Patch % Lines
...a/com/cloud/api/query/dao/SnapshotJoinDaoImpl.java 88.23% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               4.19   #10216      +/-   ##
============================================
+ Coverage     15.13%   15.14%   +0.01%     
- Complexity    11274    11283       +9     
============================================
  Files          5408     5408              
  Lines        473974   473828     -146     
  Branches      57813    57825      +12     
============================================
+ Hits          71730    71778      +48     
+ Misses       394227   394027     -200     
- Partials       8017     8023       +6     
Flag Coverage Δ
uitests 4.29% <ø> (-0.01%) ⬇️
unittests 15.87% <88.23%> (+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.

@shwstppr
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@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
Copy link

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

@nvazquez nvazquez linked an issue Jan 21, 2025 that may be closed by this pull request
@nvazquez nvazquez self-assigned this Jan 22, 2025
@nvazquez nvazquez marked this pull request as ready for review January 23, 2025 02:32
Copy link
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

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

Thanks @shwstppr LGTM - manually tested OK:

Listed snapshots without ID, with specific IDs and verified physical size is displayed on the API and the UI as well:

(localcloud) 🐱 > list snapshots listall=true filter=physicalsize,virtualsize,state
{
  "count": 2,
  "snapshot": [
    {
      "physicalsize": 1510539264,
      "state": "BackedUp",
      "virtualsize": 8589934592
    },
    {
      "physicalsize": 1510539264,
      "state": "BackedUp",
      "virtualsize": 8589934592
    }
  ]
}
(localcloud) 🐱 > list snapshots listall=true filter=physicalsize,virtualsize,state id=35cb5416-a5d6-4875-9298-d8e39b15ce77 
{
  "count": 1,
  "snapshot": [
    {
      "physicalsize": 1510539264,
      "state": "BackedUp",
      "virtualsize": 8589934592
    }
  ]
}
(localcloud) 🐱 > list snapshots listall=true filter=physicalsize,virtualsize,state id=60de4d7a-548a-44f1-a957-2f8f77f0999f 
{
  "count": 1,
  "snapshot": [
    {
      "physicalsize": 1510539264,
      "state": "BackedUp",
      "virtualsize": 8589934592
    }
  ]
}
Screenshot 2025-01-22 at 23 34 48

@nvazquez
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

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.

clgtm

@shwstppr
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

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

Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

Test Result Time (s) Test File
all_test_events_resource Skipped --- test_events_resource.py
all_test_gateway_on_shared_networks Skipped --- test_gateway_on_shared_networks.py
all_test_global_acls Skipped --- test_global_acls.py
all_test_global_settings Skipped --- test_global_settings.py
all_test_guest_os Skipped --- test_guest_os.py
all_test_guest_vlan_range Skipped --- test_guest_vlan_range.py
all_test_host_control_state Skipped --- test_host_control_state.py
all_test_hostha_simulator Skipped --- test_hostha_simulator.py
all_test_host_ping Skipped --- test_host_ping.py
all_test_human_readable_logs Skipped --- test_human_readable_logs.py
all_test_image_store_object_migration Skipped --- test_image_store_object_migration.py

@borisstoyanov
Copy link
Contributor

@blueorangutan test keepEnv

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

Test Result Time (s) Test File

Copy link
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

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

LGTM, manually checked it

@DaanHoogland DaanHoogland merged commit 37c29f8 into apache:4.19 Feb 4, 2025
26 checks passed
@DaanHoogland DaanHoogland deleted the fix-snapshot-physize branch February 4, 2025 10:24
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jun 19, 2025
Signed-off-by: Abhishek Kumar <[email protected]>
Co-authored-by: Wei Zhou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Snapshot defaulting to Physical Size 0

6 participants