extra null guard #10264

DaanHoogland · 2025-01-24T10:52:19Z

Description

This PR adds an extra null guard in notify monitors

see the discussion in #10158 (comment)

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

Screenshots (if appropriate):

How Has This Been Tested?

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

DaanHoogland · 2025-01-24T10:53:25Z

@blueorangutan package

blueorangutan · 2025-01-24T10:54: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.

codecov · 2025-01-24T11:00:06Z

Codecov Report

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

Project coverage is 15.14%. Comparing base (0a77eb7) to head (b26fbed).
Report is 1 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
...java/com/cloud/agent/manager/AgentManagerImpl.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.19   #10264   +/-   ##
=========================================
  Coverage     15.14%   15.14%           
  Complexity    11283    11283           
=========================================
  Files          5408     5408           
  Lines        473823   473823           
  Branches      57824    57826    +2     
=========================================
  Hits          71764    71764           
  Misses       394037   394037           
  Partials       8022     8022

Flag	Coverage Δ
uitests	`4.29% <ø> (ø)`
unittests	`15.86% <0.00%> (ø)`

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.

weizhouapache

code lgtm

blueorangutan · 2025-01-24T11:48:41Z

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

DaanHoogland · 2025-01-24T11:58:41Z

@blueorangutan test

blueorangutan · 2025-01-24T12:00:04Z

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

blueorangutan · 2025-01-25T02:26:56Z

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

Test	Result	Time (s)	Test File
test_04_nonsecured_to_secured_vm_migration	`Error`	395.13	test_vm_life_cycle.py

DaanHoogland · 2025-01-27T07:54:01Z

@blueorangutan test

blueorangutan · 2025-01-27T07:56:03Z

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

harikrishna-patnala

code LGTM

hsato03

LGTM

hsato03 · 2025-01-27T11:56:09Z

engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java

                } catch (final HypervisorVersionChangedException hvce) {
                    handleDisconnectWithoutInvestigation(attache, Event.ShutdownRequested, true, true);
-                    throw new CloudRuntimeException("Unable to connect " + attache.getId(), hvce);
+                    throw new CloudRuntimeException("Unable to connect " + (attache == null ? "<unknown agent>" : attache.getId()), hvce);


Maybe use ObjectUtils.defaultIfNull instead of ternary operators?

I would not mind , but don't see that this improves robustness or readability @hsato03 . What is your intention/sugestion?

@DaanHoogland the intention was to improve readability, but after reading the code again I realized that the method would not be useful in this situation. Please ignore my suggestion 😅

thanks @hsato03 .

blueorangutan · 2025-01-27T21:31:13Z

[SF] Trillian test result (tid-12190)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 46692 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10264-t12190-kvm-ol8.zip
Smoke tests completed. 131 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_sys_vm_start	`Failure`	240.42	test_secondary_storage.py
test_11_destroy_ssvm	`Failure`	924.69	test_ssvm.py
test_12_destroy_cpvm	`Failure`	918.77	test_ssvm.py

* 4.19: extra null guard (#10264)

* 4.20: server: investigate pending HA work when executing in new MS session (#10167) extra null guard (#10264)

extra null guard

b26fbed

boring-cyborg bot added component:agent component:orchestration labels Jan 24, 2025

DaanHoogland mentioned this pull request Jan 24, 2025

deal with NPE during host reconnect #10158

Merged

14 tasks

DaanHoogland requested review from harikrishna-patnala and weizhouapache January 24, 2025 10:53

weizhouapache approved these changes Jan 24, 2025

View reviewed changes

DaanHoogland marked this pull request as ready for review January 24, 2025 11:58

DaanHoogland added this to the 4.19.2 milestone Jan 27, 2025

harikrishna-patnala approved these changes Jan 27, 2025

View reviewed changes

hsato03 approved these changes Jan 27, 2025

View reviewed changes

DaanHoogland merged commit f652ad0 into apache:4.19 Jan 27, 2025
24 of 25 checks passed

DaanHoogland deleted the ghi10154-nullGuardOnReconnectHost branch January 27, 2025 13:14

DaanHoogland added a commit that referenced this pull request Jan 28, 2025

Merge release branch 4.19 to 4.20

717ce98

* 4.19: extra null guard (#10264)

DaanHoogland added a commit that referenced this pull request Jan 28, 2025

Merge release branch 4.20 to main

048649d

* 4.20: server: investigate pending HA work when executing in new MS session (#10167) extra null guard (#10264)

rg9975 pushed a commit to rg9975/cloudstack that referenced this pull request Jan 31, 2025

extra null guard (apache#10264)

c7decdb

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jun 19, 2025

extra null guard (apache#10264)

f226bd7

extra null guard #10264

extra null guard #10264

Uh oh!

Conversation

DaanHoogland commented Jan 24, 2025

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!

DaanHoogland commented Jan 24, 2025

Uh oh!

blueorangutan commented Jan 24, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jan 24, 2025

Uh oh!

DaanHoogland commented Jan 24, 2025

Uh oh!

blueorangutan commented Jan 24, 2025

Uh oh!

blueorangutan commented Jan 25, 2025

Uh oh!

DaanHoogland commented Jan 27, 2025

Uh oh!

blueorangutan commented Jan 27, 2025

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

hsato03 left a comment

Choose a reason for hiding this comment

Uh oh!

hsato03 Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

hsato03 Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blueorangutan commented Jan 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jan 24, 2025 •

edited

Loading