Broadcast URI not set to vxlan, but vlan (Fix #3040) by GabrielBrascher · Pull Request #4190 · apache/cloudstack

GabrielBrascher · 2020-06-30T17:25:31Z

Description

This PR sets properly Broadcast URI to vxlan://vxlan_id when the physical network is of VXLAN.

Description of issue #3040:

Advanced Network zone with VXLAN is created. When creating a guest network and setting the VNI this is set for the network:
broadcast_uri: vlan://1000
isolation_uri: vlan://1000

STEPS TO REPRODUCE
Create a Advanced Zone with VXLAN
Create a guest network
See that broadcast is set to vlan and not vxlan

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

Prior to the fix:

After the fix:

How Has This Been Tested?

Create a Advanced Zone with VXLAN
Create a guest network
See that broadcast uri is set to vxlan

yadvr · 2020-07-01T11:59:45Z

@blueorangutan package

blueorangutan · 2020-07-01T12:00:43Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-07-01T12:22:04Z

Packaging result: ✔centos7 ✔debian. JID-1498

GabrielBrascher · 2020-07-01T12:52:35Z

...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java

+     * @return Broadcast URI, e.g. 'vlan://vlan_ID' or 'vxlan://vlxan_ID'
+     */
+    protected URI encodeVlanIdIntoBroadcastUri(String vlanId, PhysicalNetwork pNtwk) {
+        if(StringUtils.isNotBlank(pNtwk.getIsolationMethods().get(0))) {


I got wondering if there could be more than one isolation method on a network. Considering that this is a List of isolation methods. I assume that to get into a VLAN conditional (the ones that this method is called) there will be only one isolation method, either VLAN or VXLAN.

yadvr · 2020-07-27T09:01:24Z

@blueorangutan package

blueorangutan · 2020-07-27T09:02:29Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-07-27T09:43:29Z

Packaging result: ✖centos7 ✔debian. JID-1604

yadvr · 2020-07-27T09:47:28Z

@blueorangutan package

blueorangutan · 2020-07-27T09:48:29Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-07-27T10:25:52Z

Packaging result: ✔centos7 ✔debian. JID-1608

wido · 2020-07-27T13:16:03Z

LGTM based on the code

yadvr · 2020-08-05T08:35:23Z

@GabrielBrascher did you try against 4.14, somehow I remember this was fixed recently? Thanks.
@blueorangutan package

blueorangutan · 2020-08-05T08:35:58Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-08-05T09:07:22Z

Packaging result: ✖centos7 ✔debian. JID-1657

blueorangutan · 2020-08-28T09:21:49Z

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

blueorangutan · 2020-08-29T06:49:15Z

Trillian test result (tid-2598)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 73635 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4190-t2598-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_iso.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermittent failure detected: /marvin/tests/smoke/test_reset_vm_on_reboot.py
Intermittent failure detected: /marvin/tests/smoke/test_resource_accounting.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dhcphosts.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dns.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dnsservice.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_iptables_default_policy.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_routers.py
Intermittent failure detected: /marvin/tests/smoke/test_secondary_storage.py
Intermittent failure detected: /marvin/tests/smoke/test_service_offerings.py
Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Intermittent failure detected: /marvin/tests/smoke/test_templates.py
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 54 look OK, 23 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80	`Failure`	495.14	test_internal_lb.py
test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80	`Failure`	609.05	test_internal_lb.py
test_03_vpc_internallb_haproxy_stats_on_all_interfaces	`Error`	413.72	test_internal_lb.py
test_04_rvpc_internallb_haproxy_stats_on_all_interfaces	`Error`	465.99	test_internal_lb.py
test_02_vpc_privategw_static_routes	`Failure`	302.73	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	340.93	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	433.66	test_privategw_acl.py
ContextSuite context=TestResetVmOnReboot>:setup	`Error`	0.00	test_reset_vm_on_reboot.py
ContextSuite context=TestRAMCPUResourceAccounting>:setup	`Error`	0.00	test_resource_accounting.py
ContextSuite context=TestRouterDHCPHosts>:setup	`Error`	0.00	test_router_dhcphosts.py
ContextSuite context=TestRouterDHCPOpts>:setup	`Error`	0.00	test_router_dhcphosts.py
ContextSuite context=TestRouterDns>:setup	`Error`	0.00	test_router_dns.py
test_01_sys_vm_start	`Failure`	0.11	test_secondary_storage.py
ContextSuite context=TestRouterDnsService>:setup	`Error`	0.00	test_router_dnsservice.py
ContextSuite context=TestRouterIpTablesPolicies>:setup	`Error`	0.00	test_routers_iptables_default_policy.py
ContextSuite context=TestVPCIpTablesPolicies>:setup	`Error`	0.00	test_routers_iptables_default_policy.py
ContextSuite context=TestIsolatedNetworks>:setup	`Error`	0.00	test_routers_network_ops.py
ContextSuite context=TestRedundantIsolateNetworks>:setup	`Error`	0.00	test_routers_network_ops.py
ContextSuite context=TestRouterServices>:setup	`Error`	0.00	test_routers.py
ContextSuite context=TestCpuCapServiceOfferings>:setup	`Error`	0.00	test_service_offerings.py
ContextSuite context=TestServiceOfferings>:setup	`Error`	0.34	test_service_offerings.py
ContextSuite context=TestSnapshotRootDisk>:setup	`Error`	0.00	test_snapshots.py
test_01_list_sec_storage_vm	`Failure`	0.11	test_ssvm.py
test_02_list_cpvm_vm	`Failure`	0.09	test_ssvm.py
test_03_ssvm_internals	`Failure`	0.08	test_ssvm.py
test_04_cpvm_internals	`Failure`	0.09	test_ssvm.py
test_05_stop_ssvm	`Failure`	0.10	test_ssvm.py
test_06_stop_cpvm	`Failure`	0.08	test_ssvm.py
test_07_reboot_ssvm	`Failure`	0.10	test_ssvm.py
test_08_reboot_cpvm	`Failure`	0.08	test_ssvm.py
test_09_destroy_ssvm	`Failure`	0.08	test_ssvm.py
test_10_destroy_cpvm	`Failure`	0.08	test_ssvm.py
test_02_create_template_with_checksum_sha1	`Error`	65.46	test_templates.py
test_03_create_template_with_checksum_sha256	`Error`	65.50	test_templates.py
test_04_create_template_with_checksum_md5	`Error`	65.57	test_templates.py
test_05_create_template_with_no_checksum	`Error`	65.52	test_templates.py
test_02_deploy_vm_from_direct_download_template	`Error`	1.19	test_templates.py
test_03_deploy_vm_wrong_checksum	`Error`	1.29	test_templates.py
ContextSuite context=TestTemplates>:setup	`Error`	17.16	test_templates.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestLBRuleUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestNatRuleUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestPublicIPUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestSnapshotUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestVmUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestVolumeUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestVpnUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=Test01DeployVM>:setup	`Error`	0.00	test_vm_life_cycle.py
ContextSuite context=Test02VMLifeCycle>:setup	`Error`	0.00	test_vm_life_cycle.py
test_14_secure_to_secure_vm_migration	`Error`	11.28	test_vm_life_cycle.py
test_15_secured_to_nonsecured_vm_migration	`Error`	74.06	test_vm_life_cycle.py
test_16_nonsecured_to_secured_vm_migration	`Error`	1.18	test_vm_life_cycle.py
ContextSuite context=TestVmSnapshot>:setup	`Error`	1.68	test_vm_snapshots.py
ContextSuite context=TestCreateVolume>:setup	`Error`	0.00	test_volumes.py
ContextSuite context=TestVolumes>:setup	`Error`	0.00	test_volumes.py
ContextSuite context=TestVPCRedundancy>:setup	`Error`	0.00	test_vpc_redundant.py
ContextSuite context=TestVPCNics>:setup	`Error`	0.00	test_vpc_router_nics.py
ContextSuite context=TestRVPCSite2SiteVpn>:setup	`Error`	0.00	test_vpc_vpn.py
ContextSuite context=TestVPCSite2SiteVPNMultipleOptions>:setup	`Error`	0.00	test_vpc_vpn.py
ContextSuite context=TestVpcRemoteAccessVpn>:setup	`Error`	0.00	test_vpc_vpn.py
ContextSuite context=TestVpcSite2SiteVpn>:setup	`Error`	0.00	test_vpc_vpn.py
test_disable_oobm_ha_state_ineligible	`Error`	1515.69	test_hostha_kvm.py

yadvr · 2020-08-29T09:28:20Z

@blueorangutan test

blueorangutan · 2020-08-29T09:28:53Z

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

yadvr · 2020-09-01T10:26:38Z

@blueorangutan package

blueorangutan · 2020-09-01T10:27:07Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-09-01T11:42:32Z

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-1870

yadvr · 2020-09-05T19:12:10Z

(centos8 pkg failure is expected as it's not supported on 4.13/4.14 and before)
@blueorangutan test

blueorangutan · 2020-09-05T19:13:33Z

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

blueorangutan · 2020-09-07T15:34:24Z

Trillian test result (tid-2680)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37095 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4190-t2680-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 75 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	305.66	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	204.71	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	323.17	test_privategw_acl.py
test_01_redundant_vpc_site2site_vpn	`Failure`	281.06	test_vpc_vpn.py

yadvr

LGTM, but need another pair of eyes or four

wido

LGTM based on code and feedback from @GabrielBrascher

yadvr · 2020-09-09T23:08:37Z

...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java

+     * @return Broadcast URI, e.g. 'vlan://vlan_ID' or 'vxlan://vlxan_ID'
+     */
+    protected URI encodeVlanIdIntoBroadcastUri(String vlanId, PhysicalNetwork pNtwk) {
+        if(StringUtils.isNotBlank(pNtwk.getIsolationMethods().get(0))) {


@GabrielBrascher Should we check if pNtwk is null? This could potentially cause a NPE; and the getIsolationMethods() is also a valid list with at least one item

@rhtyd thanks for the review :-)

Answering your question: I did not add null verification on the respective parameter due to the fact that both methods that call this one already validate the private network

the physical network pNtwk is validated at NetworkOrchestrator.java#L2196;

the other method that calls encodeVlanIdIntoBroadcastUri also validates the private network at this line NetworkOrchestrator.java#L2397.

However, considering that this method could be reused, I have no problem in adding null validations.

Actually, NetworkOrchestrator.java#L2397 is not exactly a null validation. I will add a null verification @rhtyd.

@GabrielBrascher please add the null validation, thnx

yadvr · 2020-09-16T07:50:39Z

@GabrielBrascher can you advise when this is ready for merging, thnx

GabrielBrascher · 2020-09-16T09:39:51Z

@rhtyd I am a bit offline this week due to vacation. I Will get back at 21st September. As soon as I get back I will add the null validation and update this PR. Thanks!

GabrielBrascher · 2020-09-21T13:49:24Z

@rhtyd updated the code with the null validation.

yadvr · 2020-09-21T14:00:55Z

@blueorangutan package

blueorangutan · 2020-09-21T14:02:07Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-09-21T14:22:57Z

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2056

DaanHoogland · 2020-09-22T08:14:16Z

@blueorangutan test

blueorangutan · 2020-09-22T08:15:10Z

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

blueorangutan · 2020-09-22T17:54:28Z

Trillian test result (tid-2828)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32434 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4190-t2828-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 76 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	213.08	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	230.40	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	309.05	test_privategw_acl.py

yadvr · 2020-09-24T05:39:28Z

Will merge and then need to kick tests on 4.14 and master

GabrielBrascher added type:bug component:networking labels Jun 30, 2020

GabrielBrascher self-assigned this Jun 30, 2020

GabrielBrascher added this to the 4.13.2.0 milestone Jun 30, 2020

Fix issue apache#3040: Broadcast URI not set to vxlan, but vlan

2c3a41c

GabrielBrascher force-pushed the broadcast-uri-vxlan-413 branch from 0f9efed to 2c3a41c Compare June 30, 2020 18:07

GabrielBrascher closed this Jun 30, 2020

GabrielBrascher reopened this Jun 30, 2020

GabrielBrascher commented Jul 1, 2020

View reviewed changes

GabrielBrascher closed this Jul 2, 2020

GabrielBrascher reopened this Jul 2, 2020

GabrielBrascher closed this Jul 8, 2020

GabrielBrascher reopened this Jul 8, 2020

GabrielBrascher modified the milestones: 4.13.2.0, 4.14.1.0 Jul 16, 2020

yadvr requested review from wido and yadvr September 9, 2020 11:28

yadvr approved these changes Sep 9, 2020

View reviewed changes

wido approved these changes Sep 9, 2020

View reviewed changes

yadvr reviewed Sep 9, 2020

View reviewed changes

Add PhysicalNetwork null validation on encodeVlanIdIntoBroadcastUri

13314d3

yadvr merged commit d6152b3 into apache:4.13 Sep 24, 2020

GabrielBrascher mentioned this pull request Nov 5, 2020

Failed to create Guest Network during basic installation #4450

Closed

DaanHoogland added the Severity:Minor label Nov 10, 2020

GabrielBrascher mentioned this pull request Jun 3, 2021

Error when 'createNetwork' with VXLAN: "VLAN requested but URI is not in the expected format" #5071

Closed

Conversation

GabrielBrascher commented Jun 30, 2020

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

yadvr commented Jul 1, 2020

Uh oh!

blueorangutan commented Jul 1, 2020

Uh oh!

blueorangutan commented Jul 1, 2020

Uh oh!

GabrielBrascher Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jul 27, 2020

Uh oh!

blueorangutan commented Jul 27, 2020

Uh oh!

blueorangutan commented Jul 27, 2020

Uh oh!

yadvr commented Jul 27, 2020

Uh oh!

blueorangutan commented Jul 27, 2020

Uh oh!

blueorangutan commented Jul 27, 2020

Uh oh!

wido commented Jul 27, 2020

Uh oh!

yadvr commented Aug 5, 2020

Uh oh!

blueorangutan commented Aug 5, 2020

Uh oh!

blueorangutan commented Aug 5, 2020

Uh oh!

blueorangutan commented Aug 28, 2020

Uh oh!

blueorangutan commented Aug 29, 2020

Uh oh!

yadvr commented Aug 29, 2020

Uh oh!

blueorangutan commented Aug 29, 2020

Uh oh!

yadvr commented Sep 1, 2020

Uh oh!

blueorangutan commented Sep 1, 2020

Uh oh!

blueorangutan commented Sep 1, 2020

Uh oh!

yadvr commented Sep 5, 2020

Uh oh!

blueorangutan commented Sep 5, 2020

Uh oh!

blueorangutan commented Sep 7, 2020

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

wido left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr commented Sep 16, 2020

Uh oh!

GabrielBrascher commented Sep 16, 2020

Uh oh!

GabrielBrascher commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GabrielBrascher commented Sep 21, 2020 •

edited

Loading