Fix NPE when sending copy command to least busy SSVM #9125

winterhazel · 2024-05-24T12:47:29Z

Description

In order to copy an object from a secondary storage to another, the MS sends a copy command to the least busy SSVM; however, removed SSVMs are considered when listing the SSVMs ordered by the quantity of commands being executed. This way, if there are commands that have not finished properly in a removed SSVM, a NPE occurs when sending the copy command if the "least busy" SSVM has been removed.

This PRs fixes the NPE.

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

Bug Severity

How Has This Been Tested?

I initiated a data migration between secondary storages;
I verified that the cmd_exec_log table had one entry for a command sent to the SSVM;
Before the data migration finished, I deleted the SSVM;
I verified that the cmd_exec_log still had one entry for the removed SSVM;
I waited until another SSVM was created;
I initiated another data migration between secondary storages.

Before the changes, step 6 would result in a NPE, as the removed SSVM was sometimes chosen as the least busy SSVM.

2024-05-24T12:41:47,662 DEBUG [o.a.c.s.i.SecondaryStorageServiceImpl] (pool-19-thread-2:[]) (logid:) Failed to copy Data java.lang.NullPointerException: Cannot invoke "com.cloud.host.Host.getId()" because "host" is null
	at org.apache.cloudstack.storage.RemoteHostEndPoint.configure(RemoteHostEndPoint.java:76)
	at org.apache.cloudstack.storage.RemoteHostEndPoint.getHypervisorHostEndPoint(RemoteHostEndPoint.java:90)
	at org.apache.cloudstack.storage.endpoint.DefaultEndPointSelector.getEndPointFromHostId(DefaultEndPointSelector.java:454)
	at org.apache.cloudstack.storage.image.BaseImageStoreDriverImpl.sendToLeastBusyEndpoint(BaseImageStoreDriverImpl.java:439)
	at org.apache.cloudstack.storage.image.BaseImageStoreDriverImpl.copyAsync(BaseImageStoreDriverImpl.java:413)
	at org.apache.cloudstack.storage.image.BaseImageStoreDriverImpl.copyAsync(BaseImageStoreDriverImpl.java:422)
	at org.apache.cloudstack.storage.motion.DataMotionServiceImpl.copyAsync(DataMotionServiceImpl.java:70)
	at org.apache.cloudstack.storage.motion.DataMotionServiceImpl.copyAsync(DataMotionServiceImpl.java:117)
	at org.apache.cloudstack.storage.image.SecondaryStorageServiceImpl.migrateJob(SecondaryStorageServiceImpl.java:167)
	at org.apache.cloudstack.storage.image.SecondaryStorageServiceImpl.migrateData(SecondaryStorageServiceImpl.java:124)
	at org.apache.cloudstack.engine.orchestration.StorageOrchestrator$MigrateDataTask.call(StorageOrchestrator.java:560)
	at org.apache.cloudstack.engine.orchestration.StorageOrchestrator$MigrateDataTask.call(StorageOrchestrator.java:529)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:840)

After applying the changes, step 6 does not throw a NPE, as the removed SSVM is not considered when listing the SSVMs anymore.

winterhazel · 2024-05-24T12:50:19Z

@blueorangutan package

blueorangutan · 2024-05-24T12:52:03Z

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

codecov · 2024-05-24T12:56:15Z

Codecov Report

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

Project coverage is 15.55%. Comparing base (1e12a80) to head (34e3c16).
Report is 15 commits behind head on main.

Files	Patch %	Lines
.../src/main/java/com/cloud/host/dao/HostDaoImpl.java	0.00%	15 Missing ⚠️
...dstack/storage/image/BaseImageStoreDriverImpl.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##               main    #9125     +/-   ##
===========================================
  Coverage     15.55%   15.55%             
  Complexity    12011    12011             
===========================================
  Files          5500     5500             
  Lines        481889   481889             
  Branches      62240    58646   -3594     
===========================================
+ Hits          74966    74970      +4     
+ Misses       398637   398633      -4     
  Partials       8286     8286

Flag	Coverage Δ
uitests	`4.17% <ø> (ø)`
unittests	`16.33% <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.

blueorangutan · 2024-05-24T14:07:45Z

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

DaanHoogland

code looks good, but i think we must consider moving this to the DAO layer.. maybe not in the scope of this pr and we'll need a standard way/place to deal with joins from the code as opposed to views.

DaanHoogland · 2024-05-28T05:47:52Z

@blueorangutan test

blueorangutan · 2024-05-28T05:48:03Z

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

blueorangutan · 2024-05-28T20:22:42Z

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

Test	Result	Time (s)	Test File
test_02_trigger_shutdown	`Failure`	351.82	test_safe_shutdown.py

weizhouapache

code lgtm

Not tested

borisstoyanov

LGTM, tested it, on the second try with new SSVM running I did not had the NPE with a record still in the DB. @DaanHoogland I think your comment is still standing, assigning to you and removing the needs-testing label

DaanHoogland · 2024-06-11T06:43:35Z

@winterhazel , do you think my comment is worth your effort now? If not I'll add a suggestion for a comment for later work.

engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java

winterhazel · 2024-06-11T13:52:28Z

@winterhazel , do you think my comment is worth your effort now? If not I'll add a suggestion for a comment for later work.

I'll move it to the DAO layer in this PR already ;)

winterhazel · 2024-06-11T14:59:02Z

@blueorangutan package

blueorangutan · 2024-06-11T15:00:04Z

@winterhazel 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-11T16:19:54Z

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

BryanMLima · 2024-06-12T18:18:07Z

@DaanHoogland, can we run the CI one last time here?

DaanHoogland · 2024-06-13T05:32:48Z

Yes @BryanMLima but our lab is reserved for 4.19 work this week. main will have to wait.

JoaoJandre · 2024-07-24T12:01:27Z

@blueorangutan package

blueorangutan · 2024-07-24T12:02:03Z

@JoaoJandre 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-07-24T13:18:35Z

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

JoaoJandre · 2024-07-25T13:50:43Z

@DaanHoogland could we run the CI here?

DaanHoogland · 2024-08-12T11:06:56Z

@blueorangutan package

blueorangutan · 2024-08-12T11:08:05Z

@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-08-12T12:23:10Z

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

DaanHoogland · 2024-08-12T13:06:48Z

@blueorangutan test

blueorangutan · 2024-08-12T13:08:05Z

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

blueorangutan · 2024-08-12T13:47:49Z

[SF] Trillian Build Failed (tid-11062)

winterhazel · 2024-08-15T13:57:59Z

@DaanHoogland could we run the CI again?

DaanHoogland · 2024-08-16T07:40:10Z

@blueorangutan LLtest

blueorangutan · 2024-08-16T07:42:03Z

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

blueorangutan · 2024-08-16T22:42:41Z

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

Test	Result	Time (s)	Test File
test_02_domain_admin_access	`Failure`	0.22	test_account_access.py
test_01_invalid_upgrade_kubernetes_cluster	`Failure`	0.01	test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster	`Failure`	0.00	test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle	`Error`	0.00	test_kubernetes_clusters.py
ContextSuite context=TestResourceNames>:setup	`Error`	0.00	test_resource_names.py
test_02_unsecure_vm_migration	`Error`	422.55	test_vm_life_cycle.py

DaanHoogland · 2024-08-23T08:27:01Z

@blueorangutan package

blueorangutan · 2024-08-23T08:28: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-08-23T09:30:39Z

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

DaanHoogland · 2024-08-26T07:21:43Z

@blueorangutan test

blueorangutan · 2024-08-26T07:24:05Z

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

blueorangutan · 2024-08-26T23:43:23Z

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

Test	Result	Time (s)	Test File
test_02_unsecure_vm_migration	`Error`	446.67	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	375.85	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	356.68	test_vm_life_cycle.py
test_01_redundant_vpc_site2site_vpn	`Failure`	444.73	test_vpc_vpn.py

DaanHoogland · 2024-08-27T19:04:12Z

@blueorangutan test

blueorangutan · 2024-08-27T19:06:03Z

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

blueorangutan · 2024-08-28T09:01:52Z

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

Test	Result	Time (s)	Test File

Fix NPE when sending copy command to least busy SSVM

1e21e4d

boring-cyborg bot added the component:storage label May 24, 2024

DaanHoogland approved these changes May 28, 2024

View reviewed changes

DaanHoogland added the status:needs-testing label May 28, 2024

weizhouapache approved these changes May 29, 2024

View reviewed changes

borisstoyanov self-assigned this Jun 10, 2024

borisstoyanov approved these changes Jun 10, 2024

View reviewed changes

borisstoyanov removed the status:needs-testing label Jun 10, 2024

borisstoyanov assigned DaanHoogland and unassigned borisstoyanov Jun 10, 2024

DaanHoogland reviewed Jun 11, 2024

View reviewed changes

engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java Outdated Show resolved Hide resolved

Move ssvmWithLeastMigrateJobs() to HostDao

ccc3462

BryanMLima approved these changes Jun 12, 2024

View reviewed changes

Merge branch 'main' into fix-npe-when-sending-copycmd-to-least-busy-ssvm

34e3c16

DaanHoogland merged commit 7692b74 into apache:main Aug 28, 2024

yadvr added this to the 4.20.0.0 milestone Aug 29, 2024

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Aug 30, 2024

Fix NPE when sending copy command to least busy SSVM (apache#9125)

dffa1db

DaanHoogland removed their assignment Dec 24, 2025

Fix NPE when sending copy command to least busy SSVM #9125

Fix NPE when sending copy command to least busy SSVM #9125

Uh oh!

Conversation

winterhazel commented May 24, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

How Has This Been Tested?

Uh oh!

winterhazel commented May 24, 2024

Uh oh!

blueorangutan commented May 24, 2024

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented May 24, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented May 28, 2024

Uh oh!

blueorangutan commented May 28, 2024

Uh oh!

blueorangutan commented May 28, 2024

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jun 11, 2024

Uh oh!

Uh oh!

winterhazel commented Jun 11, 2024

Uh oh!

winterhazel commented Jun 11, 2024

Uh oh!

blueorangutan commented Jun 11, 2024

Uh oh!

blueorangutan commented Jun 11, 2024

Uh oh!

BryanMLima commented Jun 12, 2024

Uh oh!

DaanHoogland commented Jun 13, 2024

Uh oh!

JoaoJandre commented Jul 24, 2024

Uh oh!

blueorangutan commented Jul 24, 2024

Uh oh!

blueorangutan commented Jul 24, 2024

Uh oh!

JoaoJandre commented Jul 25, 2024

Uh oh!

DaanHoogland commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

DaanHoogland commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

winterhazel commented Aug 15, 2024

Uh oh!

DaanHoogland commented Aug 16, 2024

Uh oh!

blueorangutan commented Aug 16, 2024

Uh oh!

blueorangutan commented Aug 16, 2024

Uh oh!

DaanHoogland commented Aug 23, 2024

codecov bot commented May 24, 2024 •

edited

Loading