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 2019/12/20 10:48:30 UTC

[GitHub] [cloudstack] rakgenius opened a new pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag

rakgenius opened a new pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780
 
 
   ## Description
   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
   
   <!-- 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: # -->
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
   - [ ] 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)
   
   ## Screenshots (if appropriate):
   
   ## How Has This Been Tested?
   
   This feature has been tested through cloudmonkey commands
   
   First we need to ensure that there is a spare NIC in the hypervisor on which we can create a new
   physical network.
   
   We are going to create a bridge with name "cloudbr9" on this new interface
   
   
   1 . Create a physical network
   ```
   create physicalnetwork zoneid=76aecb3c-c246-4cf7-9410-1116596a26f7 isolationmethods=VLAN name=TEST
   ```
   
   2 . Create another physical network without any tag
   ```
   create physicalnetwork zoneid=76aecb3c-c246-4cf7-9410-1116596a26f7 isolationmethods=VLAN name=TEST-2
   ```
   
   3 . Now try to add traffic type to any of the physical network created above. This should fail since there are more than 1 physical network without any tags
   ```
   add traffictype physicalnetworkid=fed2091b-9db9-40be-be32-4bdc2c4cdd23 traffictype=Guest kvmnetworklabel=cloudbr9
   
   
   Error 530: There are more than 1 physical network without tags in the zone= 1
   ```
   
   4 . Now add the tag to one of the physical network
   ```
   update physicalnetwork id=fed2091b-9db9-40be-be32-4bdc2c4cdd23 tags=NGN
   ```
   
   5 . Now we can successfully add the traffic type
   ```
    add traffictype physicalnetworkid=fed2091b-9db9-40be-be32-4bdc2c4cdd23 traffictype=Guest kvmnetworklabel=cloudbr9
   
   
   cmd = org.apache.cloudstack.api.command.admin.usage.AddTrafficTypeCmd
   jobid = 15afffb2-c600-428a-a691-a7767a145e84
   jobinstanceid = 3995aec3-1cea-4f57-bce2-c99e34e54298
   jobinstancetype = TrafficType
   jobprocstatus = 0
   jobresult:
   traffictype:
   id = 3995aec3-1cea-4f57-bce2-c99e34e54298
   kvmnetworklabel = cloudbr9
   physicalnetworkid = fed2091b-9db9-40be-be32-4bdc2c4cdd23
   traffictype = Guest
   jobresultcode = 0
   jobresulttype = object
   jobstatus = 1
   ```
   
   6 .  If we try to update the network to delete the tags, it should fail because there is already another network without tag
   
   ```
   update physicalnetwork id=fed2091b-9db9-40be-be32-4bdc2c4cdd23 tags=Error
   
   Async job 7d1e9b8a-95d1-4dd3-be3d-125e314a6bd5 failed
   Error 530, There are more than 1 physical network without tags in the zone= 1
   
   cmd = org.apache.cloudstack.api.command.admin.network.UpdatePhysicalNetworkCmd
   jobid = 7d1e9b8a-95d1-4dd3-be3d-125e314a6bd5
   jobprocstatus = 0
   jobresult:
   errorcode = 530
   errortext = There are more than 1 physical network without tags in the zone= 1
   jobresultcode = 530
   jobresulttype = object
   jobstatus = 2
   ```
   
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-580391757
 
 
   <b>Trillian test result (tid-854)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 28067 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3780-t854-kvm-centos7.zip
   Smoke tests completed. 78 look OK, 0 have error(s)
   Only failed 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] svenvogel commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

svenvogel commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568736744
 
 
   @rakgenius @andrijapanicsb i follow your discussion guys 🎅 
   
   if i understand it correctly the PR does the following.
   
   before PR 🔕 
   N physical network without tag -> 1 network Offering with tag (tag has no influence) - 1:N relation only possible
   
   after PR 🔔 
   N physical networks with tag -> N network offering with tag - 1:1 relation possible
   
   is this correctly maybe i stand in the hose? 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] svenvogel commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

svenvogel commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568761697
 
 
   >@andrijapanicsb yes you can create additional physical network without tags but this PR ensures that there will be a max of 1 physical network without any tag in particular traffic type. As of today, I guess in a particular traffic type there can be multiple physical networks in same traffic type without tags.
   
   @rakgenius i see there not a really problem. the PR only limits the creating of 1 additional physical network without tag. there are more interesting question. **if there is a scenario where we want to create a second physical network without tag?** what i see i can create an additional one but not add a traffic flag which is needed for the usage.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] weizhouapache commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

weizhouapache commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-572497437
 
 
   @svenvogel @andrijapanicsb @ravening 
   This PR aims the fix the issue in following scenario. 
   
   Suppose we have a physical network (without tag, name=PUB) and some network offerings (without tag, eg name=Network-offering)
   
   Now, we want to add another physical network (PUB-NEW), and some network offerings (eg offering-NEW). To make it work, we have to tag all physical networks (PUB, PUB-NEW) and network offerings (Network-offering, Network-offering-NEW). 
   
   With this patch, we only need to tag the new physical network (PUB-NEW) and new network offerings (eg Network-offering-NEW). We do NOT need to tag/change the existing physical network (PUB) and network offering (eg Network-offering). The networks created from network offering without tag will be allocated to the physical network without tag (it means, at most 1 physical network is not tagged).
   
   I hope it is clear.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] weizhouapache commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

weizhouapache commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-572758112
 
 
   > clear to me @weizhouapache, however a user would still want to be able to have all networks allowing all traffic in some installations, would you agree? So a global setting to enforce this would be nice.
   
   @DaanHoogland in default installation (mostly used), there is only 1 physical network for guest networks (isolated/shared)
   
   > Thanks for the explanation @weizhouapache - this should be in the description of the PR in the first place :)
   > 
   > I've repeated commands that @ravening used to create 2 physical networks and attempt (and succeeded) to create a Guest traffic type on a specific Physical network (in clean 4.5 and clean 4.13)
   > 
   > On the other hand, with this clean (zero-tags-anywhere) setup (clean 4.5 or clean 4.13) - I can see the following "warning" in the API (same one in the GUI)
   > 
   > > (localcloud) SBCM5> > create  network networkofferingid=471039c4-d0f9-4105-a5d8-2e58551bf445 zoneid=450c5085-8832-46af-a60f-109a25af293a name=yyy displaytext==yyy
   > > ** Error: (HTTP 431, error code 4350) More than one physical networks exist in zone id=1 and no tags are specified in order to make a choice**
   > 
   > So all checks are in place to ensure that an operator configures things "correctly".
   > 
   > Perhaps it's just me, but I don't see a value in the behaviour that this PR brings - it allows you to have at most 1 untagged physical network/offering combination - but why (what does it solve)? All one needs to do is to tag both the physical network and the offering and be 100% clear on what is going to be created where (update tags in DB).
   > 
   > As @DaanHoogland pointed out, the operator might have a valid case where he wants both (more than one) physical networks untagged.
   > So, can we please have a global configuration setting - that would make more sense (defaulting to the old behaviour)?
   
   @DaanHoogland @andrijapanicsb 
   in current cloudstack implementation, if there are multiple (>=2) physical networks for guest networks (isolated/shared), all MUST be tagged (network offerings as well). Othewise cloudstack is not able to make decision which physical network will be used. That's same as the error message you got in your testing. 
   
   I think it makes sense. As a cloudstack user, when I create a guest network, I am very clear which physical network will be used. I do not think It is a good idea to let cloudstack choose a physical network from a list.
   
   Maybe we are one of few cloudstack user who use multiple physical networks. On each hypervisor, there is a default interface for guest networks (which connect internet via core switches). meanwhile, there is another interface to connect to other dedicated racks in our data center which make us to build hybrid cloud very easily.
   
   In default cloudstack setup , there is 1 physical network and some network offerings. all are not tagged. when we add new physical networks, we have to tag all physical networks and offerings, including the existing. as I said in previous comment, with this patch, we only need to tag the new things. It make our changes a bit easier.
   
   
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] weizhouapache commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

weizhouapache commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-576861771
 
 
   @svenvogel yes ,this change will not break anything. I agree it is better to have it.
   
   ps: The tags of physical network and network offering can also be changed via api.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] rakgenius commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

rakgenius commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568719886
 
 
   @andrijapanicsb in 4.7, we were able to create multiple physical networks without any tag in the same traffic type (storage/managment/guest). When we create a network offering with some tag, it should match to the physical network with the same tag. This is what we were trying to achieve.
   
   If thats already solved in 4.13, then its good enough and I can close this 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] DaanHoogland commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-580183590
 
 
   @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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#discussion_r373133772
 
 

 ##########
 File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
 ##########
 @@ -5506,7 +5512,9 @@ protected void validateNtwkOffDetails(final Map<Detail, String> details, final M
                 List<Service> checkForProviders = new ArrayList<Service>();
 
                 if (checkForTags) {
-                    if (!pNtwkTags.contains(offering.getTags())) {
+                    if (offering.getTags() == null && !allowNullTag) {
+                        continue;
+                    } else if (offering.getTags() != null && !pNtwkTags.contains(offering.getTags())) {
 
 Review comment:
   method call

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-579892534
 
 
   After reading of Wei's explanation, it's became actually clear how it works, and I don't see it will break anything, but it would be good to document the behaviour in the docs. 
   
   LGTM

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#discussion_r373134659
 
 

 ##########
 File path: server/src/main/java/com/cloud/network/NetworkServiceImpl.java
 ##########
 @@ -2993,6 +2993,20 @@ public PhysicalNetwork updatePhysicalNetwork(Long id, String networkSpeed, List<
             throw new InvalidParameterException("Unable to support more than one tag on network yet");
         }
 
+        // If tags are null, then check if there are any other networks with null tags
+        // of the same traffic type. If so then dont update the tags
+        if (tags != null && tags.size() == 0) {
+            // Get all physical networks according to traffic type
+            Pair<List<PhysicalNetworkTrafficTypeVO>, Integer> result = _pNTrafficTypeDao
+                    .listAndCountBy(network.getId());
+            if (result.second() > 0) {
+                for (PhysicalNetworkTrafficTypeVO physicalNetworkTrafficTypeVO : result.first()) {
+                    TrafficType trafficType = physicalNetworkTrafficTypeVO.getTrafficType();
+                    checkForPhysicalNetworksWithoutTag(network, trafficType);
+                }
+            }
+        }
 
 Review comment:
   method call

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568722806
 
 
   @rakgenius currently you can still create additional physical network, and then the Guest traffic in this physical network, without tags. But you have to define tags on the network offering and the physical network, so that should solve your issue if i understand correctly? 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#discussion_r383183411
 
 

 ##########
 File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
 ##########
 @@ -5471,9 +5472,14 @@ protected void validateNtwkOffDetails(final Map<Detail, String> details, final M
                 for (final PhysicalNetworkVO pNtwk : pNtwks) {
                     final List<String> pNtwkTag = pNtwk.getTags();
                     if (pNtwkTag == null || pNtwkTag.isEmpty()) {
-                        throw new CloudRuntimeException("Tags are not defined for physical network in the zone id=" + zoneId);
+                        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);
 
 Review comment:
   yes @ravening, methods seem to grow out of hand in our project so any addition of more than one line I'd like to see in a separate method. As said I'm being pedantic, and so open for discussion, but as a matter of principle and keeping in mind that this is inherited code that will be passed on to posterity.... 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] svenvogel commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

svenvogel commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-572776529

Hi @weizhouapache,

For me it’s now clear.

One question from me. What I see is that all untagged offerings will be always as they are untagged assigned to the untagged one. Right? Tagged means I can use tags and fit the Network to the offer I want on the physical network.

For me it’s an extension they don’t break anything. You are right! Normal admin will not affected because he can use only physical network untagged with all offerings without the need of an tag. But later if anyone get an more complex setup he need to edit the database and that’s always not an good idea.
I think it is as you said if anyone has a more complex network like another rack or hybrid cloud he will need that.

just by the way, now we could talk again about why is it so. Hey CS is a old lady and we don’t want to break things but we need to go forward, fix things, make them better and bring new features. This is like for me make them better.

For me it’s an good solution if anyone gets an more complex setup but don’t want to edit the DB. This is always a no go. Normally ;)))

Sven Vogel
Teamlead Platform

EWERK DIGITAL GmbH
Brühl 24, D-04109 Leipzig
P +49 341 42649 - 99
F +49 341 42649 - 98
S.Vogel@ewerk.com
www.ewerk.com

Geschäftsführer:
Dr. Erik Wende, Hendrik Schubert, Frank Richter
Registergericht: Leipzig HRB 9065

Support:
+49 341 42649 555

Zertifiziert nach:
ISO/IEC 27001:2013
DIN EN ISO 9001:2015
DIN ISO/IEC 20000-1:2011

ISAE 3402 Typ II Assessed

EWERK-Blog<https://blog.ewerk.com/> | LinkedIn<https://www.linkedin.com/company/ewerk-group> | Xing<https://www.xing.com/company/ewerk> | Twitter<https://twitter.com/EWERK_Group> | Facebook<https://de-de.facebook.com/EWERK.IT/>

Mit Handelsregistereintragung vom 09.07.2019 ist die EWERK RZ GmbH auf die EWERK IT GmbH verschmolzen und firmiert nun gemeinsam unter dem Namen: EWERK DIGITAL GmbH, für weitere Informationen klicken Sie hier<https://www.ewerk.com/ewerkdigital>.

Auskünfte und Angebote per Mail sind freibleibend und unverbindlich.

Disclaimer Privacy:
Der Inhalt dieser E-Mail (einschließlich etwaiger beigefügter Dateien) ist vertraulich und nur für den Empfänger bestimmt. Sollten Sie nicht der bestimmungsgemäße Empfänger sein, ist Ihnen jegliche Offenlegung, Vervielfältigung, Weitergabe oder Nutzung des Inhalts untersagt. Bitte informieren Sie in diesem Fall unverzüglich den Absender und löschen Sie die E-Mail (einschließlich etwaiger beigefügter Dateien) von Ihrem System. Vielen Dank.

The contents of this e-mail (including any attachments) are confidential and may be legally privileged. If you are not the intended recipient of this e-mail, any disclosure, copying, distribution or use of its contents is strictly prohibited, and you should please notify the sender immediately and then delete it (including any attachments) from your system. Thank you.

Am 09.01.2020 um 11:32 schrieb weizhouapache <no...@github.com>:

@svenvogel<https://github.com/svenvogel> @andrijapanicsb<https://github.com/andrijapanicsb> @ravening<https://github.com/ravening>
This PR aims the fix the issue in following scenario.

Suppose we have a physical network (without tag, name=PUB) and some network offerings (without tag, eg name=Network-offering)

Now, we want to add another physical network (PUB-NEW), and some network offerings (eg offering-NEW). To make it work, we have to tag all physical networks (PUB, PUB-NEW) and network offerings (Network-offering, Network-offering-NEW).

With this patch, we only need to tag the new physical network (PUB-NEW) and new network offerings (eg Network-offering-NEW). We do NOT need to tag/change the existing physical network (PUB) and network offering (eg Network-offering). The networks created from network offering without tag will be allocated to the physical network without tag (it means, at most 1 physical network is not tagged).

I hope it is clear.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/apache/cloudstack/pull/3780?email_source=notifications&email_token=ABJOT5CQ7A4JJFZWCGT7CMTQ434JNA5CNFSM4J54HLTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIPZ4HI#issuecomment-572497437>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABJOT5HIK75WJGZ47FOYKL3Q434JNANCNFSM4J54HLTA>.

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

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

With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-580183781
 
 
   @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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] rakgenius commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

rakgenius commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568745769
 
 
   r
   
   > @rakgenius @andrijapanicsb i follow your discussion guys 🎅
   > 
   > if i understand it correctly the PR does the following.
   > 
   > before PR 🔕
   > N physical network without tag -> 1 network Offering with tag (tag has no influence) - 1:N relation only possible
   > 
   > after PR 🔔
   > N physical networks with tag -> N network offering with tag - 1:1 relation possible
   > 
   > is this correctly maybe i stand in the hose?
   
   yes thats correct

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] rakgenius commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

rakgenius commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568723816
 
 
   @andrijapanicsb yes you can create additional physical network without tags but this PR ensures that there will be a max of 1 physical network without any tag in particular traffic type. As of today, I guess in a particular traffic type there can be multiple physical networks in same traffic type without 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#discussion_r373135040
 
 

 ##########
 File path: server/src/main/java/com/cloud/network/NetworkServiceImpl.java
 ##########
 @@ -3915,12 +3929,15 @@ public long findPhysicalNetworkId(long zoneId, String tag, TrafficType trafficTy
         }
 
         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");
-            }
-
             Long pNtwkId = null;
             for (PhysicalNetwork pNtwk : pNtwks) {
+                if (pNtwk.getTags().isEmpty() && tag == null) {
+                    s_logger.debug("Found physical network id=" + pNtwk.getId() + " with null tag");
 
 Review comment:
   if isdebugenabled

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] DaanHoogland commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-572704250
 
 
   clear to me @weizhouapache, however a user would still want to be able to have all networks allowing all traffic in some installations, would you agree? So a global setting to enforce this would be nice.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] rakgenius edited a comment on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

rakgenius edited a comment on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568745769
 
 
   > @rakgenius @andrijapanicsb i follow your discussion guys 🎅
   > 
   > if i understand it correctly the PR does the following.
   > 
   > before PR 🔕
   > N physical network without tag -> 1 network Offering with tag (tag has no influence) - 1:N relation only possible
   > 
   > after PR 🔔
   > N physical networks with tag -> N network offering with tag - 1:1 relation possible
   > 
   > is this correctly maybe i stand in the hose?
   
   yes thats correct

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] DaanHoogland commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568691387
 
 
   @rakgenius I can see how you want this. Can you answer @andrijapanicsb on to why enforce this and if you guys agree, can we hide this behind a global - or zone-wide setting?
   @andrijapanicsb, I'll code-review if we have functional agreement.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] rakgenius commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

rakgenius commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568713816
 
 
   @andrijapanicsb @DaanHoogland Your point sounds valid. I can hide this behind a global setting or completely remove this feature if this is of no use to anyone. If I hide this feature behind the global setting, then I need to add lot of if conditions to see if this has to be enforced or not.
   
   @ustcweizhou do you have any other solution?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-572745727
 
 
   Thanks for the explanation @weizhouapache - this should be in the description of the PR in the first place :)
   
   I've repeated commands that @ravening used to create 2 physical networks and attempt (and succeeded) to create a Guest traffic type on a specific Physical network (in clean 4.5 and clean 4.13)
   
   On the other hand, with this clean (zero-tags-anywhere) setup (clean 4.5 or clean 4.13)  - I can see the following "warning" in the API (same one in the GUI)
   
   > (localcloud) SBCM5> > create  network networkofferingid=471039c4-d0f9-4105-a5d8-2e58551bf445 zoneid=450c5085-8832-46af-a60f-109a25af293a name=yyy displaytext==yyy
   ** Error: (HTTP 431, error code 4350) More than one physical networks exist in zone id=1 and no tags are specified in order to make a choice**
   
   So all checks are in place to ensure that an operator configures things "correctly".
   
   Perhaps it's just me, but I don't see a value in the behaviour that this PR brings - it allows you to have at most 1 untagged physical network/offering combination - but why (what does it solve)?  All one needs to do is to tag both the physical network and the offering and be 100% clear on what is going to be created where (update tags in DB).
   
   As @DaanHoogland pointed out, the operator might have a valid case where he wants both (more than one) physical networks untagged.
    So, can we please have a global configuration setting - that would make more sense (defaulting to the old behaviour)?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568716299
 
 
   in 4.13 at least (or was it master actually) you can't create Guest network if you have 2 physical networks both carrying Guest traffic and if tags are not in use (i.e. the network offering requires a tag). This solves the "guest network placements on a particular physical network" problem I believe? 
   
   I'm struggling to see the use case for these changes @rakgenius - what problem is this trying to solve?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] rhtyd commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

rhtyd commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-579800108
 
 
   @DaanHoogland @andrijapanicsb @weizhouapache @svenvogel - what's the consensus on this PR? Keep in 4.14 or move to 4.15?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] ravening commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

ravening commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#discussion_r383180974
 
 

 ##########
 File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
 ##########
 @@ -5471,9 +5472,14 @@ protected void validateNtwkOffDetails(final Map<Detail, String> details, final M
                 for (final PhysicalNetworkVO pNtwk : pNtwks) {
                     final List<String> pNtwkTag = pNtwk.getTags();
                     if (pNtwkTag == null || pNtwkTag.isEmpty()) {
-                        throw new CloudRuntimeException("Tags are not defined for physical network in the zone id=" + zoneId);
+                        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);
 
 Review comment:
   you mean, move it to a method?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] DaanHoogland commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-580146886
 
 
   ok @andrijapanicsb , so @rhtyd @weizhouapache @svenvogel we go ahead with it. I'll review asap, as only Wei has done so so far.
   @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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-572727742
 
 
   Let me test this on 4.13, I don't recall seeing that requirement/limitation @weizhouapache...? 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#discussion_r373135619
 
 

 ##########
 File path: server/src/main/java/com/cloud/network/NetworkServiceImpl.java
 ##########
 @@ -3983,6 +4024,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) {
+            checkForPhysicalNetworksWithoutTag(network, trafficType);
+        }
 
 Review comment:
   method call

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568044158
 
 
   @radeksm I can see edge cases where this constraint can be useful (operator lack of understanding how things should be done/tagged), but otherwise, I don't see the usability of this in the wider sense - why at all putting such constraint? Apologies if I'm missing something....

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#discussion_r373133652
 
 

 ##########
 File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
 ##########
 @@ -5471,9 +5472,14 @@ protected void validateNtwkOffDetails(final Map<Detail, String> details, final M
                 for (final PhysicalNetworkVO pNtwk : pNtwks) {
                     final List<String> pNtwkTag = pNtwk.getTags();
                     if (pNtwkTag == null || pNtwkTag.isEmpty()) {
-                        throw new CloudRuntimeException("Tags are not defined for physical network in the zone id=" + zoneId);
+                        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);
 
 Review comment:
   method call

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-580160967
 
 
   Packaging result: ✖centos6 ✔centos7 ✔debian. JID-712

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#discussion_r373133907
 
 

 ##########
 File path: server/src/main/java/com/cloud/network/NetworkModelImpl.java
 ##########
 @@ -1154,13 +1154,15 @@ public long findPhysicalNetworkId(long zoneId, String tag, TrafficType trafficTy
         }
 
         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");
-            }
-
             Long pNtwkId = null;
             for (PhysicalNetwork pNtwk : pNtwks) {
+                if (pNtwk.getTags().isEmpty() && tag == null) {
+                    s_logger.debug("Found physical network id=" + pNtwk.getId() + " with null tag");
+                    if (pNtwkId != null) {
+                        throw new CloudRuntimeException("There is more than 1 physical network with empty tag in the zone id=" + zoneId);
+                    }
+                    pNtwkId = pNtwk.getId();
+                }
 
 Review comment:
   method call

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] svenvogel commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

svenvogel commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-576691676
 
 
   @weizhouapache @andrijapanicsb @ravening @DaanHoogland some news here? how we can proceed here?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] DaanHoogland commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-577305105
 
 
   @svenvogel I did not review because of the functional argument going on. I have no issue, myself.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#discussion_r373134826
 
 

 ##########
 File path: server/src/main/java/com/cloud/network/NetworkServiceImpl.java
 ##########
 @@ -3915,12 +3929,15 @@ public long findPhysicalNetworkId(long zoneId, String tag, TrafficType trafficTy
         }
 
         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");
-            }
-
             Long pNtwkId = null;
             for (PhysicalNetwork pNtwk : pNtwks) {
+                if (pNtwk.getTags().isEmpty() && tag == null) {
+                    s_logger.debug("Found physical network id=" + pNtwk.getId() + " with null tag");
+                    if (pNtwkId != null) {
+                        throw new CloudRuntimeException("There is more than 1 physical network with empty tag in the zone id=" + zoneId);
+                    }
+                    pNtwkId = pNtwk.getId();
+                }
 
 Review comment:
   method call

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

andrijapanicsb commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-568748913
 
 
   I don't like imposing a limitation (unless its controlled via global setting and defaults to the old behaviour) - as it seems to me here we are trying to avoid an operator configuration error by imposing a hardcoded limit.
   
   The correct scenario can be created by paying attention to the configuration of physical network, various traffic and network offering. 
   
   If one doesn't tag his offerings and physical networks, perhaps he has the reason to do so, or it is his job to understand what he is doing. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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

Posted by GitBox <gi...@apache.org>.

DaanHoogland commented on a change in pull request #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#discussion_r373134535
 
 

 ##########
 File path: server/src/main/java/com/cloud/network/NetworkModelImpl.java
 ##########
 @@ -1154,13 +1154,15 @@ public long findPhysicalNetworkId(long zoneId, String tag, TrafficType trafficTy
         }
 
         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");
-            }
-
             Long pNtwkId = null;
             for (PhysicalNetwork pNtwk : pNtwks) {
+                if (pNtwk.getTags().isEmpty() && tag == null) {
+                    s_logger.debug("Found physical network id=" + pNtwk.getId() + " with null tag");
 
 Review comment:
   a string concal in a log message, can you surround with if(isDebugEnabled()){}

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [cloudstack] blueorangutan commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag

Posted by GitBox <gi...@apache.org>.

blueorangutan commented on issue #3780: Enhancement: Allow creating atmost 1 physical network with null tag
URL: https://github.com/apache/cloudstack/pull/3780#issuecomment-580147146
 
 
   @DaanHoogland a Jenkins job has been kicked to build packages. 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services