Uh oh!
There was an error while loading. Please reload this page.
CLOUDSTACK-10102: New network type (L2) - #2281
Conversation
yadvr
commented
Oct 6, 2017
Did not test it, but good work @nvazquez |
blueorangutan
commented
Oct 6, 2017
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Oct 6, 2017
Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1135 |
3a9fb97 to
df47971Compareborisstoyanov
commented
Oct 10, 2017
@blueorangutan test |
blueorangutan
commented
Oct 10, 2017
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Oct 10, 2017
Trillian test result (tid-1573)
|
yadvr
left a comment
There was a problem hiding this comment.
Overall nice changes, I've not tested it though. Does it make sense to add some marvin test?
| nicSearch.and().op("ip4Address", nicSearch.entity().getIPv4Address(), SearchCriteria.Op.NNULL); | ||
| nicSearch.or("ip6Address", nicSearch.entity().getIPv6Address(), SearchCriteria.Op.NNULL); | ||
| nicSearch.cp(); | ||
| if (! network.getGuestType().equals(Network.GuestType.L2)) { |
| addr.setState(assign ? IpAddress.State.Allocated : IpAddress.State.Allocating); | ||
| if (vlanUse != VlanType.DirectAttached) { | ||
| if (assign && vlanUse != VlanType.DirectAttached) { |
There was a problem hiding this comment.
Could this cause a backward compatibility issue?
| } | ||
| } | ||
| $useVpc.css('display', 'inline-block'); | ||
| $supportedServices.css('display', 'inline-block'); | ||
| } | ||
| else if ($guestTypeField.val() == 'L2') { |
There was a problem hiding this comment.
Put } else if ... on same line as line 2413.
| } else if ($(this).val() == "Isolated") { | ||
| $form.find('.form-item[rel=specifyVlan]').find('input[type=checkbox]').removeAttr("disabled"); //make it editable | ||
| $form.find('.form-item[rel=isPersistent]').find('input[type=checkbox]').removeAttr("disabled"); | ||
| } else { //$(this).val() == "Isolated" | ||
| } else if ($(this).val() == "Isolated") { |
There was a problem hiding this comment.
Can both the cases for L2 and Isolated be collapsed, as both are using same code?
| @@ -3315,6 +3328,13 @@ | |||
| delete inputData.isPersistent; //if Persistent checkbox is unchecked, do not pass isPersistent parameter to API call since we need to keep API call's size as small as possible (p.s. isPersistent is defaulted as false at server-side) | |||
| } | |||
| } | |||
borisstoyanov
commented
Oct 18, 2017
@blueorangutan package |
blueorangutan
commented
Oct 18, 2017
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Oct 18, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1170 |
borisstoyanov
commented
Oct 18, 2017
@blueorangutan test |
blueorangutan
commented
Oct 18, 2017
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Oct 18, 2017
Trillian test result (tid-1595)
|
borisstoyanov
commented
Oct 19, 2017
@DaanHoogland I see this error here when listing hosts: Does this looks like a db upgrade failure in annotations PR to you? |
borisstoyanov
commented
Oct 19, 2017
@nvazquez can you please rebase and push |
df47971 to
78f1ecaComparenvazquez
commented
Oct 19, 2017
@borisstoyanov sure, done. |
borisstoyanov
commented
Oct 19, 2017
Thanks @nvazquez, @blueorangutan package |
blueorangutan
commented
Oct 19, 2017
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Oct 19, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1174 |
borisstoyanov
commented
Oct 19, 2017
@blueorangutan test |
blueorangutan
commented
Oct 19, 2017
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Oct 19, 2017
Trillian test result (tid-1598)
|
borisstoyanov
commented
Oct 20, 2017
@nvazquez can you please see the failing tests |
nvazquez
commented
Oct 20, 2017
@borisstoyanov sure, I'll work on fixing failures |
78f1eca to
9bad6fcCompare9bad6fc to
3e4a3eaComparefa8778f to
405ca44Comparenvazquez
commented
Dec 12, 2017
@rhtyd @borisstoyanov master rebased, Jenkins and Travis are green now |
yadvr
commented
Dec 12, 2017
Additional review from others request. |
blueorangutan
commented
Dec 12, 2017
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Dec 12, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1367 |
yadvr
commented
Dec 17, 2017
@blueorangutan package |
blueorangutan
commented
Dec 17, 2017
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Dec 17, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1394 |
nvazquez
commented
Dec 18, 2017
@blueorangutan test |
blueorangutan
commented
Dec 18, 2017
@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Dec 19, 2017
Trillian test result (tid-1821)
|
yadvr
commented
Dec 19, 2017
@nvazquez Tests LGTM, additional review requested following which this can be merged. |
nvazquez
commented
Dec 19, 2017
Thanks @rhtyd |
rafaelweingartner
left a comment
There was a problem hiding this comment.
@nvazquez I only have one or two remarks here.
Everything else seems to be ok
| nicSearch.and().op("ip4Address", nicSearch.entity().getIPv4Address(), SearchCriteria.Op.NNULL); | ||
| nicSearch.or("ip6Address", nicSearch.entity().getIPv6Address(), SearchCriteria.Op.NNULL); | ||
| nicSearch.cp(); | ||
| if (!network.getGuestType().equals(Network.GuestType.L2)) { |
There was a problem hiding this comment.
Can the method getGuestType return Null?
If not, the code is ok. Otherwise, it might be more interesting to do Network.GuestType.L2.equals(network.getGuestType())
Or even using ==, if it is an enum type, it is ok to use ==
| @Override | ||
| public Set<Long> getAvailableIps(Network network, String requestedIp) { | ||
| if (network.getCidr() == null) { | ||
| return null; |
There was a problem hiding this comment.
Instead of null, what about an empty set here?
nvazquez
commented
Dec 19, 2017
Thanks @rafaelweingartner, I pushed a commit with suggested improvements |
6748208 to
40fda18Comparenvazquez
commented
Dec 19, 2017
@blueorangutan package |
blueorangutan
commented
Dec 19, 2017
@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Dec 19, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1428 |
nvazquez
commented
Dec 19, 2017
@blueorangutan test |
blueorangutan
commented
Dec 19, 2017
@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Dec 20, 2017
Trillian test result (tid-1832)
|
yadvr
commented
Dec 20, 2017
Tests LGTM, others are known failures. This is ready for merging. |
JIRA Ticket: https://issues.apache.org/jira/browse/CLOUDSTACK-10102
Feature Specification: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=74680920
Introduction
This feature allows CloudStack administrators to create layer 2 networks on CloudStack. As these networks are purely layer 2, they don't require IP addresses or Virtual Router, only VLAN is necessary (provided by administrator or assigned by CloudStack). Also, network services should be handled externally, e.g. DNS, DHCP, as they are not provided by L2 networks.
As a consequence, a new Guest Network type is created within CloudStack: L2
Description
UI changes
A new button is added on Networks tab, available for admins, to allow L2 networks creation
