npe guard for get host info on vmware #11054
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
| if (aboutInfo == null) { | ||
| String msg = "no type info about host known" | ||
| s_logger.error(msg) | ||
| throw new Exception(msg); |
There was a problem hiding this comment.
can it be defaulted to VmwareHostType.ESXi ?
There was a problem hiding this comment.
what does it mean if the host does not have this info? Is it in a valid state then?
There was a problem hiding this comment.
I never face this issue before, but I know some users have faced the issue.
If we throw an Exception with message, instead of NPE, I think it does not help, as user will still get an exception.
There was a problem hiding this comment.
yes, but at least it will be clear they have an ill-configured host.
There was a problem hiding this comment.
to be honest, I do not know what is misconfigured and any other issues caused by the misconfiguration.
I think we could consider the host as an ESXi host if the about info is not found.
anyone uses ESX host or other type of host ?
There was a problem hiding this comment.
ok, if y’all insist ;)
There was a problem hiding this comment.
let's get more opinions
@nvazquez @harikrishna-patnala @sureshanaparti @shwstppr @rohityadavcloud
what's your opinion on it ?
There was a problem hiding this comment.
what I meant @weizhouapache , is your argument makes sense, I yield. Please see my changed code fragment at #11054 (comment).
There was a problem hiding this comment.
agreed, assuming ESXi seems to be a good option.
DaanHoogland
commented
Jun 19, 2025
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostMO.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #11054 +/- ##
=========================================
Coverage 15.18% 15.18%
- Complexity 11364 11365 +1
=========================================
Files 5415 5415
Lines 475858 475862 +4
Branches 58092 58093 +1
=========================================
+ Hits 72251 72252 +1
- Misses 395521 395525 +4
+ Partials 8086 8085 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
weizhouapache
approved these changes
Jun 20, 2025
nvazquez
approved these changes
Jun 20, 2025
DaanHoogland
force-pushed
the
ghi10930-NPEGuard
branch
from
f66ce65 to
d3857c8
Compare
DaanHoogland
commented
Jun 23, 2025
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HostMO.java
Outdated
Show resolved
Hide resolved
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13884 |
|
@blueorangutan test ol8 vmware-80u3 |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + vmware-80u3) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13662)
|
|
[SF] Trillian test result (tid-13700)
|
|
I think vmware needs attention, but none of these are related to the change afaict. |
DaanHoogland
force-pushed
the
ghi10930-NPEGuard
branch
from
0524523 to
0a92c41
Compare
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 14086 |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14094 |
|
@blueorangutan test ol8 vmware-70u3 |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + vmware-70u3) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-13724)
|
|
[SF] Trillian test result (tid-13699)
|
borisstoyanov
approved these changes
Jul 11, 2025
|
this seems fine with vmware 70u3, just out of insanity trying 80u3, but i think we can merge and have to give the rest attention in other efforts. |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + vmware-80u3) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-13767) |
|
I think this can be merged right! @DaanHoogland The failures does not seems to be related this change |
these tests are ok, I think, this is good to go @DaanHoogland |
|
[SF] Trillian test result (tid-13768)
|
harikrishna-patnala
pushed a commit
to shapeblue/cloudstack
that referenced
this pull request
Jul 24, 2025
Co-authored-by: Daan Hoogland <[email protected]>
dhslove
pushed a commit
to ablecloud-team/ablestack-cloud
that referenced
this pull request
Aug 6, 2025
Co-authored-by: Daan Hoogland <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR
Fixes: #10930
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?