Remove duplicate network state checks before shutdown network by sureshanaparti · Pull Request #8462 · apache/cloudstack

sureshanaparti · 2024-01-08T12:12:30Z

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

    
           if (network.getState() == Network.State.Allocated) { 
        
               s_logger.debug("Network is already shutdown: " + network); 
        
               return true; 
        
           } 
        
           if (network.getState() != Network.State.Implemented && network.getState() != Network.State.Shutdown) { 
        
               s_logger.debug("Network is not implemented: " + network); 
        
               return false;

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

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

sureshanaparti · 2024-01-08T12:13:55Z

@blueorangutan package

blueorangutan · 2024-01-08T12:16:05Z

@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 · 2024-01-08T12:21:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 14.97%. Comparing base (b2ef53b) to head (055f2bc).

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

Flag	Coverage Δ
uitests	`4.29% <ø> (ø)`
unittests	`15.68% <100.00%> (+<0.01%)`	⬆️

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.

blueorangutan · 2024-01-08T13:40:38Z

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

github-actions · 2024-01-10T08:32:59Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

DaanHoogland · 2024-01-11T09:06:40Z

@blueorangutan package

blueorangutan · 2024-01-11T09:20:04Z

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

blueorangutan · 2024-01-11T10:26:14Z

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

sureshanaparti · 2024-01-11T10:34:53Z

@blueorangutan test

blueorangutan · 2024-01-11T10:36:03Z

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

JoaoJandre

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));

github-actions · 2024-01-12T08:49:02Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

sureshanaparti · 2024-01-12T13:54:08Z

@blueorangutan package

blueorangutan · 2024-01-12T13:56:03Z

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

blueorangutan · 2024-01-12T14:55:39Z

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

JoaoJandre

CLGTM, didn't test it

sureshanaparti · 2024-01-16T14:15:53Z

@blueorangutan test

blueorangutan · 2024-01-16T14:18:03Z

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

blueorangutan · 2024-01-17T04:12:55Z

[SF] Trillian test result (tid-8853)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48534 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8462-t8853-kvm-centos7.zip
Smoke tests completed. 120 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_create_pvlan_network	`Error`	0.05	test_pvlan.py

DaanHoogland

clgtm

vishesh92

clgtm. didn't test.

DaanHoogland · 2024-05-18T19:02:33Z

@sureshanaparti , you want to revive this?

sureshanaparti · 2024-06-12T15:47:12Z

@blueorangutan package

blueorangutan · 2024-06-12T15:48:03Z

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

blueorangutan · 2024-06-12T16:58:39Z

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

vladimirpetrov

LGTM based on manual testing - did some smoke testing with different network types, create/restart with and without cleanup/destroy - no issues found.

yadvr · 2024-06-21T10:25:28Z

@blueorangutan test

blueorangutan · 2024-06-21T10:26:04Z

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

DaanHoogland · 2024-06-21T12:12:59Z

sorry, was maybe a bit rash here, as smoke tests are still running. GHA were ok though, and prior run passed.

blueorangutan · 2024-06-21T22:41:13Z

[SF] Trillian test result (tid-10548)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42492 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8462-t10548-kvm-centos7.zip
Smoke tests completed. 131 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

…#8462)

boring-cyborg Bot added the component:orchestration label Jan 8, 2024

sureshanaparti force-pushed the duplicate-checks-in-shutdown-network branch from 0284972 to e4c6586 Compare January 10, 2024 05:13

sureshanaparti requested review from DaanHoogland and JoaoJandre January 10, 2024 05:53

github-actions Bot added the status:has-conflicts label Jan 10, 2024

DaanHoogland reviewed Jan 10, 2024

View reviewed changes

Comment thread ...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java Outdated

github-actions Bot removed the status:has-conflicts label Jan 10, 2024

JoaoJandre reviewed Jan 11, 2024

View reviewed changes

Comment thread ...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java Outdated

github-actions Bot added the status:has-conflicts label Jan 12, 2024

github-actions Bot removed the status:has-conflicts label Jan 12, 2024

JoaoJandre approved these changes Jan 12, 2024

View reviewed changes

DaanHoogland approved these changes Jan 17, 2024

View reviewed changes

DaanHoogland added the status:needs-testing label Jan 17, 2024

DaanHoogland added this to the 4.19.1.0 milestone Jan 17, 2024

yadvr changed the base branch from main to 4.19 February 5, 2024 08:47

vishesh92 approved these changes May 9, 2024

View reviewed changes

vladimirpetrov self-assigned this Jun 11, 2024

sureshanaparti added 4 commits June 12, 2024 20:42

Removed duplicate network state checks before shutdown network

ebe844b

Added unit tests for the network state checks in shutdown network

14cc4b1

Review comments

9c9499b

Review comments, and merge fixes

ec463c2

sureshanaparti force-pushed the duplicate-checks-in-shutdown-network branch from 2386c0d to ec463c2 Compare June 12, 2024 15:21

unit test fixes

055f2bc

apache deleted a comment from blueorangutan Jun 12, 2024

vladimirpetrov approved these changes Jun 13, 2024

View reviewed changes

vladimirpetrov removed the status:needs-testing label Jun 13, 2024

DaanHoogland merged commit 9055610 into apache:4.19 Jun 21, 2024

sureshanaparti deleted the duplicate-checks-in-shutdown-network branch June 21, 2024 12:15

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jul 2, 2024

Remove duplicate network state checks before shutdown network (apache…

f65ba81

…#8462)

	if (network.getState() == Network.State.Allocated) {
	s_logger.debug("Network is already shutdown: " + network);
	return true;
	}

	if (network.getState() != Network.State.Implemented && network.getState() != Network.State.Shutdown) {
	s_logger.debug("Network is not implemented: " + network);
	return false;

Conversation

sureshanaparti commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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?

Uh oh!

sureshanaparti commented Jan 8, 2024

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

codecov Bot commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

github-actions Bot commented Jan 10, 2024

Uh oh!

Uh oh!

DaanHoogland commented Jan 11, 2024

Uh oh!

blueorangutan commented Jan 11, 2024

Uh oh!

blueorangutan commented Jan 11, 2024

Uh oh!

sureshanaparti commented Jan 11, 2024

Uh oh!

blueorangutan commented Jan 11, 2024

Uh oh!

JoaoJandre left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jan 12, 2024

Uh oh!

sureshanaparti commented Jan 12, 2024

Uh oh!

blueorangutan commented Jan 12, 2024

Uh oh!

blueorangutan commented Jan 12, 2024

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Jan 16, 2024

Uh oh!

blueorangutan commented Jan 16, 2024

Uh oh!

blueorangutan commented Jan 17, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

vishesh92 left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented May 18, 2024

Uh oh!

sureshanaparti commented Jun 12, 2024

Uh oh!

blueorangutan commented Jun 12, 2024

Uh oh!

blueorangutan commented Jun 12, 2024

Uh oh!

vladimirpetrov left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jun 21, 2024

Uh oh!

blueorangutan commented Jun 21, 2024

Uh oh!

DaanHoogland commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sureshanaparti commented Jan 8, 2024 •

edited

Loading

codecov Bot commented Jan 8, 2024 •

edited

Loading

JoaoJandre left a comment •

edited

Loading

DaanHoogland commented Jun 21, 2024 •

edited

Loading