Enhancement: Allow creating atmost 1 physical network with null tag#3780
Enhancement: Allow creating atmost 1 physical network with null tag#3780ravening wants to merge 2 commits intoapache:mainfrom
Conversation
Currently, we can create multiple physical networks in the same traffic types with null tags. This feature will ensure that we can create atmost 1 physical network in the same traffic type with null tag. Every physical network should be associted 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
|
@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.... |
|
@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 @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? |
|
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? |
|
@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 |
|
@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? |
|
@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 @andrijapanicsb i follow your discussion guys 🎅 if i understand it correctly the PR does the following. before PR 🔕 after PR 🔔 is this correctly maybe i stand in the hose? |
yes thats correct |
|
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. |
@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. |
|
@svenvogel @andrijapanicsb @ravening 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. |
|
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. |
|
Let me test this on 4.13, I don't recall seeing that requirement/limitation @weizhouapache...? |
|
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)
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. |
@DaanHoogland in default installation (mostly used), there is only 1 physical network for guest networks (isolated/shared)
@DaanHoogland @andrijapanicsb 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. |
|
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 <notifications@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<#3780?email_source=notifications&email_token=ABJOT5CQ7A4JJFZWCGT7CMTQ434JNA5CNFSM4J54HLTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIPZ4HI#issuecomment-572497437>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABJOT5HIK75WJGZ47FOYKL3Q434JNANCNFSM4J54HLTA>.
|
|
@weizhouapache @andrijapanicsb @ravening @DaanHoogland some news here? how we can proceed here? |
|
@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. |
|
@svenvogel I did not review because of the functional argument going on. I have no issue, myself. |
|
@DaanHoogland @andrijapanicsb @weizhouapache @svenvogel - what's the consensus on this PR? Keep in 4.14 or move to 4.15? |
|
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 |
|
ok @andrijapanicsb , so @rhtyd @weizhouapache @svenvogel we go ahead with it. I'll review asap, as only Wei has done so so far. |
|
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✖centos6 ✔centos7 ✔debian. JID-712 |
|
@blueorangutan test |
|
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-854)
|
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
being pedantic (only about the java code). the code itself looks good.
|
@ravening @weizhouapache please remind me once we have 4.14 out. looks good but haven't tested. |
|
ping @ravening |
|
@ravening can you rebase, I'll revisit my review when done. |
|
Hi @ravening , can you rebase the PR with latest main. |
|
Hi @ravening can you please fix the conflicts? Is this PR ready for review? |
|
@ravening |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
some comments on tests house-keeping
| allocationstate="Enabled" | ||
| ) | ||
| # Cleanup resources used | ||
| cleanup_resources(cls.apiclient, cls._cleanup) |
There was a problem hiding this comment.
| cleanup_resources(cls.apiclient, cls._cleanup) | |
| super(TestMulipleNetworkCreation, cls).tearDownClass() |
| try: | ||
| cls.physical_network = PhysicalNetwork.create( | ||
| cls.apiclient, | ||
| cls.services["l2-network"], | ||
| zoneid=cls.zone.id | ||
| ) | ||
|
|
||
| cls.physical_network_2 = PhysicalNetwork.create( | ||
| cls.apiclient, | ||
| cls.services["l2-network"], | ||
| zoneid=cls.zone.id | ||
| ) | ||
| except Exception as e: | ||
| cls.tearDownClass() | ||
| raise unittest.SkipTest(e) | ||
|
|
||
| cls._cleanup.append(cls.physical_network) | ||
| cls._cleanup.append(cls.physical_network_2) |
There was a problem hiding this comment.
| try: | |
| cls.physical_network = PhysicalNetwork.create( | |
| cls.apiclient, | |
| cls.services["l2-network"], | |
| zoneid=cls.zone.id | |
| ) | |
| cls.physical_network_2 = PhysicalNetwork.create( | |
| cls.apiclient, | |
| cls.services["l2-network"], | |
| zoneid=cls.zone.id | |
| ) | |
| except Exception as e: | |
| cls.tearDownClass() | |
| raise unittest.SkipTest(e) | |
| cls._cleanup.append(cls.physical_network) | |
| cls._cleanup.append(cls.physical_network_2) | |
| 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) |
| try: | ||
| # Clean up | ||
| cleanup_resources(self.apiclient, self.cleanup) | ||
| except Exception as e: | ||
| raise Exception("Warning: Exception during cleanup : %s" % e) | ||
| return |
There was a problem hiding this comment.
| try: | |
| # Clean up | |
| cleanup_resources(self.apiclient, self.cleanup) | |
| except Exception as e: | |
| raise Exception("Warning: Exception during cleanup : %s" % e) | |
| return | |
| super(TestMulipleNetworkCreation, self).tearDown() |
| self.physical_network_3 = PhysicalNetwork.create( | ||
| self.apiclient, | ||
| self.services["l2-network"], | ||
| isolationmethods="VLAN", | ||
| zoneid=self.zone.id | ||
| ) |
There was a problem hiding this comment.
| self.physical_network_3 = PhysicalNetwork.create( | |
| self.apiclient, | |
| self.services["l2-network"], | |
| isolationmethods="VLAN", | |
| zoneid=self.zone.id | |
| ) | |
| 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) |
| 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 | ||
| ) |
There was a problem hiding this comment.
| 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.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) |
| self.service_offering = ServiceOffering.create( | ||
| self.apiclient, | ||
| self.testdata["service_offerings"]["small"] | ||
| ) |
There was a problem hiding this comment.
| self.service_offering = ServiceOffering.create( | |
| self.apiclient, | |
| self.testdata["service_offerings"]["small"] | |
| ) | |
| self.service_offering = ServiceOffering.create( | |
| self.apiclient, | |
| self.testdata["service_offerings"]["small"] | |
| ) | |
| self.cleanup.append(self.service_offering) |
| 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 | ||
| ) |
There was a problem hiding this comment.
| 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.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) |
|
Hi @${author}, your pull request has merge conflicts. Can you fix the conflicts and sync your branch with the base branch? |
|
@ravening |
11 participants
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
Types of changes
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
2 . Create another physical network without any tag
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
4 . Now add the tag to one of the physical network
5 . Now we can successfully add the traffic type
6 . If we try to update the network to delete the tags, it should fail because there is already another network without tag