adapt condition to use the correct letter for pvlan types by DK101010 · Pull Request #5194 · apache/cloudstack

DK101010 · 2021-07-08T13:52:19Z

Description

In the past, the broadcast uri was changed to an "i" for isolated, as well as c for community.

However, the ingest logic has not been adjusted, which means it's no longer possible to ingest a VM with a community
network.

For this reason I've adapted the condition and add correct pvlan type.

Important

All existing community network broadcast uri's in cloudstack before 4.15.1.0 must be updated manually or new created
because all community networks contains the letter i in the broadcast uri.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Tested in own VMware env.

import unmanagedinstance clusterid=b6dc2477-f3ec-43e2-a405-3605e29aaa42 name=ubuvtst6 displayname=ubuvtst6 projectid=fc61ebe1-6290-4100-98ca-18fb3d950af2 serviceofferingid=15b5681f-08c2-417a-a9ba-4480c229ec03 

{
  "accountid": "226c8611-ff24-11ea-9d07-005056827a97",
  "cmd": "org.apache.cloudstack.api.command.admin.vm.ImportUnmanagedInstanceCmd",
  "completed": "2021-07-15T09:42:19+0100",
  "created": "2021-07-15T09:42:05+0100",
  "jobid": "189390c5-6401-4e62-830f-7ba2b96c174f",
  "jobprocstatus": 0,
  "jobresult": {
    "virtualmachine": {
      "affinitygroup": [],
      "cpunumber": 1,
      "cpuspeed": 0,
      "created": "2021-07-15T09:42:19+0100",
      "details": {
        "cpuNumber": "1",
        "dataDiskController": "osdefault",
        "deployvm": "true",
        "memory": "1024",
        "nicAdapter": "Vmxnet3",
        "rootDiskController": "pvscsi"
      },
      "displayname": "ubuvtst6",
      "displayvm": true,
      "domain": "itelligence",
      "domainid": "05dfd7d7-5b11-4f3e-94e4-9569c95f75ee",
      "guestosid": "224af937-ff24-11ea-9d07-005056827a97",
      "haenable": false,
      "hostid": "73676017-b962-4942-bb4c-cf297634b212",
      "hostname": "iosvemt01.esx-tst.os.itelligence.de",
      "hypervisor": "VMware",
      "id": "c13c12de-aa87-413c-8fe3-895a7094af60",
      "instancename": "ubuvtst6",
      "isdynamicallyscalable": false,
      "memory": 1024,
      "name": "ubuvtst6",
      "nic": [
        {
          "broadcasturi": "vlan://289",
          "extradhcpoption": [],
          "id": "c5f110b3-cdc1-492d-be66-c313861942df",
          "isdefault": true,
          "isolationuri": "vlan://289",
          "macaddress": "00:50:56:92:12:41",
          "networkid": "c0816fba-bac0-46a7-b749-cc2c36e97f60",
          "networkname": "Frontend_289",
          "secondaryip": [],
          "traffictype": "Guest",
          "type": "L2"
        },
        {
          "broadcasturi": "pvlan://63-c289",
          "extradhcpoption": [],
          "id": "46820c91-c310-4b1e-9383-7a8036e97a8f",
          "isdefault": false,
          "isolationuri": "pvlan://63-c289",
          "macaddress": "02:00:39:f3:00:01",
          "networkid": "baad9713-c8a8-4f5f-8ea5-546190db4306",
          "networkname": "Backup_63-289",
          "secondaryip": [],
          "traffictype": "Guest",
          "type": "L2"
        }
      ],
      "osdisplayname": "Other Ubuntu (64-bit)",
      "ostypeid": "224af937-ff24-11ea-9d07-005056827a97",
      "passwordenabled": false,
      "project": "dev-01",
      "projectid": "fc61ebe1-6290-4100-98ca-18fb3d950af2",
      "rootdeviceid": 0,
      "rootdevicetype": "ROOT",
      "securitygroup": [],
      "serviceofferingid": "15b5681f-08c2-417a-a9ba-4480c229ec03",
      "serviceofferingname": "custom_std-ha-std_fat-fp-std",
      "state": "Running",
      "tags": [],
      "templatedisplaytext": "VM Import Default Template",
      "templateid": "59faead6-743e-442f-a964-ffb31da3e785",
      "templatename": "system-default-vm-import-dummy-template.iso",
      "userid": "22712228-ff24-11ea-9d07-005056827a97",
      "username": "admin",
      "zoneid": "de1c4b6f-a58b-46e6-9418-1fd55a9f022c",
      "zonename": "eu-de-vct5"
    }
  },
  "jobresultcode": 0,
  "jobresulttype": "object",
  "jobstatus": 1,
  "userid": "22712228-ff24-11ea-9d07-005056827a97"
}

shwstppr · 2021-07-12T05:52:47Z

                (Strings.isNullOrEmpty(network.getBroadcastUri().toString()) ||
-                        !networkBroadcastUri.equals(String.format("pvlan://%d-i%d", nic.getVlan(), nic.getPvlan())))) {
-            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("PVLAN of network(ID: %s) %s is found different from the VLAN of nic(ID: %s) pvlan://%d-i%d during VM import", network.getUuid(), networkBroadcastUri, nic.getNicId(), nic.getVlan(), nic.getPvlan()));
+                        !networkBroadcastUri.equals(String.format("pvlan://%d-%s%d", nic.getVlan(), nic.getPvlanType().charAt(0), nic.getPvlan())))) {


@DK101010 will case of pvlan type matter? Does nic.getPvlanType() always return lowercase string?

Hi @shwstppr yes, the types community and isolated are lowercase. I'm not quite sure if this will continue in the future. But I also didn't want to create pointless source code. That's why I left out for now

living dangerously, huh? as it is used twice anyway, better prepare it in a var above.

…gerImpl.java Co-authored-by: dahn <daan.hoogland@gmail.com>

shwstppr · 2021-07-14T08:58:59Z

@blueorangutan package

blueorangutan · 2021-07-14T09:00:03Z

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

blueorangutan · 2021-07-14T09:06:00Z

Packaging result: ✖️ el7 ✖️ el8 ✖️ debian. SL-JID 544

shwstppr

Other than the compilation issue, code changes are correct based on the premise that broadcast URI for PVLAN will be based on its type and the first character of the type string.

@DK101010 since we do not have any smoke to test VM import, it will be great if you can add API call/output for testing this change

…gerImpl.java Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

DK101010 · 2021-07-15T06:24:26Z

Other than the compilation issue, code changes are correct based on the premise that broadcast URI for PVLAN will be based on its type and the first character of the type string.

@DK101010 since we do not have any smoke to test VM import, it will be great if you can add API call/output for testing this change

Do you want this ?

import unmanagedinstance clusterid=b6dc2477-f3ec-43e2-a405-3605e29aaa42 name=VM-ac60ee4e-7231-4057-8ff6-40f0c9eacd04 displayname=VM-ac60ee4e-7231-4057-8ff6-40f0c9eacd04 serviceofferingid=35d68014-0257-4775-b256-49a009aa5750

{
  "accountid": "226c8611-ff24-11ea-9d07-005056827a97",
  "cmd": "org.apache.cloudstack.api.command.admin.vm.ImportUnmanagedInstanceCmd",
  "completed": "2021-07-15T07:18:39+0100",
  "created": "2021-07-15T07:18:36+0100",
  "jobid": "c07d365c-7057-4124-bf6b-ce4efbe2ca1b",
  "jobprocstatus": 0,
  "jobresult": {
    "virtualmachine": {
      "account": "admin",
      "affinitygroup": [],
      "cpunumber": 1,
      "cpuspeed": 0,
      "created": "2021-07-15T07:18:38+0100",
      "details": {
        "dataDiskController": "osdefault",
        "deployvm": "true",
        "nicAdapter": "E1000",
        "rootDiskController": "pvscsi"
      },
      "displayname": "VM-ac60ee4e-7231-4057-8ff6-40f0c9eacd04",
      "displayvm": true,
      "domain": "ROOT",
      "domainid": "226c46c2-ff24-11ea-9d07-005056827a97",
      "guestosid": "224af937-ff24-11ea-9d07-005056827a97",
      "haenable": false,
      "hostid": "73676017-b962-4942-bb4c-cf297634b212",
      "hostname": "iosvemt01.esx-tst.os.itelligence.de",
      "hypervisor": "VMware",
      "id": "93309b0b-8af7-4ad2-85ea-a7e492b9af2e",
      "instancename": "VM-ac60ee4e-7231-4057-8ff6-40f0c9eacd04",
      "isdynamicallyscalable": false,
      "memory": 1024,
      "name": "VM-ac60ee4e-7231-4057-8ff6-40f0c9eacd04",
      "nic": [
        {
          "broadcasturi": "vlan://2",
          "extradhcpoption": [],
          "id": "3d4d8f5e-2ce6-4575-91c8-9d367520d97c",
          "isdefault": true,
          "isolationuri": "vlan://2",
          "macaddress": "02:00:07:e5:00:60",
          "networkid": "85d27cc3-9184-4d7f-87c4-e5b08b2eb2cc",
          "networkname": "Guest",
          "secondaryip": [],
          "traffictype": "Guest",
          "type": "L2"
        }
      ],
      "osdisplayname": "Other Ubuntu (64-bit)",
      "ostypeid": "224af937-ff24-11ea-9d07-005056827a97",
      "passwordenabled": false,
      "rootdeviceid": 0,
      "rootdevicetype": "ROOT",
      "securitygroup": [],
      "serviceofferingid": "35d68014-0257-4775-b256-49a009aa5750",
      "serviceofferingname": "testCPU0Mhz",
      "state": "Running",
      "tags": [],
      "templatedisplaytext": "VM Import Default Template",
      "templateid": "59faead6-743e-442f-a964-ffb31da3e785",
      "templatename": "system-default-vm-import-dummy-template.iso",
      "userid": "22712228-ff24-11ea-9d07-005056827a97",
      "username": "admin",
      "zoneid": "de1c4b6f-a58b-46e6-9418-1fd55a9f022c",
      "zonename": "eu-de-vct5"
    }
  },
  "jobresultcode": 0,
  "jobresulttype": "object",
  "jobstatus": 1,
  "userid": "22712228-ff24-11ea-9d07-005056827a97"
}

shwstppr · 2021-07-15T08:09:17Z

@blueorangutan package

@DK101010 yes, will be great to have a list and import API call/output added in the PR's description. Though I feel the test you added above is not for PVLAN broadcast URI type.

blueorangutan · 2021-07-15T08:10:03Z

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

blueorangutan · 2021-07-15T08:31:48Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 553

DK101010 · 2021-07-15T08:49:26Z

@blueorangutan package

@DK101010 yes, will be great to have a list and import API call/output added in the PR's description. Though I feel the test you added above is not for PVLAN broadcast URI type.

@shwstppr you are completely right, wrong vm

import unmanagedinstance clusterid=b6dc2477-f3ec-43e2-a405-3605e29aaa42 name=ubuvtst6 displayname=ubuvtst6 projectid=fc61ebe1-6290-4100-98ca-18fb3d950af2 serviceofferingid=15b5681f-08c2-417a-a9ba-4480c229ec03

 
{
  "accountid": "226c8611-ff24-11ea-9d07-005056827a97",
  "cmd": "org.apache.cloudstack.api.command.admin.vm.ImportUnmanagedInstanceCmd",
  "completed": "2021-07-15T09:42:19+0100",
  "created": "2021-07-15T09:42:05+0100",
  "jobid": "189390c5-6401-4e62-830f-7ba2b96c174f",
  "jobprocstatus": 0,
  "jobresult": {
    "virtualmachine": {
      "affinitygroup": [],
      "cpunumber": 1,
      "cpuspeed": 0,
      "created": "2021-07-15T09:42:19+0100",
      "details": {
        "cpuNumber": "1",
        "dataDiskController": "osdefault",
        "deployvm": "true",
        "memory": "1024",
        "nicAdapter": "Vmxnet3",
        "rootDiskController": "pvscsi"
      },
      "displayname": "ubuvtst6",
      "displayvm": true,
      "domain": "itelligence",
      "domainid": "05dfd7d7-5b11-4f3e-94e4-9569c95f75ee",
      "guestosid": "224af937-ff24-11ea-9d07-005056827a97",
      "haenable": false,
      "hostid": "73676017-b962-4942-bb4c-cf297634b212",
      "hostname": "iosvemt01.esx-tst.os.itelligence.de",
      "hypervisor": "VMware",
      "id": "c13c12de-aa87-413c-8fe3-895a7094af60",
      "instancename": "ubuvtst6",
      "isdynamicallyscalable": false,
      "memory": 1024,
      "name": "ubuvtst6",
      "nic": [
        {
          "broadcasturi": "vlan://289",
          "extradhcpoption": [],
          "id": "c5f110b3-cdc1-492d-be66-c313861942df",
          "isdefault": true,
          "isolationuri": "vlan://289",
          "macaddress": "00:50:56:92:12:41",
          "networkid": "c0816fba-bac0-46a7-b749-cc2c36e97f60",
          "networkname": "Frontend_289",
          "secondaryip": [],
          "traffictype": "Guest",
          "type": "L2"
        },
        {
          "broadcasturi": "pvlan://63-c289",
          "extradhcpoption": [],
          "id": "46820c91-c310-4b1e-9383-7a8036e97a8f",
          "isdefault": false,
          "isolationuri": "pvlan://63-c289",
          "macaddress": "02:00:39:f3:00:01",
          "networkid": "baad9713-c8a8-4f5f-8ea5-546190db4306",
          "networkname": "Backup_63-289",
          "secondaryip": [],
          "traffictype": "Guest",
          "type": "L2"
        }
      ],
      "osdisplayname": "Other Ubuntu (64-bit)",
      "ostypeid": "224af937-ff24-11ea-9d07-005056827a97",
      "passwordenabled": false,
      "project": "dev-01",
      "projectid": "fc61ebe1-6290-4100-98ca-18fb3d950af2",
      "rootdeviceid": 0,
      "rootdevicetype": "ROOT",
      "securitygroup": [],
      "serviceofferingid": "15b5681f-08c2-417a-a9ba-4480c229ec03",
      "serviceofferingname": "custom_std-ha-std_fat-fp-std",
      "state": "Running",
      "tags": [],
      "templatedisplaytext": "VM Import Default Template",
      "templateid": "59faead6-743e-442f-a964-ffb31da3e785",
      "templatename": "system-default-vm-import-dummy-template.iso",
      "userid": "22712228-ff24-11ea-9d07-005056827a97",
      "username": "admin",
      "zoneid": "de1c4b6f-a58b-46e6-9418-1fd55a9f022c",
      "zonename": "eu-de-vct5"
    }
  },
  "jobresultcode": 0,
  "jobresulttype": "object",
  "jobstatus": 1,
  "userid": "22712228-ff24-11ea-9d07-005056827a97"
}

blueorangutan · 2021-07-15T08:50:03Z

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

shwstppr

LGTM based on code changes and test import reported. Packaging working fine now

blueorangutan · 2021-07-15T09:25:24Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 554

DaanHoogland

clgtm

nvazquez · 2021-07-20T02:36:15Z

@DK101010 can you please include a DB upgrade for older pvlans as mentioned in #5202?

upgrade path missing

DK101010 · 2021-07-21T06:14:08Z

@DK101010 can you please include a DB upgrade for older pvlans as mentioned in #5202?

Hi @nvazquez I have currently no clue how the cs upgrade works and have a couple of other stuff to do. Therefore I'm not sure if I get finished this until 1.15.2 release.

nvazquez · 2021-07-21T11:26:50Z

@DK101010 when upgrading from version X to Y, CloudStack will apply all the upgrade scripts from versions between X and Y (e.g. from 4.15.0 to 4.16.0 has to apply DB upgrades from intermediate version 4.15.1)

The upgrade scripts are executed in this way:

Prepare scripts: schema-XtoY.sql where X and Y are versions
Data migration scripts (if any, provided in the java classes UpgradeXtoY.java)
Cleanup scripts: schema-XtoY-cleanup.sql

In short, as this PR is going into 4.16 you can place the DB update SQL for older PVLANs in the file schema-41510to41600.sql (some examples in the same file: https://github.com/apache/cloudstack/blob/main/engine/schema/src/main/resources/META-INF/db/schema-41510to41600.sql#L396-L407)

Happy to help, please ping me if you need help with that

P.S: on the issue #5202 the SQL update is proposed:

UPDATE cloud.networks
SET broadcast_uri = REPLACE(broadcast_uri, '-i', '-c')
WHERE removed is null and broadcast_uri like '%pvlan%';

mib1185 · 2021-07-21T20:02:15Z

P.S: on the issue #5202 the SQL update is proposed:

UPDATE cloud.networks
SET broadcast_uri = REPLACE(broadcast_uri, '-i', '-c')
WHERE removed is null and broadcast_uri like '%pvlan%';

@nvazquez Please don't oversee the remarks before and after the SQL in #5202:

In our case - since all pvlan are of type community - we simple updated all entries in DB with following SQL statement:

But to be more generic, it is needed that the correct pvlan type is gathered from infrastructure (ex. VMware portgroup) and corresponding DB entry is proper updated.

Those I guess it is unfortunately not done by a simple sql update script 🤔

nvazquez · 2021-07-21T21:55:18Z

Thanks @mib1185 good remarks, I'll check further cc. @davidjumani

mib1185 · 2021-07-21T22:03:18Z

There can only be one isolated secondary vlan per each primary vlan, but multiple community secondary vlan.
Furthermore, promiscuous typed networks needs also to be checked 🤔

nvazquez

Code LGTM

yadvr · 2021-08-03T05:25:42Z

cc @davidjumani

yadvr · 2021-08-09T08:36:03Z

@nvazquez @shwstppr @davidjumani - can you discuss and check with @DK101010 if it's possible to support backward compatibility or automatic fix post-upgrade?

davidjumani · 2021-08-17T09:02:35Z

@rhtyd That's being tracked here #5202

nvazquez · 2021-08-19T05:03:56Z

@blueorangutan package

blueorangutan · 2021-08-19T05:05:03Z

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

blueorangutan · 2021-08-19T05:34:36Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 926

nvazquez · 2021-08-19T11:25:45Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-08-19T11:26:03Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

blueorangutan · 2021-08-19T22:21:44Z

Trillian test result (tid-1723)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 38084 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5194-t1723-vmware-67u3.zip
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

yadvr · 2021-08-20T11:45:46Z

Ping @davidjumani can you review this - I think you worked on pvlan related feature so you may have some ideas to review/test this? cc @nvazquez

nvazquez · 2021-08-20T12:50:47Z

@rhtyd as discussed in #5202 this PR is good to go, but needs a separate PR to address the upgrade for older pvlans (DB upgrade will not work)

adapt condition to use the correct letter for pvlan types

054f698

yadvr requested review from davidjumani and shwstppr July 8, 2021 15:38

shwstppr reviewed Jul 12, 2021

View reviewed changes

changed to var and add lower case

4db48f7

DaanHoogland reviewed Jul 13, 2021

View reviewed changes

Comment thread server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java Outdated

Update server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsMana…

475ec04

…gerImpl.java Co-authored-by: dahn <daan.hoogland@gmail.com>

mib1185 mentioned this pull request Jul 13, 2021

Addition to #5194 - update of broadcast_uri for all existing pvlan networks is necessary during upgrade to 4.15.x #5202

Closed

shwstppr requested changes Jul 14, 2021

View reviewed changes

Comment thread server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java Outdated

Update server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsMana…

00050b5

…gerImpl.java Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr approved these changes Jul 15, 2021

View reviewed changes

DaanHoogland previously approved these changes Jul 16, 2021

View reviewed changes

shwstppr requested a review from nvazquez July 16, 2021 08:09

sureshanaparti added this to the 4.15.2.0 milestone Jul 19, 2021

nvazquez approved these changes Aug 2, 2021

View reviewed changes

yadvr modified the milestones: 4.15.2.0, 4.16.0.0 Aug 20, 2021

nvazquez merged commit 1b041ee into apache:main Aug 20, 2021

nvazquez added component:advanced-networking type:bug labels Oct 14, 2021

Conversation

DK101010 commented Jul 8, 2021 • edited by shwstppr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Important

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

shwstppr Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

DK101010 Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shwstppr commented Jul 14, 2021

Uh oh!

blueorangutan commented Jul 14, 2021

Uh oh!

blueorangutan commented Jul 14, 2021

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DK101010 commented Jul 15, 2021

Uh oh!

shwstppr commented Jul 15, 2021

Uh oh!

blueorangutan commented Jul 15, 2021

Uh oh!

blueorangutan commented Jul 15, 2021

Uh oh!

DK101010 commented Jul 15, 2021

Uh oh!

blueorangutan commented Jul 15, 2021

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 15, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Jul 20, 2021

Uh oh!

DK101010 commented Jul 21, 2021

Uh oh!

nvazquez commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mib1185 commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvazquez commented Jul 21, 2021

Uh oh!

mib1185 commented Jul 21, 2021

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Aug 3, 2021

Uh oh!

yadvr commented Aug 9, 2021

Uh oh!

davidjumani commented Aug 17, 2021

Uh oh!

nvazquez commented Aug 19, 2021

Uh oh!

blueorangutan commented Aug 19, 2021

Uh oh!

blueorangutan commented Aug 19, 2021

DK101010 commented Jul 8, 2021 •

edited by shwstppr

Loading

nvazquez commented Jul 21, 2021 •

edited

Loading

mib1185 commented Jul 21, 2021 •

edited

Loading