List default network offerings when multiple physical networks for guest traffic type exists

[Pearl1594]

Description

This PR fixes: #9673
When there are multiple physical networks for guest traffic type, the default offerings aren't listed for the tagged physical network. Currently new offerings are needed to be created for tagged physical networks to be able to create (Shared) networks.

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
• test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

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

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 15.19%. Comparing base (afc95f1) to head (d4b721e).
Report is 17 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
.../cloud/configuration/ConfigurationManagerImpl.java	0.00%	7 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.19   #10222      +/-   ##
============================================
+ Coverage     15.12%   15.19%   +0.06%     
- Complexity    11268    11404     +136     
============================================
  Files          5408     5408              
  Lines        473954   477623    +3669     
  Branches      57810    59453    +1643     
============================================
+ Hits          71709    72566     +857     
- Misses       394231   396943    +2712     
- Partials       8014     8114     +100     

Flag	Coverage Δ
uitests	4.38% <ø> (+0.08%)	⬆️
unittests	15.91% <0.00%> (+0.06%)	⬆️

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.

[DaanHoogland]

clgtm, needs testing

[DaanHoogland]

@blueorangutan package

[blueorangutan]

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

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

[DaanHoogland]

@blueorangutan test keepEnv

[blueorangutan]

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

[weizhouapache]

@Pearl1594
IMHO , the issue #9673 is because zoneId is passed, therefore ACS checks the available network offerings by tags.

cloudstack/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

Lines 7268 to 7271 in 1c626c8

	if (zone != null) {
	allowNullTag = allowNetworkOfferingWithNullTag(zoneId, pNtwkTags);
	checkForTags = !pNtwkTags.isEmpty() || allowNullTag;
	}

it is not a bug I think.

[Pearl1594]

@Pearl1594 IMHO , the issue #9673 is because zoneId is passed, therefore ACS checks the available network offerings by tags.

cloudstack/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

Lines 7268 to 7271 in 1c626c8

	if (zone != null) {
	allowNullTag = allowNetworkOfferingWithNullTag(zoneId, pNtwkTags);
	checkForTags = !pNtwkTags.isEmpty() || allowNullTag;
	}

it is not a bug I think.

ACS does check the tags, but includes only the ones with the matching tags and not the default ones. I believe the default offerings of the specific network type should be displayed for all physical networks

[weizhouapache]

@Pearl1594 IMHO , the issue #9673 is because zoneId is passed, therefore ACS checks the available network offerings by tags.

cloudstack/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

Lines 7268 to 7271 in 1c626c8

	if (zone != null) {
	allowNullTag = allowNetworkOfferingWithNullTag(zoneId, pNtwkTags);
	checkForTags = !pNtwkTags.isEmpty() || allowNullTag;
	}

it is not a bug I think.

ACS does check the tags, but includes only the ones with the matching tags and not the default ones. I believe the default offerings of the specific network type should be displayed for all physical networks

@Pearl1594
the default offering does not have tags.
Which physical network will be used if guest physical networks are tagged ?

[Pearl1594]

I didn't understand the question @weizhouapache . When creating a shared network, we select the physical network to be used. If there's more than 1 physical network fr guest traffic, we tag them.. based on the physical network chosen - when creating the network, the offerings are listed. So say, if we select the phy network that has a tag, an there exists no offerings matching the tag, nothing is returned. Because we do not include default offerings in the query

[weizhouapache]

I didn't understand the question @weizhouapache . When creating a shared network, we select the physical network to be used. If there's more than 1 physical network fr guest traffic, we tag them.. based on the physical network chosen - when creating the network, the offerings are listed. So say, if we select the phy network that has a tag, an there exists no offerings matching the tag, nothing is returned. Because we do not include default offerings in the query

@Pearl1594
I checked the database and get the following default network offerings

+----+------------------------------------------------------------+
| id | unique_name                                                |
+----+------------------------------------------------------------+
|  1 | System-Public-Network                                      |
|  2 | System-Management-Network                                  |
|  3 | System-Control-Network                                     |
|  4 | System-Storage-Network                                     |
|  5 | System-Private-Gateway-Network-Offering                    |
|  6 | System-Private-Gateway-Network-Offering-Without-Vlan       |
|  7 | DefaultSharedNetworkOfferingWithSGService                  |
|  8 | DefaultSharedNetworkOffering                               |
|  9 | DefaultTungstenSharedNetworkOfferingWithSGService          |
| 10 | DefaultIsolatedNetworkOfferingWithSourceNatService         |
| 11 | DefaultIsolatedNetworkOffering                             |
| 12 | DefaultSharedNetscalerEIPandELBNetworkOffering             |
| 13 | DefaultIsolatedNetworkOfferingForVpcNetworks               |
| 14 | DefaultIsolatedNetworkOfferingForVpcNetworksNoLB           |
| 15 | DefaultIsolatedNetworkOfferingForVpcNetworksWithInternalLB |
| 16 | DefaultL2NetworkOffering                                   |
| 17 | DefaultL2NetworkOfferingVlan                               |
| 18 | DefaultL2NetworkOfferingConfigDrive                        |
| 19 | DefaultL2NetworkOfferingConfigDriveVlan                    |
| 20 | QuickCloudNoServices                                       |
| 21 | DefaultNetworkOfferingforKubernetesService                 |
| 22 | DefaultNSXNetworkOfferingforKubernetesService              |
| 23 | DefaultNSXVPCNetworkOfferingforKubernetesService           |
+----+------------------------------------------------------------+
23 rows in set (0.00 sec)

some are for Isolated networks or L2 networks.
But we do not specify physical networks when create isolated or L2 networks.

IMHO, default=1 just means the network offerings are created by ACS.
they are same as other network offerings created by administrators.

[Pearl1594]

I understand that. Those created by ACS aren't listed by cloudstack when we list by tags

[weizhouapache]

I understand that. Those created by ACS aren't listed by cloudstack when we list by tags

right. I think it is expected.
Users can keep one guest physical network untagged.

[Pearl1594]

Right. That has been the historic behavior. But personally I find it less user friendly that those (default) offerings aren't listed for tagged phy networks and one needs to explicitly create network offerings if they add additional physical networks.

[weizhouapache]

Right. That has been the historic behavior. But personally I find it less user friendly that those (default) offerings aren't listed for tagged phy networks and one needs to explicitly create network offerings if they add additional physical networks.

indeed.
for shared network, we could improve it as the physical network id is passed so tags check might be unnecessary.
for isolated/l2 network, I have no idea how to improve it.

[DaanHoogland]

Right. That has been the historic behavior. But personally I find it less user friendly that those (default) offerings aren't listed for tagged phy networks and one needs to explicitly create network offerings if they add additional physical networks.

indeed. for shared network, we could improve it as the physical network id is passed so tags check might be unnecessary. for isolated/l2 network, I have no idea how to improve it.

maybe by taking action as the second gust network is created, asking if the default offerings should be copied?

I do think that having a second guest network mean/implies you do not want to mix traffic/offerings, or is that daft?

[Pearl1594]

Right. That has been the historic behavior. But personally I find it less user friendly that those (default) offerings aren't listed for tagged phy networks and one needs to explicitly create network offerings if they add additional physical networks.

indeed. for shared network, we could improve it as the physical network id is passed so tags check might be unnecessary. for isolated/l2 network, I have no idea how to improve it.

@weizhouapache I can add a check for guestType = Shared, before searching for offerings based on the tags - however, historically too tags were being used to filter out the offerings listed irrespective of the network type. Do we want this behavior to change cc @DaanHoogland ?

[blueorangutan]

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

Test	Result	Time (s)	Test File

[weizhouapache]

@Pearl1594
thanks for the update
I think the default shared network offerings will be listed, that's good

Can you test the creation of shared networks with

• default share network offering (untagged)
• on tagged physical network ?

[DaanHoogland]

@Pearl1594 @weizhouapache , I know I am a bit of a functional hork, but I really think no offerings should be shown if the tags do not match, for any type of network. For any physnet for which tags are being said we could implement some kind of copyDefaultOfferings(). Would that make sense?
/me not to happy with the conditional.

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Maintainability Rating on New Code (required ≥ B)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[Pearl1594]

@DaanHoogland I am not sure what copyDefaultOfferings() is supposed to do. CloudStack forces a tag for any additional physical (guest) network added. Which should not render the default (shared) offerings unusable for this newly added phy net. It does not list all shared network offerings that have no tags, just those that are provided by CloudStack by default, i.e., If a user creates a shared network offering with no tags, this will not be listed for the a tagged phy net.
The else part added to the conditional, it to allow legacy behaviour for Isolated and L2 networks

[weizhouapache]

@DaanHoogland

Actually there are only 5 default shared network offerings will be listed.

mysql> select id,unique_name  from network_offerings where `default`=1 and guest_type='Shared';
+----+---------------------------------------------------+
| id | unique_name                                       |
+----+---------------------------------------------------+
|  7 | DefaultSharedNetworkOfferingWithSGService         |
|  8 | DefaultSharedNetworkOffering                      |
|  9 | DefaultTungstenSharedNetworkOfferingWithSGService |
| 12 | DefaultSharedNetscalerEIPandELBNetworkOffering    |
| 20 | QuickCloudNoServices                              |
+----+---------------------------------------------------+
5 rows in set (0.00 sec)

not a big deal I think.
my only concern is, do they work with both tagged and untagged physical networks (without more code changes) ?

[Pearl1594]

@Pearl1594 thanks for the update I think the default shared network offerings will be listed, that's good

Can you test the creation of shared networks with

* default share network offering (untagged)

* on tagged physical network ?

@weizhouapache I was able to deploy a VM on a shared network that was created on Physical network 2 (newly added phy net with tag: phy2) using the default offering "QuickCloudNoServices"

[DaanHoogland]

ok, if it works, I'm not blocking it. Need psychopathic testing though!
@rajujith , you wnat to do the honours?

[weizhouapache]

@Pearl1594 thanks for the update I think the default shared network offerings will be listed, that's good
Can you test the creation of shared networks with

* default share network offering (untagged)

* on tagged physical network ?

@weizhouapache I was able to deploy a VM on a shared network that was created on Physical network 2 (newly added phy net with tag: phy2) using the default offering "QuickCloudNoServices"

@Pearl1594
that's good.

can you test other non-default network offerings for shared networks ?
If they work as well, we can list them as well ...

[cmercado93]

Hi, I would like to know if this fix will be applied in version 4.19.2 or if it is expected for a later version

[weizhouapache]

Hi, I would like to know if this fix will be applied in version 4.19.2 or if it is expected for a later version

we do not have consensus yet.

If you have multiple physical networks, I suggest you remove the tag of default physical network.

[DaanHoogland]

The logic looks good to me. @rajujith can you test this? (whether it meats your expectations from #9673 )

[rajujith]

@blueorangutan package

[blueorangutan]

@rajujith 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 ✔️ debian ✔️ suse15. SL-JID 12271