You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/09/28 15:17:54 UTC

[GitHub] [cloudstack] soreana opened a new pull request, #6781: Allow creating atmost 1 physical network with null tag

soreana opened a new pull request, #6781:
URL: https://github.com/apache/cloudstack/pull/6781

   ### Description
   
   This pr is a follow up to the https://github.com/apache/cloudstack/pull/3780 quoted the following from the pr:
   
   > Currently, we can create multiple physical networks in the same traffic types with null tags. This feature will ensure that we can create at most 1 physical network in the same traffic type with a null tag.
   >Every physical network should be associated with a tag so that when we create a new shared network, using the tags we can match the shared network with the physical network
   
   
   <!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
   
   <!-- For new features, provide link to FS, dev ML discussion etc. -->
   <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
   
   <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
   <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
   <!-- Fixes: # -->
   
   <!--- ********************************************************************************* -->
   <!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
   <!--- PLEASE PUT AN 'X' in only **ONE** box -->
   <!--- ********************************************************************************* -->
   
   ### 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)
   - [x] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   #### Feature/Enhancement Scale
   
   - [ ] Major
   - [x] Minor
   
   
   ### How Has This Been Tested?
   <!-- Please describe in detail how you tested your changes. -->
   <!-- Include details of your testing environment, and the tests you ran to -->
   <!-- see how your change affects other areas of the code, etc. -->
   
   Please refer to the old pr at: https://github.com/apache/cloudstack/pull/3780
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r996962042


##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4617,36 +4635,30 @@ public PhysicalNetworkServiceProvider getCreatedPhysicalNetworkServiceProvider(L
 
     @Override
     public long findPhysicalNetworkId(long zoneId, String tag, TrafficType trafficType) {
-        List<PhysicalNetworkVO> pNtwks = new ArrayList<PhysicalNetworkVO>();
-        if (trafficType != null) {
-            pNtwks = _physicalNetworkDao.listByZoneAndTrafficType(zoneId, trafficType);
-        } else {
-            pNtwks = _physicalNetworkDao.listByZone(zoneId);
-        }
-
-        if (pNtwks.isEmpty()) {
-            throw new InvalidParameterValueException("Unable to find physical network in zone id=" + zoneId);
-        }
+        return _networkModel.findPhysicalNetworkId(zoneId, tag, trafficType);
+    }
 
-        if (pNtwks.size() > 1) {
-            if (tag == null) {
-                throw new InvalidParameterValueException("More than one physical networks exist in zone id=" + zoneId + " and no tags are specified in order to make a choice");
-            }
+    /**
+     * Function to check if there are any physical networks with traffic type of "trafficType"
+     * and check their tags. If there is more than one network with null tags then throw exception
+     * @param physicalNetwork
+     * @param trafficType
+     */
+    private void checkForPhysicalNetworksWithoutTag(PhysicalNetworkVO physicalNetwork, TrafficType trafficType) {
+        int networkWithoutTagCount = 0;
+        List<PhysicalNetworkVO> physicalNetworkVOList = _physicalNetworkDao
+                .listByZoneAndTrafficType(physicalNetwork.getDataCenterId(), trafficType);
 
-            Long pNtwkId = null;
-            for (PhysicalNetwork pNtwk : pNtwks) {
-                if (pNtwk.getTags().contains(tag)) {
-                    s_logger.debug("Found physical network id=" + pNtwk.getId() + " based on requested tags " + tag);
-                    pNtwkId = pNtwk.getId();
-                    break;
-                }
+        for (PhysicalNetworkVO physicalNetworkVO : physicalNetworkVOList) {
+            List<String> tags = physicalNetworkVO.getTags();
+            if (tags == null || tags.size() == 0){

Review Comment:
   ```suggestion
               CollectionUtils.isEmpty(tags) {
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1280609237

   @DaanHoogland I addressed those. Let me know if you have any concerns.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1261098944

   @blueorangutan package


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1275567168

   <b>Trillian test result (tid-5098)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 46181 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6781-t5098-kvm-centos7.zip
   Smoke tests completed. 102 look OK, 2 have errors, 0 did not run
   Only failed and skipped tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_08_upgrade_kubernetes_ha_cluster | `Failure` | 807.91 | test_kubernetes_clusters.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 468.30 | test_vpc_redundant.py
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] acs-robot commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
acs-robot commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1262050499

   Found UI changes, kicking a new UI QA build
   @blueorangutan ui


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1279759825

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1280728402

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1281205351

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1283548409

   @andrijapanicsb @JoaoJandre @sureshanaparti , are all your comments and requests met?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r996866803


##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -6748,6 +6737,31 @@ public Pair<List<? extends NetworkOffering>, Integer> searchForNetworkOfferings(
         }
     }
 
+    private boolean allowNetworkOfferingWithNullTag(Long zoneId, List<String> pNtwkTags) {
+        boolean allowNullTag = false;
+        final List<PhysicalNetworkVO> pNtwks = _physicalNetworkDao.listByZoneAndTrafficType(zoneId, TrafficType.Guest);

Review Comment:
   Done ๐Ÿ‘ 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1281955339

   @DaanHoogland Can I also have this one in 4.18?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1273490989

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1371545015

   <b>Trillian test result (tid-5742)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 45176 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6781-t5742-kvm-centos7.zip
   Smoke tests completed. 105 look OK, 1 have errors, 0 did not run
   Only failed and skipped tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_07_deploy_kubernetes_ha_cluster | `Failure` | 3618.19 | test_kubernetes_clusters.py
   test_08_upgrade_kubernetes_ha_cluster | `Failure` | 0.05 | test_kubernetes_clusters.py
   test_09_delete_kubernetes_ha_cluster | `Failure` | 0.04 | test_kubernetes_clusters.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 67.72 | test_kubernetes_clusters.py
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1261197712

   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4304


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] codecov[bot] commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1261141222

   # [Codecov](https://codecov.io/gh/apache/cloudstack/pull/6781?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#6781](https://codecov.io/gh/apache/cloudstack/pull/6781?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c22455d) into [main](https://codecov.io/gh/apache/cloudstack/commit/d9dd4c1e3ad795c6842411e76b34f487a095603e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d9dd4c1) will **decrease** coverage by `0.00%`.
   > The diff coverage is `1.56%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##               main    #6781      +/-   ##
   ============================================
   - Coverage     10.52%   10.52%   -0.01%     
   + Complexity     6784     6782       -2     
   ============================================
     Files          2464     2464              
     Lines        243988   244008      +20     
     Branches      38185    38187       +2     
   ============================================
   - Hits          25690    25687       -3     
   - Misses       215065   215088      +23     
     Partials       3233     3233              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/cloudstack/pull/6781?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage ฮ” | |
   |---|---|---|
   | [...ain/java/com/cloud/network/dao/NetworkDaoImpl.java](https://codecov.io/gh/apache/cloudstack/pull/6781/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZW5naW5lL3NjaGVtYS9zcmMvbWFpbi9qYXZhL2NvbS9jbG91ZC9uZXR3b3JrL2Rhby9OZXR3b3JrRGFvSW1wbC5qYXZh) | `3.21% <0.00%> (รธ)` | |
   | [.../main/java/com/cloud/network/NetworkModelImpl.java](https://codecov.io/gh/apache/cloudstack/pull/6781/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL25ldHdvcmsvTmV0d29ya01vZGVsSW1wbC5qYXZh) | `10.88% <0.00%> (-0.04%)` | :arrow_down: |
   | [...ain/java/com/cloud/network/NetworkServiceImpl.java](https://codecov.io/gh/apache/cloudstack/pull/6781/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL25ldHdvcmsvTmV0d29ya1NlcnZpY2VJbXBsLmphdmE=) | `9.79% <0.00%> (-0.03%)` | :arrow_down: |
   | [.../cloud/configuration/ConfigurationManagerImpl.java](https://codecov.io/gh/apache/cloudstack/pull/6781/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL2NvbmZpZ3VyYXRpb24vQ29uZmlndXJhdGlvbk1hbmFnZXJJbXBsLmphdmE=) | `11.23% <4.76%> (-0.01%)` | :arrow_down: |
   | [...dstack/network/contrail/model/ModelObjectBase.java](https://codecov.io/gh/apache/cloudstack/pull/6781/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGx1Z2lucy9uZXR3b3JrLWVsZW1lbnRzL2p1bmlwZXItY29udHJhaWwvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Nsb3Vkc3RhY2svbmV0d29yay9jb250cmFpbC9tb2RlbC9Nb2RlbE9iamVjdEJhc2UuamF2YQ==) | `21.15% <0.00%> (-7.70%)` | :arrow_down: |
   
   :mega: Weโ€™re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1261142038

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6781)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL)
   
   [![4.1%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '4.1%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list) [4.1% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1274847866

   @blueorangutan test


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1273557271

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6781)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL)
   
   [![4.1%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '4.1%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list) [4.1% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1281378144

   > @soreana , some simple changes sugested by sonar. I am running another suite of tests. There is also a suggestion to externalise a string, but I'd leave that one for a separate cleanup-PR.
   
   @DaanHoogland Thanks for the reviews and the tests. Personally, I don't prefer to touch that Throwable line. It may break something unwanted :-)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r996963085


##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4697,6 +4709,13 @@ public PhysicalNetworkTrafficType addTrafficTypeToPhysicalNetwork(Long physicalN
             }
         }
 
+        // Check if there are more than 1 physical network with null tags in same traffic type.
+        // If so then dont allow to add traffic type.
+        List<String> tags = network.getTags();
+        if (tags == null || tags.size() == 0) {

Review Comment:
   ```suggestion
               CollectionUtils.isEmpty(tags) {
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1261844020

   @soreana did you apply any of the comments from #3780?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1261099452

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1261842968

   @blueorangutan test


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1278670971

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1278670711

   @blueorangutan package


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r992453955


##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -6748,6 +6737,33 @@ public Pair<List<? extends NetworkOffering>, Integer> searchForNetworkOfferings(
         }
     }
 
+    private boolean allowNetworkOfferingWithNullTag(Long zoneId, List<String> pNtwkTags) {
+        boolean allowNullTag = false;
+        final List<PhysicalNetworkVO> pNtwks = _physicalNetworkDao.listByZoneAndTrafficType(zoneId, TrafficType.Guest);
+        for (final PhysicalNetworkVO pNtwk : pNtwks) {
+            final List<String> pNtwkTag = pNtwk.getTags();
+            if (pNtwkTag == null || pNtwkTag.isEmpty()) {
+                if (!allowNullTag) {
+                    allowNullTag = true;
+                } else {
+                    throw new CloudRuntimeException("There are more than 1 physical network with empty tag in the zone id=" + zoneId);
+                }
+            } else {
+                pNtwkTags.addAll(pNtwkTag);
+            }
+        }
+        return allowNullTag;
+    }
+
+    private boolean checkNetworkOfferingTags(List<String> pNtwkTags, boolean allowNullTag, String offeringTags) {
+        if (offeringTags == null && !allowNullTag) {
+            return false;
+        } else if (offeringTags != null && !pNtwkTags.contains(offeringTags)) {
+            return false;
+        }

Review Comment:
   this can be one condition
   ```suggestion
           if ((offeringTags == null && !allowNullTag) || (offeringTags != null && !pNtwkTags.contains(offeringTags)) {
               return false;
           }
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1280610671

   @blueorangutan package


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1279759617

   @blueorangutan test


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1279883963

   <b>Trillian test result (tid-5140)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 45215 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6781-t5140-kvm-centos7.zip
   Smoke tests completed. 102 look OK, 2 have errors, 0 did not run
   Only failed and skipped tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_08_upgrade_kubernetes_ha_cluster | `Failure` | 853.17 | test_kubernetes_clusters.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 468.07 | test_vpc_redundant.py
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1262056948

   @DaanHoogland I've changed the integration regarding your comments. Let me know if you have any concerns.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1370755091

   @blueorangutan test


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland closed pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland closed pull request #6781: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/6781


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1372000249

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6781)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL)
   
   [![4.7%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '4.7%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list) [4.7% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1274848830

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1280611382

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1281579508

   <b>Trillian test result (tid-5157)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 37725 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6781-t5157-kvm-centos7.zip
   Smoke tests completed. 104 look OK, 0 have errors, 0 did not run
   Only failed and skipped tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1280511546

   @soreana can you have a look at the comments?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1261966612

   > @soreana did you apply any of the comments from #3780?
   
   @DaanHoogland Sure, I'm on it.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r983545154


##########
test/integration/component/test_multiple_physical_network_creation.py:
##########
@@ -0,0 +1,410 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Import Local Modules
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase, unittest
+from marvin.lib.utils import (validateList,
+                              cleanup_resources,
+                              _execute_ssh_command,
+                              get_host_credentials,
+                              random_gen)
+from marvin.lib.base import (PhysicalNetwork,
+                             Host,
+                             TrafficType,
+                             Domain,
+                             Network,
+                             NetworkOffering,
+                             VirtualMachine,
+                             ServiceOffering,
+                             Zone)
+from marvin.lib.common import (get_domain,
+                               get_zone,
+                               get_template,
+                               list_virtual_machines)
+
+import logging
+
+class TestMulipleNetworkCreation(cloudstackTestCase):
+    @classmethod
+    def setUpClass(cls):
+        cls.testClient = super(
+            TestMulipleNetworkCreation,
+            cls).getClsTestClient()
+        cls.apiclient = cls.testClient.getApiClient()
+        cls.testdata = cls.testClient.getParsedTestDataConfig()
+        cls.services = cls.testClient.getParsedTestDataConfig()
+        cls.domain = get_domain(cls.apiclient)
+        zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.zone = Zone(zone.__dict__)
+        cls.template = get_template(cls.apiclient, cls.zone.id)
+        cls._cleanup = []
+
+        cls.logger = logging.getLogger("TestMulipleNetworkCreation")
+        cls.stream_handler = logging.StreamHandler()
+        cls.logger.setLevel(logging.DEBUG)
+        cls.logger.addHandler(cls.stream_handler)
+
+        # Disable the zone to create physical networks
+        cls.zone.update(
+            cls.apiclient,
+            allocationstate="Disabled"
+        )
+
+        try:
+            cls.physical_network = PhysicalNetwork.create(
+                cls.apiclient,
+                cls.services["l2-network"],
+                zoneid=cls.zone.id
+            )
+            cls._cleanup.append(cls.physical_network)
+
+            cls.physical_network_2 = PhysicalNetwork.create(
+                cls.apiclient,
+                cls.services["l2-network"],
+                zoneid=cls.zone.id
+            )
+            cls._cleanup.append(cls.physical_network_2)
+        except Exception as e:
+            cls.tearDownClass()
+            raise unittest.SkipTest(e)
+
+        cls.kvmnetworklabel=None
+        try:
+            hosts = Host.list(cls.apiclient, type='Routing')
+            if isinstance(hosts, list) and len(hosts) > 0:
+                host = hosts[0]
+            else:
+                return
+            if host.hypervisor.lower() not in "kvm":
+                return
+            host.user, host.passwd = get_host_credentials(cls.config, host.ipaddress)
+            bridges = _execute_ssh_command(host.ipaddress, 22, host.user, host.passwd, "brctl show |grep cloudbr |awk '{print $1}'")
+            existing_physical_networks = PhysicalNetwork.list(cls.apiclient)
+            for existing_physical_network in existing_physical_networks:
+                trafficTypes = TrafficType.list(
+                    cls.apiclient,
+                    physicalnetworkid=existing_physical_network.id)
+                if trafficTypes is None:
+                    continue
+                for traffic_type in trafficTypes:
+                    if traffic_type.traffictype == "Guest":
+                        try:
+                            for bridge in bridges:
+                                if bridge == str(traffic_type.kvmnetworklabel):
+                                    bridges.remove(bridge)
+                        except Exception as e:
+                            continue
+
+            if bridges is not None and len(bridges) > 0:
+                cls.kvmnetworklabel = bridges[0]
+            if cls.kvmnetworklabel is not None:
+                cls.logger.debug("Found an unused kvmnetworklabel %s" %cls.kvmnetworklabel)
+            else:
+                cls.logger.debug("Not find an unused kvmnetworklabel")
+        except Exception as e:
+            return
+
+    @classmethod
+    def tearDownClass(cls):
+        try:
+            cls.zone.update(
+                cls.apiclient,
+                allocationstate="Enabled"
+            )
+            # Cleanup resources used
+            super(TestMulipleNetworkCreation, cls).tearDownClass()
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.cleanup = []
+
+        return
+
+    def tearDown(self):
+        super(TestMulipleNetworkCreation, self).tearDown()
+
+    @attr(tags=["advanced"], required_hardware="false")
+    def test_01_add_traffictype_for_untagged_networks(self):
+        """
+        Try to add to traffic type Guest to each of the network
+        Two physical networks are already created without tags
+
+        1. Try to add traffic type Guest to physcial network
+        2. Ensure that it throws exception
+        3. Now add the tags to the physical network
+        4. Add traffic type Guest to the physical network and it should not throw exception
+        5. Do the same above steps for the other physical network
+
+        :return:
+        """
+        try:
+            # Add traffic type
+            self.physical_network.addTrafficType(
+                self.apiclient,
+                type="Guest"
+            )
+
+            # If the control comes then there is something wrong with the code.
+            # The code should throw exception as there are multiple networks with null tags
+            raise Exception("Exception should occur when adding traffic type since tags are null")
+        except Exception as e:
+            self.logger.info("Exception happened as expected")
+
+        # Now update the network with tags
+        self.physical_network.update(
+            self.apiclient,
+            tags="Guest"
+        )
+
+        # try adding traffic type again. it should not throw error
+        self.physical_network.addTrafficType(
+            self.apiclient,
+            type="Guest"
+        )
+
+        # Do the same thing for the second network
+        try:
+            self.physical_network_2.addTrafficType(
+                self.apiclient,
+                type="Guest"
+            )
+
+            # It should throw exception and should not come here
+            raise Exception("Exception should occur when adding traffic type since tags are null")
+        except Exception as e:
+            self.logger.info("Exception happened as expected")
+
+        # Now update the network with tags
+        self.physical_network_2.update(
+            self.apiclient,
+            tags="Guest"
+        )
+
+        # try adding traffic type again. it should not throw error
+        self.physical_network_2.addTrafficType(
+            self.apiclient,
+            type="Guest"
+        )
+
+    @attr(tags=["advanced"], required_hardware="false")
+    def test_02_created_shared_guest_network(self):
+        """
+        1. Create new physical network
+        2. Update the network with tags and traffic type "Guest"
+        3. Create a network offering and shared network based on the above physical network
+        4. Create a virtual machine using the above created network
+        4. Ensure that the traffic type is Guest and vlan is same as the shared network
+        :return:
+        """
+        #1. Create a physical network
+        self.physical_network_3 = PhysicalNetwork.create(
+            self.apiclient,
+            self.services["l2-network"],
+            isolationmethods="VLAN",
+            zoneid=self.zone.id
+        )
+        self.cleanup.append(self.physical_network_3)
+
+        # Enable the network
+        self.physical_network_3.update(
+            self.apiclient,
+            tags="guest",
+            state="Enabled"
+        )
+
+        #2. try adding traffic type Guest
+        TrafficType.add(
+            self.apiclient,
+            physicalnetworkid=self.physical_network_3.id,
+            kvmnetworklabel=self.kvmnetworklabel,
+            traffictype="Guest"
+        )
+
+        # Create network offering
+        self.services["network_offering_shared"]["supportedservices"] = ""
+        self.services["network_offering_shared"]["serviceProviderList"] = {}
+        self.services["network_offering_shared"]["tags"] = "guest"
+        self.network_offering = NetworkOffering.create(
+            self.apiclient,
+            self.services["network_offering_shared"]
+        )
+
+        # Enable network offering
+        self.network_offering.update(
+            self.apiclient,
+            state='Enabled'
+        )
+
+        #3. Create a shared network
+        self.shared_network = Network.create(
+            self.apiclient,
+            self.services["network2"],
+            networkofferingid=self.network_offering.id,
+            zoneid=self.zone.id,
+            domainid=self.domain.id
+            #physicalnetworkid=self.physical_network_3.id
+        )
+        self.cleanup.append(self.shared_network)
+
+        # Create small service offering
+        self.service_offering = ServiceOffering.create(
+            self.apiclient,
+            self.testdata["service_offerings"]["small"]
+        )
+        self.cleanup.append(self.service_offering)
+
+        #4. Create virtual machine
+        self.testdata["virtual_machine"]["zoneid"] = self.zone.id
+        self.testdata["virtual_machine"]["template"] = self.template.id
+        self.virtual_machine = VirtualMachine.create(
+            self.apiclient,
+            self.testdata["virtual_machine"],
+            templateid=self.template.id,
+            serviceofferingid=self.service_offering.id,
+            networkids=self.shared_network.id
+        )
+        self.cleanup.append(self.virtual_machine)
+
+        list_vms = VirtualMachine.list(
+            self.apiclient,
+            id=self.virtual_machine.id
+        )
+
+        vm = list_vms[0]
+        # make sure that vm uses the same vlan of the network
+        self.assertEqual(vm.nic[0].traffictype,
+                         "Guest",
+                         "Vm traffic type is not same")
+
+        self.assertEqual(vm.nic[0].isolationuri,
+                         "vlan://" + str(self.services["network2"]["vlan"]),
+                         "Vm network vlan is not same")
+
+        self.cleanup.append(self.virtual_machine)
+        self.cleanup.append(self.service_offering)
+        self.cleanup.append(self.shared_network)
+        self.cleanup.append(self.network_offering)
+        self.cleanup.append(self.physical_network_3)

Review Comment:
   @soreana these should be move up each just below the respective create API call they apply to. If not and expcption will prevent these from being cleaned.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1262733326

   <b>Trillian test result (tid-5040)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 44926 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6781-t5040-kvm-centos7.zip
   Smoke tests completed. 101 look OK, 2 have errors, 0 did not run
   Only failed and skipped tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_08_upgrade_kubernetes_ha_cluster | `Failure` | 611.00 | test_kubernetes_clusters.py
   test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 466.08 | test_vpc_redundant.py
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r995443068


##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -6748,6 +6737,33 @@ public Pair<List<? extends NetworkOffering>, Integer> searchForNetworkOfferings(
         }
     }
 
+    private boolean allowNetworkOfferingWithNullTag(Long zoneId, List<String> pNtwkTags) {
+        boolean allowNullTag = false;
+        final List<PhysicalNetworkVO> pNtwks = _physicalNetworkDao.listByZoneAndTrafficType(zoneId, TrafficType.Guest);
+        for (final PhysicalNetworkVO pNtwk : pNtwks) {
+            final List<String> pNtwkTag = pNtwk.getTags();
+            if (pNtwkTag == null || pNtwkTag.isEmpty()) {
+                if (!allowNullTag) {
+                    allowNullTag = true;
+                } else {
+                    throw new CloudRuntimeException("There are more than 1 physical network with empty tag in the zone id=" + zoneId);
+                }
+            } else {
+                pNtwkTags.addAll(pNtwkTag);
+            }
+        }
+        return allowNullTag;
+    }
+
+    private boolean checkNetworkOfferingTags(List<String> pNtwkTags, boolean allowNullTag, String offeringTags) {
+        if (offeringTags == null && !allowNullTag) {
+            return false;
+        } else if (offeringTags != null && !pNtwkTags.contains(offeringTags)) {
+            return false;
+        }

Review Comment:
   Good catch. Done.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1273490072

   @blueorangutan package


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1280680675

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6781)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL)
   
   [![4.4%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '4.4%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list) [4.4% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1280735784

   @soreana , some simple changes sugested by sonar. I am running another suite of tests. There is also a suggestion to externalise a string, but I'd leave that one for a separate cleanup-PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1280728009

   @blueorangutan test


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1273540361

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6781)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL)
   
   [![4.1%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '4.1%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list) [4.1% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1281286239

   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4485


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1369965550

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1262051631

   @acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r983547032


##########
test/integration/component/test_multiple_physical_network_creation.py:
##########
@@ -0,0 +1,410 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Import Local Modules
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase, unittest
+from marvin.lib.utils import (validateList,
+                              cleanup_resources,
+                              _execute_ssh_command,
+                              get_host_credentials,
+                              random_gen)
+from marvin.lib.base import (PhysicalNetwork,
+                             Host,
+                             TrafficType,
+                             Domain,
+                             Network,
+                             NetworkOffering,
+                             VirtualMachine,
+                             ServiceOffering,
+                             Zone)
+from marvin.lib.common import (get_domain,
+                               get_zone,
+                               get_template,
+                               list_virtual_machines)
+
+import logging
+
+class TestMulipleNetworkCreation(cloudstackTestCase):
+    @classmethod
+    def setUpClass(cls):
+        cls.testClient = super(
+            TestMulipleNetworkCreation,
+            cls).getClsTestClient()
+        cls.apiclient = cls.testClient.getApiClient()
+        cls.testdata = cls.testClient.getParsedTestDataConfig()
+        cls.services = cls.testClient.getParsedTestDataConfig()
+        cls.domain = get_domain(cls.apiclient)
+        zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.zone = Zone(zone.__dict__)
+        cls.template = get_template(cls.apiclient, cls.zone.id)
+        cls._cleanup = []
+
+        cls.logger = logging.getLogger("TestMulipleNetworkCreation")
+        cls.stream_handler = logging.StreamHandler()
+        cls.logger.setLevel(logging.DEBUG)
+        cls.logger.addHandler(cls.stream_handler)
+
+        # Disable the zone to create physical networks
+        cls.zone.update(
+            cls.apiclient,
+            allocationstate="Disabled"
+        )
+
+        try:
+            cls.physical_network = PhysicalNetwork.create(
+                cls.apiclient,
+                cls.services["l2-network"],
+                zoneid=cls.zone.id
+            )
+            cls._cleanup.append(cls.physical_network)
+
+            cls.physical_network_2 = PhysicalNetwork.create(
+                cls.apiclient,
+                cls.services["l2-network"],
+                zoneid=cls.zone.id
+            )
+            cls._cleanup.append(cls.physical_network_2)
+        except Exception as e:
+            cls.tearDownClass()
+            raise unittest.SkipTest(e)
+
+        cls.kvmnetworklabel=None
+        try:
+            hosts = Host.list(cls.apiclient, type='Routing')
+            if isinstance(hosts, list) and len(hosts) > 0:
+                host = hosts[0]
+            else:
+                return
+            if host.hypervisor.lower() not in "kvm":
+                return
+            host.user, host.passwd = get_host_credentials(cls.config, host.ipaddress)
+            bridges = _execute_ssh_command(host.ipaddress, 22, host.user, host.passwd, "brctl show |grep cloudbr |awk '{print $1}'")
+            existing_physical_networks = PhysicalNetwork.list(cls.apiclient)
+            for existing_physical_network in existing_physical_networks:
+                trafficTypes = TrafficType.list(
+                    cls.apiclient,
+                    physicalnetworkid=existing_physical_network.id)
+                if trafficTypes is None:
+                    continue
+                for traffic_type in trafficTypes:
+                    if traffic_type.traffictype == "Guest":
+                        try:
+                            for bridge in bridges:
+                                if bridge == str(traffic_type.kvmnetworklabel):
+                                    bridges.remove(bridge)
+                        except Exception as e:
+                            continue
+
+            if bridges is not None and len(bridges) > 0:
+                cls.kvmnetworklabel = bridges[0]
+            if cls.kvmnetworklabel is not None:
+                cls.logger.debug("Found an unused kvmnetworklabel %s" %cls.kvmnetworklabel)
+            else:
+                cls.logger.debug("Not find an unused kvmnetworklabel")
+        except Exception as e:
+            return
+
+    @classmethod
+    def tearDownClass(cls):
+        try:
+            cls.zone.update(
+                cls.apiclient,
+                allocationstate="Enabled"
+            )
+            # Cleanup resources used
+            super(TestMulipleNetworkCreation, cls).tearDownClass()
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.cleanup = []
+
+        return
+
+    def tearDown(self):
+        super(TestMulipleNetworkCreation, self).tearDown()
+
+    @attr(tags=["advanced"], required_hardware="false")
+    def test_01_add_traffictype_for_untagged_networks(self):
+        """
+        Try to add to traffic type Guest to each of the network
+        Two physical networks are already created without tags
+
+        1. Try to add traffic type Guest to physcial network
+        2. Ensure that it throws exception
+        3. Now add the tags to the physical network
+        4. Add traffic type Guest to the physical network and it should not throw exception
+        5. Do the same above steps for the other physical network
+
+        :return:
+        """
+        try:
+            # Add traffic type
+            self.physical_network.addTrafficType(
+                self.apiclient,
+                type="Guest"
+            )
+
+            # If the control comes then there is something wrong with the code.
+            # The code should throw exception as there are multiple networks with null tags
+            raise Exception("Exception should occur when adding traffic type since tags are null")
+        except Exception as e:
+            self.logger.info("Exception happened as expected")
+
+        # Now update the network with tags
+        self.physical_network.update(
+            self.apiclient,
+            tags="Guest"
+        )
+
+        # try adding traffic type again. it should not throw error
+        self.physical_network.addTrafficType(
+            self.apiclient,
+            type="Guest"
+        )
+
+        # Do the same thing for the second network
+        try:
+            self.physical_network_2.addTrafficType(
+                self.apiclient,
+                type="Guest"
+            )
+
+            # It should throw exception and should not come here
+            raise Exception("Exception should occur when adding traffic type since tags are null")
+        except Exception as e:
+            self.logger.info("Exception happened as expected")
+
+        # Now update the network with tags
+        self.physical_network_2.update(
+            self.apiclient,
+            tags="Guest"
+        )
+
+        # try adding traffic type again. it should not throw error
+        self.physical_network_2.addTrafficType(
+            self.apiclient,
+            type="Guest"
+        )
+
+    @attr(tags=["advanced"], required_hardware="false")
+    def test_02_created_shared_guest_network(self):
+        """
+        1. Create new physical network
+        2. Update the network with tags and traffic type "Guest"
+        3. Create a network offering and shared network based on the above physical network
+        4. Create a virtual machine using the above created network
+        4. Ensure that the traffic type is Guest and vlan is same as the shared network
+        :return:
+        """
+        #1. Create a physical network
+        self.physical_network_3 = PhysicalNetwork.create(
+            self.apiclient,
+            self.services["l2-network"],
+            isolationmethods="VLAN",
+            zoneid=self.zone.id
+        )
+        self.cleanup.append(self.physical_network_3)
+
+        # Enable the network
+        self.physical_network_3.update(
+            self.apiclient,
+            tags="guest",
+            state="Enabled"
+        )
+
+        #2. try adding traffic type Guest
+        TrafficType.add(
+            self.apiclient,
+            physicalnetworkid=self.physical_network_3.id,
+            kvmnetworklabel=self.kvmnetworklabel,
+            traffictype="Guest"
+        )
+
+        # Create network offering
+        self.services["network_offering_shared"]["supportedservices"] = ""
+        self.services["network_offering_shared"]["serviceProviderList"] = {}
+        self.services["network_offering_shared"]["tags"] = "guest"
+        self.network_offering = NetworkOffering.create(
+            self.apiclient,
+            self.services["network_offering_shared"]
+        )
+
+        # Enable network offering
+        self.network_offering.update(
+            self.apiclient,
+            state='Enabled'
+        )
+
+        #3. Create a shared network
+        self.shared_network = Network.create(
+            self.apiclient,
+            self.services["network2"],
+            networkofferingid=self.network_offering.id,
+            zoneid=self.zone.id,
+            domainid=self.domain.id
+            #physicalnetworkid=self.physical_network_3.id
+        )
+        self.cleanup.append(self.shared_network)
+
+        # Create small service offering
+        self.service_offering = ServiceOffering.create(
+            self.apiclient,
+            self.testdata["service_offerings"]["small"]
+        )
+        self.cleanup.append(self.service_offering)
+
+        #4. Create virtual machine
+        self.testdata["virtual_machine"]["zoneid"] = self.zone.id
+        self.testdata["virtual_machine"]["template"] = self.template.id
+        self.virtual_machine = VirtualMachine.create(
+            self.apiclient,
+            self.testdata["virtual_machine"],
+            templateid=self.template.id,
+            serviceofferingid=self.service_offering.id,
+            networkids=self.shared_network.id
+        )
+        self.cleanup.append(self.virtual_machine)
+
+        list_vms = VirtualMachine.list(
+            self.apiclient,
+            id=self.virtual_machine.id
+        )
+
+        vm = list_vms[0]
+        # make sure that vm uses the same vlan of the network
+        self.assertEqual(vm.nic[0].traffictype,
+                         "Guest",
+                         "Vm traffic type is not same")
+
+        self.assertEqual(vm.nic[0].isolationuri,
+                         "vlan://" + str(self.services["network2"]["vlan"]),
+                         "Vm network vlan is not same")
+
+        self.cleanup.append(self.virtual_machine)
+        self.cleanup.append(self.service_offering)
+        self.cleanup.append(self.shared_network)
+        self.cleanup.append(self.network_offering)
+        self.cleanup.append(self.physical_network_3)

Review Comment:
   ```suggestion
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1370755961

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] weizhouapache commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1369964422

   @blueorangutan package


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland merged pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland merged PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1262064296

   UI build: :heavy_check_mark:
   Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6781 (SL-JID-2437)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1262296189

   @soreana in the original PR it was asked to put this behind a setting by @andrijapanicsb . I do not see that implemented here yet. Are you planning to do that?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1273555077

   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4411


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] sureshanaparti commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r996051633


##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -6748,6 +6737,31 @@ public Pair<List<? extends NetworkOffering>, Integer> searchForNetworkOfferings(
         }
     }
 
+    private boolean allowNetworkOfferingWithNullTag(Long zoneId, List<String> pNtwkTags) {
+        boolean allowNullTag = false;
+        final List<PhysicalNetworkVO> pNtwks = _physicalNetworkDao.listByZoneAndTrafficType(zoneId, TrafficType.Guest);
+        for (final PhysicalNetworkVO pNtwk : pNtwks) {
+            final List<String> pNtwkTag = pNtwk.getTags();
+            if (pNtwkTag == null || pNtwkTag.isEmpty()) {

Review Comment:
   ```suggestion
               if (CollectionUtils.isEmpty(pNtwkTag)) {
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] JoaoJandre commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
JoaoJandre commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r995939043


##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -6748,6 +6737,31 @@ public Pair<List<? extends NetworkOffering>, Integer> searchForNetworkOfferings(
         }
     }
 
+    private boolean allowNetworkOfferingWithNullTag(Long zoneId, List<String> pNtwkTags) {
+        boolean allowNullTag = false;
+        final List<PhysicalNetworkVO> pNtwks = _physicalNetworkDao.listByZoneAndTrafficType(zoneId, TrafficType.Guest);

Review Comment:
   ```suggestion
       private boolean allowNetworkOfferingWithNullTag(Long zoneId, List<String> physicalNetworkTags) {
           boolean allowNullTag = false;
           final List<PhysicalNetworkVO> physicalNetworks = _physicalNetworkDao.listByZoneAndTrafficType(zoneId, TrafficType.Guest);
   ```
   I think we should make variable names readable and understandable to avoid confusion.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1278736935

   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4463


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1281303352

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6781)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL)
   
   [![4.7%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '4.7%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list) [4.7% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1281303008

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6781)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL)
   
   [![4.5%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '4.5%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list) [4.5% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1261843679

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r983546196


##########
test/integration/component/test_multiple_physical_network_creation.py:
##########
@@ -0,0 +1,410 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Import Local Modules
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase, unittest
+from marvin.lib.utils import (validateList,
+                              cleanup_resources,
+                              _execute_ssh_command,
+                              get_host_credentials,
+                              random_gen)
+from marvin.lib.base import (PhysicalNetwork,
+                             Host,
+                             TrafficType,
+                             Domain,
+                             Network,
+                             NetworkOffering,
+                             VirtualMachine,
+                             ServiceOffering,
+                             Zone)
+from marvin.lib.common import (get_domain,
+                               get_zone,
+                               get_template,
+                               list_virtual_machines)
+
+import logging
+
+class TestMulipleNetworkCreation(cloudstackTestCase):
+    @classmethod
+    def setUpClass(cls):
+        cls.testClient = super(
+            TestMulipleNetworkCreation,
+            cls).getClsTestClient()
+        cls.apiclient = cls.testClient.getApiClient()
+        cls.testdata = cls.testClient.getParsedTestDataConfig()
+        cls.services = cls.testClient.getParsedTestDataConfig()
+        cls.domain = get_domain(cls.apiclient)
+        zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.zone = Zone(zone.__dict__)
+        cls.template = get_template(cls.apiclient, cls.zone.id)
+        cls._cleanup = []
+
+        cls.logger = logging.getLogger("TestMulipleNetworkCreation")
+        cls.stream_handler = logging.StreamHandler()
+        cls.logger.setLevel(logging.DEBUG)
+        cls.logger.addHandler(cls.stream_handler)
+
+        # Disable the zone to create physical networks
+        cls.zone.update(
+            cls.apiclient,
+            allocationstate="Disabled"
+        )
+
+        try:
+            cls.physical_network = PhysicalNetwork.create(
+                cls.apiclient,
+                cls.services["l2-network"],
+                zoneid=cls.zone.id
+            )
+            cls._cleanup.append(cls.physical_network)
+
+            cls.physical_network_2 = PhysicalNetwork.create(
+                cls.apiclient,
+                cls.services["l2-network"],
+                zoneid=cls.zone.id
+            )
+            cls._cleanup.append(cls.physical_network_2)
+        except Exception as e:
+            cls.tearDownClass()
+            raise unittest.SkipTest(e)
+
+        cls.kvmnetworklabel=None
+        try:
+            hosts = Host.list(cls.apiclient, type='Routing')
+            if isinstance(hosts, list) and len(hosts) > 0:
+                host = hosts[0]
+            else:
+                return
+            if host.hypervisor.lower() not in "kvm":
+                return
+            host.user, host.passwd = get_host_credentials(cls.config, host.ipaddress)
+            bridges = _execute_ssh_command(host.ipaddress, 22, host.user, host.passwd, "brctl show |grep cloudbr |awk '{print $1}'")
+            existing_physical_networks = PhysicalNetwork.list(cls.apiclient)
+            for existing_physical_network in existing_physical_networks:
+                trafficTypes = TrafficType.list(
+                    cls.apiclient,
+                    physicalnetworkid=existing_physical_network.id)
+                if trafficTypes is None:
+                    continue
+                for traffic_type in trafficTypes:
+                    if traffic_type.traffictype == "Guest":
+                        try:
+                            for bridge in bridges:
+                                if bridge == str(traffic_type.kvmnetworklabel):
+                                    bridges.remove(bridge)
+                        except Exception as e:
+                            continue
+
+            if bridges is not None and len(bridges) > 0:
+                cls.kvmnetworklabel = bridges[0]
+            if cls.kvmnetworklabel is not None:
+                cls.logger.debug("Found an unused kvmnetworklabel %s" %cls.kvmnetworklabel)
+            else:
+                cls.logger.debug("Not find an unused kvmnetworklabel")
+        except Exception as e:
+            return
+
+    @classmethod
+    def tearDownClass(cls):
+        try:
+            cls.zone.update(
+                cls.apiclient,
+                allocationstate="Enabled"
+            )
+            # Cleanup resources used
+            super(TestMulipleNetworkCreation, cls).tearDownClass()
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.cleanup = []
+
+        return
+
+    def tearDown(self):
+        super(TestMulipleNetworkCreation, self).tearDown()
+
+    @attr(tags=["advanced"], required_hardware="false")
+    def test_01_add_traffictype_for_untagged_networks(self):
+        """
+        Try to add to traffic type Guest to each of the network
+        Two physical networks are already created without tags
+
+        1. Try to add traffic type Guest to physcial network
+        2. Ensure that it throws exception
+        3. Now add the tags to the physical network
+        4. Add traffic type Guest to the physical network and it should not throw exception
+        5. Do the same above steps for the other physical network
+
+        :return:
+        """
+        try:
+            # Add traffic type
+            self.physical_network.addTrafficType(
+                self.apiclient,
+                type="Guest"
+            )
+
+            # If the control comes then there is something wrong with the code.
+            # The code should throw exception as there are multiple networks with null tags
+            raise Exception("Exception should occur when adding traffic type since tags are null")
+        except Exception as e:
+            self.logger.info("Exception happened as expected")
+
+        # Now update the network with tags
+        self.physical_network.update(
+            self.apiclient,
+            tags="Guest"
+        )
+
+        # try adding traffic type again. it should not throw error
+        self.physical_network.addTrafficType(
+            self.apiclient,
+            type="Guest"
+        )
+
+        # Do the same thing for the second network
+        try:
+            self.physical_network_2.addTrafficType(
+                self.apiclient,
+                type="Guest"
+            )
+
+            # It should throw exception and should not come here
+            raise Exception("Exception should occur when adding traffic type since tags are null")
+        except Exception as e:
+            self.logger.info("Exception happened as expected")
+
+        # Now update the network with tags
+        self.physical_network_2.update(
+            self.apiclient,
+            tags="Guest"
+        )
+
+        # try adding traffic type again. it should not throw error
+        self.physical_network_2.addTrafficType(
+            self.apiclient,
+            type="Guest"
+        )
+
+    @attr(tags=["advanced"], required_hardware="false")
+    def test_02_created_shared_guest_network(self):
+        """
+        1. Create new physical network
+        2. Update the network with tags and traffic type "Guest"
+        3. Create a network offering and shared network based on the above physical network
+        4. Create a virtual machine using the above created network
+        4. Ensure that the traffic type is Guest and vlan is same as the shared network
+        :return:
+        """
+        #1. Create a physical network
+        self.physical_network_3 = PhysicalNetwork.create(
+            self.apiclient,
+            self.services["l2-network"],
+            isolationmethods="VLAN",
+            zoneid=self.zone.id
+        )
+        self.cleanup.append(self.physical_network_3)
+
+        # Enable the network
+        self.physical_network_3.update(
+            self.apiclient,
+            tags="guest",
+            state="Enabled"
+        )
+
+        #2. try adding traffic type Guest
+        TrafficType.add(
+            self.apiclient,
+            physicalnetworkid=self.physical_network_3.id,
+            kvmnetworklabel=self.kvmnetworklabel,
+            traffictype="Guest"
+        )
+
+        # Create network offering
+        self.services["network_offering_shared"]["supportedservices"] = ""
+        self.services["network_offering_shared"]["serviceProviderList"] = {}
+        self.services["network_offering_shared"]["tags"] = "guest"
+        self.network_offering = NetworkOffering.create(
+            self.apiclient,
+            self.services["network_offering_shared"]
+        )
+
+        # Enable network offering
+        self.network_offering.update(
+            self.apiclient,
+            state='Enabled'
+        )
+
+        #3. Create a shared network
+        self.shared_network = Network.create(
+            self.apiclient,
+            self.services["network2"],
+            networkofferingid=self.network_offering.id,
+            zoneid=self.zone.id,
+            domainid=self.domain.id
+            #physicalnetworkid=self.physical_network_3.id
+        )
+        self.cleanup.append(self.shared_network)
+
+        # Create small service offering
+        self.service_offering = ServiceOffering.create(
+            self.apiclient,
+            self.testdata["service_offerings"]["small"]
+        )
+        self.cleanup.append(self.service_offering)
+
+        #4. Create virtual machine
+        self.testdata["virtual_machine"]["zoneid"] = self.zone.id
+        self.testdata["virtual_machine"]["template"] = self.template.id
+        self.virtual_machine = VirtualMachine.create(
+            self.apiclient,
+            self.testdata["virtual_machine"],
+            templateid=self.template.id,
+            serviceofferingid=self.service_offering.id,
+            networkids=self.shared_network.id
+        )
+        self.cleanup.append(self.virtual_machine)
+
+        list_vms = VirtualMachine.list(
+            self.apiclient,
+            id=self.virtual_machine.id
+        )
+
+        vm = list_vms[0]
+        # make sure that vm uses the same vlan of the network
+        self.assertEqual(vm.nic[0].traffictype,
+                         "Guest",
+                         "Vm traffic type is not same")
+
+        self.assertEqual(vm.nic[0].isolationuri,
+                         "vlan://" + str(self.services["network2"]["vlan"]),
+                         "Vm network vlan is not same")
+
+        self.cleanup.append(self.virtual_machine)
+        self.cleanup.append(self.service_offering)
+        self.cleanup.append(self.shared_network)
+        self.cleanup.append(self.network_offering)
+        self.cleanup.append(self.physical_network_3)

Review Comment:
   sorry, I see a few already are and those should be removed here completely.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1262108141

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6781)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL)
   
   [![4.1%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '4.1%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list) [4.1% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1273060631

   @DaanHoogland Can I have this on 4.18, please ? :)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1276594087

   @andrijapanicsb @weizhouapache can you review this?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1280673013

   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 4483


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1278688185

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6781)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6781&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6781&resolved=false&types=CODE_SMELL)
   
   [![4.3%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '4.3%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list) [4.3% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6781&metric=new_duplicated_lines_density&view=list)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r996866313


##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -6748,6 +6737,31 @@ public Pair<List<? extends NetworkOffering>, Integer> searchForNetworkOfferings(
         }
     }
 
+    private boolean allowNetworkOfferingWithNullTag(Long zoneId, List<String> pNtwkTags) {
+        boolean allowNullTag = false;
+        final List<PhysicalNetworkVO> pNtwks = _physicalNetworkDao.listByZoneAndTrafficType(zoneId, TrafficType.Guest);
+        for (final PhysicalNetworkVO pNtwk : pNtwks) {
+            final List<String> pNtwkTag = pNtwk.getTags();
+            if (pNtwkTag == null || pNtwkTag.isEmpty()) {

Review Comment:
   Done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r996956458


##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -6748,6 +6737,31 @@ public Pair<List<? extends NetworkOffering>, Integer> searchForNetworkOfferings(
         }
     }
 
+    private boolean allowNetworkOfferingWithNullTag(Long zoneId, List<String> allPhysicalNetworkTags) {
+        boolean allowNullTag = false;
+        final List<PhysicalNetworkVO> physicalNetworks = _physicalNetworkDao.listByZoneAndTrafficType(zoneId, TrafficType.Guest);
+        for (final PhysicalNetworkVO physicalNetwork : physicalNetworks) {
+            final List<String> physicalNetworkTags = physicalNetwork.getTags();
+            if (CollectionUtils.isEmpty(physicalNetworkTags)) {
+                if (!allowNullTag) {
+                    allowNullTag = true;
+                } else {
+                    throw new CloudRuntimeException("There are more than 1 physical network with empty tag in the zone id=" + zoneId);
+                }
+            } else {
+                allPhysicalNetworkTags.addAll(physicalNetworkTags);
+            }
+        }
+        return allowNullTag;
+    }
+
+    private boolean checkNetworkOfferingTags(List<String> pNtwkTags, boolean allowNullTag, String offeringTags) {
+        if ((offeringTags == null && !allowNullTag) || (offeringTags != null && !pNtwkTags.contains(offeringTags))) {
+            return false;
+        }
+        return true;

Review Comment:
   ```suggestion
           return !((offeringTags == null && !allowNullTag) || (offeringTags != null && !pNtwkTags.contains(offeringTags)));
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1281204353

   @blueorangutan package


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on a diff in pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on code in PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#discussion_r991417119


##########
test/integration/component/test_multiple_physical_network_creation.py:
##########
@@ -0,0 +1,410 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Import Local Modules
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase, unittest
+from marvin.lib.utils import (validateList,
+                              cleanup_resources,
+                              _execute_ssh_command,
+                              get_host_credentials,
+                              random_gen)
+from marvin.lib.base import (PhysicalNetwork,
+                             Host,
+                             TrafficType,
+                             Domain,
+                             Network,
+                             NetworkOffering,
+                             VirtualMachine,
+                             ServiceOffering,
+                             Zone)
+from marvin.lib.common import (get_domain,
+                               get_zone,
+                               get_template,
+                               list_virtual_machines)
+
+import logging
+
+class TestMulipleNetworkCreation(cloudstackTestCase):
+    @classmethod
+    def setUpClass(cls):
+        cls.testClient = super(
+            TestMulipleNetworkCreation,
+            cls).getClsTestClient()
+        cls.apiclient = cls.testClient.getApiClient()
+        cls.testdata = cls.testClient.getParsedTestDataConfig()
+        cls.services = cls.testClient.getParsedTestDataConfig()
+        cls.domain = get_domain(cls.apiclient)
+        zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+        cls.zone = Zone(zone.__dict__)
+        cls.template = get_template(cls.apiclient, cls.zone.id)
+        cls._cleanup = []
+
+        cls.logger = logging.getLogger("TestMulipleNetworkCreation")
+        cls.stream_handler = logging.StreamHandler()
+        cls.logger.setLevel(logging.DEBUG)
+        cls.logger.addHandler(cls.stream_handler)
+
+        # Disable the zone to create physical networks
+        cls.zone.update(
+            cls.apiclient,
+            allocationstate="Disabled"
+        )
+
+        try:
+            cls.physical_network = PhysicalNetwork.create(
+                cls.apiclient,
+                cls.services["l2-network"],
+                zoneid=cls.zone.id
+            )
+            cls._cleanup.append(cls.physical_network)
+
+            cls.physical_network_2 = PhysicalNetwork.create(
+                cls.apiclient,
+                cls.services["l2-network"],
+                zoneid=cls.zone.id
+            )
+            cls._cleanup.append(cls.physical_network_2)
+        except Exception as e:
+            cls.tearDownClass()
+            raise unittest.SkipTest(e)
+
+        cls.kvmnetworklabel=None
+        try:
+            hosts = Host.list(cls.apiclient, type='Routing')
+            if isinstance(hosts, list) and len(hosts) > 0:
+                host = hosts[0]
+            else:
+                return
+            if host.hypervisor.lower() not in "kvm":
+                return
+            host.user, host.passwd = get_host_credentials(cls.config, host.ipaddress)
+            bridges = _execute_ssh_command(host.ipaddress, 22, host.user, host.passwd, "brctl show |grep cloudbr |awk '{print $1}'")
+            existing_physical_networks = PhysicalNetwork.list(cls.apiclient)
+            for existing_physical_network in existing_physical_networks:
+                trafficTypes = TrafficType.list(
+                    cls.apiclient,
+                    physicalnetworkid=existing_physical_network.id)
+                if trafficTypes is None:
+                    continue
+                for traffic_type in trafficTypes:
+                    if traffic_type.traffictype == "Guest":
+                        try:
+                            for bridge in bridges:
+                                if bridge == str(traffic_type.kvmnetworklabel):
+                                    bridges.remove(bridge)
+                        except Exception as e:
+                            continue
+
+            if bridges is not None and len(bridges) > 0:
+                cls.kvmnetworklabel = bridges[0]
+            if cls.kvmnetworklabel is not None:
+                cls.logger.debug("Found an unused kvmnetworklabel %s" %cls.kvmnetworklabel)
+            else:
+                cls.logger.debug("Not find an unused kvmnetworklabel")
+        except Exception as e:
+            return
+
+    @classmethod
+    def tearDownClass(cls):
+        try:
+            cls.zone.update(
+                cls.apiclient,
+                allocationstate="Enabled"
+            )
+            # Cleanup resources used
+            super(TestMulipleNetworkCreation, cls).tearDownClass()
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
+
+    def setUp(self):
+        self.apiclient = self.testClient.getApiClient()
+        self.cleanup = []
+
+        return
+
+    def tearDown(self):
+        super(TestMulipleNetworkCreation, self).tearDown()
+
+    @attr(tags=["advanced"], required_hardware="false")
+    def test_01_add_traffictype_for_untagged_networks(self):
+        """
+        Try to add to traffic type Guest to each of the network
+        Two physical networks are already created without tags
+
+        1. Try to add traffic type Guest to physcial network
+        2. Ensure that it throws exception
+        3. Now add the tags to the physical network
+        4. Add traffic type Guest to the physical network and it should not throw exception
+        5. Do the same above steps for the other physical network
+
+        :return:
+        """
+        try:
+            # Add traffic type
+            self.physical_network.addTrafficType(
+                self.apiclient,
+                type="Guest"
+            )
+
+            # If the control comes then there is something wrong with the code.
+            # The code should throw exception as there are multiple networks with null tags
+            raise Exception("Exception should occur when adding traffic type since tags are null")
+        except Exception as e:
+            self.logger.info("Exception happened as expected")
+
+        # Now update the network with tags
+        self.physical_network.update(
+            self.apiclient,
+            tags="Guest"
+        )
+
+        # try adding traffic type again. it should not throw error
+        self.physical_network.addTrafficType(
+            self.apiclient,
+            type="Guest"
+        )
+
+        # Do the same thing for the second network
+        try:
+            self.physical_network_2.addTrafficType(
+                self.apiclient,
+                type="Guest"
+            )
+
+            # It should throw exception and should not come here
+            raise Exception("Exception should occur when adding traffic type since tags are null")
+        except Exception as e:
+            self.logger.info("Exception happened as expected")
+
+        # Now update the network with tags
+        self.physical_network_2.update(
+            self.apiclient,
+            tags="Guest"
+        )
+
+        # try adding traffic type again. it should not throw error
+        self.physical_network_2.addTrafficType(
+            self.apiclient,
+            type="Guest"
+        )
+
+    @attr(tags=["advanced"], required_hardware="false")
+    def test_02_created_shared_guest_network(self):
+        """
+        1. Create new physical network
+        2. Update the network with tags and traffic type "Guest"
+        3. Create a network offering and shared network based on the above physical network
+        4. Create a virtual machine using the above created network
+        4. Ensure that the traffic type is Guest and vlan is same as the shared network
+        :return:
+        """
+        #1. Create a physical network
+        self.physical_network_3 = PhysicalNetwork.create(
+            self.apiclient,
+            self.services["l2-network"],
+            isolationmethods="VLAN",
+            zoneid=self.zone.id
+        )
+        self.cleanup.append(self.physical_network_3)
+
+        # Enable the network
+        self.physical_network_3.update(
+            self.apiclient,
+            tags="guest",
+            state="Enabled"
+        )
+
+        #2. try adding traffic type Guest
+        TrafficType.add(
+            self.apiclient,
+            physicalnetworkid=self.physical_network_3.id,
+            kvmnetworklabel=self.kvmnetworklabel,
+            traffictype="Guest"
+        )
+
+        # Create network offering
+        self.services["network_offering_shared"]["supportedservices"] = ""
+        self.services["network_offering_shared"]["serviceProviderList"] = {}
+        self.services["network_offering_shared"]["tags"] = "guest"
+        self.network_offering = NetworkOffering.create(
+            self.apiclient,
+            self.services["network_offering_shared"]
+        )
+
+        # Enable network offering
+        self.network_offering.update(
+            self.apiclient,
+            state='Enabled'
+        )
+
+        #3. Create a shared network
+        self.shared_network = Network.create(
+            self.apiclient,
+            self.services["network2"],
+            networkofferingid=self.network_offering.id,
+            zoneid=self.zone.id,
+            domainid=self.domain.id
+            #physicalnetworkid=self.physical_network_3.id
+        )
+        self.cleanup.append(self.shared_network)
+
+        # Create small service offering
+        self.service_offering = ServiceOffering.create(
+            self.apiclient,
+            self.testdata["service_offerings"]["small"]
+        )
+        self.cleanup.append(self.service_offering)
+
+        #4. Create virtual machine
+        self.testdata["virtual_machine"]["zoneid"] = self.zone.id
+        self.testdata["virtual_machine"]["template"] = self.template.id
+        self.virtual_machine = VirtualMachine.create(
+            self.apiclient,
+            self.testdata["virtual_machine"],
+            templateid=self.template.id,
+            serviceofferingid=self.service_offering.id,
+            networkids=self.shared_network.id
+        )
+        self.cleanup.append(self.virtual_machine)
+
+        list_vms = VirtualMachine.list(
+            self.apiclient,
+            id=self.virtual_machine.id
+        )
+
+        vm = list_vms[0]
+        # make sure that vm uses the same vlan of the network
+        self.assertEqual(vm.nic[0].traffictype,
+                         "Guest",
+                         "Vm traffic type is not same")
+
+        self.assertEqual(vm.nic[0].isolationuri,
+                         "vlan://" + str(self.services["network2"]["vlan"]),
+                         "Vm network vlan is not same")
+
+        self.cleanup.append(self.virtual_machine)
+        self.cleanup.append(self.service_offering)
+        self.cleanup.append(self.shared_network)
+        self.cleanup.append(self.network_offering)
+        self.cleanup.append(self.physical_network_3)

Review Comment:
   I've updated the test, apparently I forgot to remove these :-)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] soreana commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
soreana commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1370035126

   @DaanHoogland Can I have this on 4.18? ๐Ÿ™ˆ 
   There isn't any astounding comment. :)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [cloudstack] blueorangutan commented on pull request #6781: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6781:
URL: https://github.com/apache/cloudstack/pull/6781#issuecomment-1370028448

   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 5161


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org