Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

extra null guard#10264

Merged
DaanHoogland merged 1 commit intoapache:4.19apache/cloudstack:4.19from
shapeblue:ghi10154-nullGuardOnReconnectHostshapeblue/cloudstack:ghi10154-nullGuardOnReconnectHostCopy head branch name to clipboard
Jan 27, 2025
Merged

extra null guard#10264
DaanHoogland merged 1 commit intoapache:4.19apache/cloudstack:4.19from
shapeblue:ghi10154-nullGuardOnReconnectHostshapeblue/cloudstack:ghi10154-nullGuardOnReconnectHostCopy head branch name to clipboard

Conversation

@DaanHoogland
Copy link
Contributor

@DaanHoogland DaanHoogland commented Jan 24, 2025

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

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

@DaanHoogland DaanHoogland mentioned this pull request Jan 24, 2025
Merged
14 tasks
@DaanHoogland
Copy link
Contributor Author

DaanHoogland commented Jan 24, 2025

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Jan 24, 2025

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

codecov bot commented Jan 24, 2025 •
edited
Loading

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
weizhouapache approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

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

code lgtm

@blueorangutan
Copy link

blueorangutan commented Jan 24, 2025

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

@DaanHoogland
Copy link
Contributor Author

DaanHoogland commented Jan 24, 2025

@blueorangutan test

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

blueorangutan commented Jan 24, 2025

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

@blueorangutan
Copy link

blueorangutan commented Jan 25, 2025

[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
Copy link
Contributor Author

DaanHoogland commented Jan 27, 2025

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Jan 27, 2025

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

@DaanHoogland DaanHoogland added this to the 4.19.2 milestone Jan 27, 2025
harikrishna-patnala
harikrishna-patnala approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@harikrishna-patnala harikrishna-patnala left a comment

Choose a reason for hiding this comment

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

code LGTM

hsato03
hsato03 approved these changes Jan 27, 2025
View reviewed changes
Copy link
Collaborator

@hsato03 hsato03 left a comment

Choose a reason for hiding this comment

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

LGTM

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);
Copy link
Collaborator

@hsato03 hsato03 Jan 27, 2025

Choose a reason for hiding this comment

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

Maybe use ObjectUtils.defaultIfNull instead of ternary operators?

Copy link
Contributor Author

@DaanHoogland DaanHoogland Jan 27, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@hsato03 hsato03 Jan 27, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@DaanHoogland DaanHoogland Jan 27, 2025

Choose a reason for hiding this comment

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

thanks @hsato03 .

@DaanHoogland DaanHoogland merged commit f652ad0 into apache:4.19 Jan 27, 2025
24 of 25 checks passed
@DaanHoogland DaanHoogland deleted the ghi10154-nullGuardOnReconnectHost branch January 27, 2025 13:14
@blueorangutan
Copy link

blueorangutan commented Jan 27, 2025

[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

DaanHoogland added a commit that referenced this pull request Jan 28, 2025
@DaanHoogland
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
@DaanHoogland
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
@DaanHoogland
extra null guard (apache#10264)
c7decdb
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jun 19, 2025
@DaanHoogland @dhslove
extra null guard (apache#10264)
f226bd7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@harikrishna-patnala harikrishna-patnala harikrishna-patnala approved these changes

@weizhouapache weizhouapache weizhouapache approved these changes

+1 more reviewer

@hsato03 hsato03 hsato03 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component:agent component:orchestration

Projects

None yet

Milestone

4.19.2

Development

Successfully merging this pull request may close these issues.

5 participants

@DaanHoogland @blueorangutan @harikrishna-patnala @hsato03 @weizhouapache
Morty Proxy This is a proxified and sanitized view of the page, visit original site.