-
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
Remove duplicate network state checks before shutdown network #8462
base: 4.19
Are you sure you want to change the base?
Remove duplicate network state checks before shutdown network #8462
Conversation
@blueorangutan package |
@sureshanaparti 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #8462 +/- ##
==========================================
Coverage 14.96% 14.97%
- Complexity 11013 11017 +4
==========================================
Files 5377 5377
Lines 469567 469561 -6
Branches 60162 59785 -377
==========================================
+ Hits 70285 70300 +15
+ Misses 391498 391473 -25
- Partials 7784 7788 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8231 |
0284972
to
e4c6586
Compare
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
Outdated
Show resolved
Hide resolved
@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]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8286 |
@blueorangutan test |
@sureshanaparti a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) 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.
I would also fix the log message at line 3034 to something like:
s_logger.debug(String.format("Network [%s] is in Allocated state, no need to shutdown.", network));
...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
Outdated
Show resolved
Hide resolved
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
@blueorangutan package |
@sureshanaparti 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 8305 |
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, didn't test it
@blueorangutan test |
@sureshanaparti a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
[SF] Trillian test result (tid-8853)
|
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
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. didn't test.
@sureshanaparti , you want to revive this? |
2386c0d
to
ec463c2
Compare
@blueorangutan package |
@sureshanaparti 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 9905 |
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 - did some smoke testing with different network types, create/restart with and without cleanup/destroy - no issues found.
Description
This PR removes duplicate network state checks before shutdown network (seems these checks again are not required, maybe added while resolving conflicts),and adds unit tests for the network state checks in shutdown network.
Keeping this checks after acquiring lock, removed others.
cloudstack/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
Lines 3042 to 3049 in 3f9dd4d
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?