New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kvm: fix error when enable SSL for kvm agent #7923
Conversation
@blueorangutan package |
@weizhouapache a [SF] 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. |
@weizhouapache could describe the steps to reproducee the error? |
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6941 |
Codecov Report
@@ Coverage Diff @@
## 4.18 #7923 +/- ##
============================================
- Coverage 13.07% 13.06% -0.01%
+ Complexity 9110 9108 -2
============================================
Files 2720 2720
Lines 257526 257533 +7
Branches 40150 40150
============================================
- Hits 33661 33656 -5
- Misses 219636 219649 +13
+ Partials 4229 4228 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@GutoVeronezi
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@weizhouapache is that a critical/blocker issue for 4.18.1, could env loose the passphrase key from agent.properties on usage or upgrade? @blueorangutan test |
@rohityadavcloud a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
@rohityadavcloud |
[SF] Trillian test result (tid-7608)
|
@blueorangutan test rocky8 kvm-rocky8 |
@weizhouapache a [SF] Trillian-Jenkins test job (rocky8 mgmt + kvm-rocky8) has been kicked to run smoke tests |
@blueorangutan test ubuntu22 kvm-ubuntu22 |
@weizhouapache a [SF] Trillian-Jenkins test job (ubuntu22 mgmt + kvm-ubuntu22) has been kicked to run smoke tests |
...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
The URL check issue has been fixed by #7693 in another way I am not sure if this PR will break the feature. let's NOT merge it into 4.18.1.0 |
@@ -1319,13 +1319,13 @@ protected void setupMemoryBalloonStatsPeriod(Connect conn) { | |||
} | |||
} | |||
|
|||
private void enableSSLForKvmAgent(final Map<String, Object> params) { | |||
private void enableSSLForKvmAgent() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have some unit tests for this method.
[SF] Trillian test result (tid-7611)
|
[SF] Trillian test result (tid-7860)
|
@weizhouapache is this covoured by any tests? |
@DaanHoogland no... |
@weizhouapache is this ready for review/testing or more work is needed? |
…en enable SSL for kvm agent This was implemented in PR#6200 and apache#6371 , but broken in PR#6348
5f312b2
to
26702b8
Compare
@harikrishna-patnala , it needs some test(ing) i would put it on my list save the number of PRs on there. We could do with manual testing for now. |
@blueorangutan package |
@vladimirpetrov a [SF] 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]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7421 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM based on manual testing. Uploaded a new download certificate, registered a new HTTPS template, no cloud.jks failures in agents' logs.
@weizhouapache is this ready now? see also #7923 (comment), will you add to that (in this PR)? |
@DaanHoogland |
ok, as @vladimirpetrov tested manually I gues we can merge this, right? |
@DaanHoogland @shwstppr |
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7945 |
@blueorangutan test |
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
[SF] Trillian test result (tid-8485)
|
…en enable SSL for kvm agent (apache#7923)
Description
This PR fixes the error: 'Failed to find passphrase for keystore: cloud.jks'
This was implemented in PR #6200 and #6371 , but broken in PR #6348.
The issue has been fixed in PR #7693, not sure if this change is required or not
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?