Fix API deleteTrafficType not filtering physical network II

[GutoVeronezi]

Description

While deleting a traffic type, ACS checks whether there are any networks associated with it. However, if we have several physical networks containing a traffic type, ACS does not filter the physical network to do the validation. For instance, if we have two (2) physical networks containing the traffic type Guest, the first one having networks related to it, and the second not having networks related to it, if we try to remove the traffic type Guest from the second physical network, ACS give us the message The Traffic Type is not deletable because there are existing networks with this traffic type:Guest.

The API deleteTrafficType was designed to filter the physical network where the traffic type is, however, the filtering is not applied correctly. This PR intends to fix this typo to honor the API behavior. This bug was fixed once with #6510; however, it was reintroduced with #6781.

Types of changes

• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] New feature (non-breaking change which adds functionality)
• [x] 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)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• [ ] BLOCKER
• [ ] Critical
• [ ] Major
• [x] Minor
• [ ] Trivial

How Has This Been Tested?

To test the fix, I executed the following steps on an advanced zone:

• create 2 physical networks (A and B);
• add the Guest traffic type in physical network A;
• create a network and a VM on the network;
• add the Guest traffic type in physical network B.

Without the changes, I tried to remove the Guest traffic type from physical networks A and B; for both, ACS returned the error message The Traffic Type is not deletable because there are existing networks with this traffic type:Guest.

With the changes, I tried to remove the Guest traffic type from physical networks A and B; for A, ACS returned the error message The Traffic Type is not deletable because there are existing networks with this traffic type:Guest; for B, it was removed correctly.

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 17.57%. Comparing base (2600965) to head (d32f8c2). :warning: Report is 7 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #12184      +/-   ##
============================================
- Coverage     17.57%   17.57%   -0.01%     
+ Complexity    15550    15549       -1     
============================================
  Files          5913     5913              
  Lines        529427   529427              
  Branches      64677    64677              
============================================
- Hits          93024    93022       -2     
- Misses       425940   425943       +3     
+ Partials      10463    10462       -1     

Flag	Coverage Δ
uitests	3.58% <ø> (ø)
unittests	18.63% <100.00%> (-0.01%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[GutoVeronezi]

@blueorangutan package

[blueorangutan]

@GutoVeronezi 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]

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

[DaanHoogland]

regression tests make no sense as testing this will need destructive action on an env. @soreana , any insights to share?

[soreana]

regression tests make no sense as testing this will need destructive action on an env. @soreana , any insights to share?

I’m not sure how this issue is related to my PR. I didn’t change the listByPhysicalNetworkTrafficType method in NetworkDaoImpl.java. My change only checks whether the network has at most one tag with null traffic. Let me know if you need more clarification testing, I can have a look later this week.

[GutoVeronezi]

regression tests make no sense as testing this will need destructive action on an env. @soreana , any insights to share?

I’m not sure how this issue is related to my PR. I didn’t change the listByPhysicalNetworkTrafficType method in NetworkDaoImpl.java. My change only checks whether the network has at most one tag with null traffic. Let me know if you need more clarification testing, I can have a look later this week.

Hello @soreana,

The PR #6871 changed the and operator for AllFieldsSearch on line 132 of the file:

https://github.com/apache/cloudstack/pull/6781/files#diff-1581a1045b0596815f553a6cd480c708dd25e29f7042d7b053dba51a19f5bf34

AllFieldsSearch with the physical network ID was only used on listByPhysicalNetworkTrafficType. As the PRs renamed the parameter from physicalNetwork to physicalNetworkId on line 132 but didn't change where it called the setParameters method, the filter stopped working.

Also, refer to https://github.com/apache/cloudstack/pull/6510/files#r908135825.

[soreana]

Now it makes sense, thanks for the detailed answer @GutoVeronezi