-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
prevent duplicate ip table rules in SSVM #8530
prevent duplicate ip table rules in SSVM #8530
Conversation
057ab32
to
ca2f543
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.18 #8530 +/- ##
=========================================
Coverage 13.17% 13.17%
Complexity 9204 9204
=========================================
Files 2724 2724
Lines 258137 258137
Branches 40235 40235
=========================================
Hits 33998 33998
Misses 219830 219830
Partials 4309 4309 ☔ View full report in Codecov by Sentry. |
...server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java
Outdated
Show resolved
Hide resolved
...server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java
Outdated
Show resolved
Hide resolved
@DaanHoogland |
...server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java
Outdated
Show resolved
Hide resolved
5dd121c
to
0d50cba
Compare
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9860 |
...dary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/IpTablesHelper.java
Show resolved
Hide resolved
...dary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/IpTablesHelper.java
Outdated
Show resolved
Hide resolved
...storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java
Outdated
Show resolved
Hide resolved
...storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java
Outdated
Show resolved
Hide resolved
...storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java
Outdated
Show resolved
Hide resolved
@blueorangutan package |
@weizhouapache 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 9881 |
@blueorangutan test rocky8 kvm-rocky8 |
@weizhouapache a [SL] Trillian-Jenkins test job (rocky8 mgmt + kvm-rocky8) has been kicked to run smoke tests |
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.
overall lgtm
left minor comments
...storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java
Show resolved
Hide resolved
...storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java
Show resolved
Hide resolved
@@ -243,12 +243,19 @@ public String execute(OutputInterpreter interpreter) { | |||
//process completed successfully | |||
if (_process.exitValue() == 0) { | |||
_logger.debug("Execution is successful."); | |||
String result; |
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.
I guess these are for debugging, right ? @DaanHoogland
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.
yes, should I remove those? I think they could serve purpose for other work as well.
...storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java
Outdated
Show resolved
Hide resolved
...storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java
Outdated
Show resolved
Hide resolved
@@ -243,12 +243,19 @@ public String execute(OutputInterpreter interpreter) { | |||
//process completed successfully | |||
if (_process.exitValue() == 0) { | |||
_logger.debug("Execution is successful."); | |||
String result; |
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.
yes, should I remove those? I think they could serve purpose for other work as well.
...storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java
Outdated
Show resolved
Hide resolved
...dary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/IpTablesHelper.java
Outdated
Show resolved
Hide resolved
…udstack/storage/template/DownloadManagerImpl.java Co-authored-by: Wei Zhou <[email protected]>
@blueorangutan package |
@vladimirpetrov 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 9902 |
[SF] Trillian test result (tid-10417)
|
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.
Code LGTM
The failures are due to a host being in connecting state, which has not to do with the SSVM, so I consider them unrelated. |
a side-note, I have faced the issue frequently, a random node (among 3) is stuck at Connecting state. It comes normal after 3600 seconds.
I think it deserves an investigation. cc @vishesh92 @DaanHoogland |
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, using steps as described in the issue.
Co-authored-by: Wei Zhou <[email protected]>
Description
This PR...
Fixes: #8061
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
might become more than minor on long running ssvms
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?