Display vlan ip range for specified domainid by ravening · Pull Request #4634 · apache/cloudstack

ravening · 2021-02-01T10:59:37Z

Description

Currently if we try to list vlan ip range by passing
domainid then it lists for all domain. Make sure that
it lists only for that domain

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):

Before the changes

(local) 🐱 > list vlanipranges domainid=4aa4c-f694-45c0-8844-8d9a129
{
  "count": 14,
  "vlaniprange": [

After the changes

(local) 🐱 > list vlanipranges domainid=4560c-f694-45c0-8844-8d94129
{
  "count": 1,
  "vlaniprange": [

How Has This Been Tested?

DaanHoogland · 2021-02-01T11:13:29Z

@ravening, do you intend this for master/4.16 only or do you want this on older versions?

ravening · 2021-02-01T11:14:22Z

@ravening, do you intend this for master/4.16 only or do you want this on older versions?

@DaanHoogland 4.16 is good

DaanHoogland

no comment on this PR but all this DAO code really shouldn't be in the ManagementServer

yadvr · 2021-06-17T07:41:53Z

ping @ravening

ravening · 2021-07-30T07:25:48Z

no comment on this PR but all this DAO code really shouldn't be in the ManagementServer

@DaanHoogland any idea where else I can add this ?

DaanHoogland · 2021-07-30T15:46:47Z

no comment on this PR but all this DAO code really shouldn't be in the ManagementServer

@DaanHoogland any idea where else I can add this ?

DomainVlanMapDaoImpl, i'd say.

DaanHoogland · 2021-07-30T15:47:55Z

@ravening you have a new conflict

nvazquez · 2021-08-12T03:38:19Z

@ravening can you move the logic to the DAO layer and fix the conflicts?

ravening · 2021-08-18T08:29:01Z

no comment on this PR but all this DAO code really shouldn't be in the ManagementServer

@DaanHoogland any idea where else I can add this ?

DomainVlanMapDaoImpl, i'd say.

@DaanHoogland Im using the same code which is used by account and pod also which are present above and below of this code. So even they needs to be moved away?

I searched in other places in the same file and they are also using the same logic. for eg: publicipaddress, loadbalancer, resourcetag, vlan, guestos

DaanHoogland · 2021-08-18T13:59:01Z

no comment on this PR but all this DAO code really shouldn't be in the ManagementServer

@DaanHoogland any idea where else I can add this ?

DomainVlanMapDaoImpl, i'd say.

@DaanHoogland Im using the same code which is used by account and pod also which are present above and below of this code. So even they needs to be moved away?

I searched in other places in the same file and they are also using the same logic. for eg: publicipaddress, loadbalancer, resourcetag, vlan, guestos

@ravening the rest of the code is not perfect and should be in DAOs as well. I'm not asking that of you now as I don't expect you to fix everything in one go, just to not add to the technical debt we have.

yadvr · 2021-09-14T06:25:54Z

Ping @ravening can you address review and fix conflicts, thanks

weizhouapache · 2021-09-14T06:57:21Z

no comment on this PR but all this DAO code really shouldn't be in the ManagementServer

@DaanHoogland any idea where else I can add this ?

DomainVlanMapDaoImpl, i'd say.

@DaanHoogland Im using the same code which is used by account and pod also which are present above and below of this code. So even they needs to be moved away?
I searched in other places in the same file and they are also using the same logic. for eg: publicipaddress, loadbalancer, resourcetag, vlan, guestos

@ravening the rest of the code is not perfect and should be in DAOs as well. I'm not asking that of you now as I don't expect you to fix everything in one go, just to not add to the technical debt we have.

@DaanHoogland

there are more than ten Dao.search() or Dao.searchAndCount() in ManagementServerImpl.java, and much more in QueryManagerImpl.java
we do not need to move everything into DAO, especially for the searches in multiple (more than 2) tables.

ravening · 2021-09-14T07:27:10Z

Ping @ravening can you address review and fix conflicts, thanks

@rhtyd done

DaanHoogland · 2021-09-15T08:19:14Z

no comment on this PR but all this DAO code really shouldn't be in the ManagementServer

@DaanHoogland any idea where else I can add this ?

DomainVlanMapDaoImpl, i'd say.

@DaanHoogland Im using the same code which is used by account and pod also which are present above and below of this code. So even they needs to be moved away?
I searched in other places in the same file and they are also using the same logic. for eg: publicipaddress, loadbalancer, resourcetag, vlan, guestos

@ravening the rest of the code is not perfect and should be in DAOs as well. I'm not asking that of you now as I don't expect you to fix everything in one go, just to not add to the technical debt we have.

@DaanHoogland

there are more than ten Dao.search() or Dao.searchAndCount() in ManagementServerImpl.java, and much more in QueryManagerImpl.java
we do not need to move everything into DAO, especially for the searches in multiple (more than 2) tables.

no, but those single table transactions that can we should.

weizhouapache · 2021-09-15T08:24:25Z

no comment on this PR but all this DAO code really shouldn't be in the ManagementServer

@DaanHoogland any idea where else I can add this ?

DomainVlanMapDaoImpl, i'd say.

@DaanHoogland Im using the same code which is used by account and pod also which are present above and below of this code. So even they needs to be moved away?
I searched in other places in the same file and they are also using the same logic. for eg: publicipaddress, loadbalancer, resourcetag, vlan, guestos

@ravening the rest of the code is not perfect and should be in DAOs as well. I'm not asking that of you now as I don't expect you to fix everything in one go, just to not add to the technical debt we have.

@DaanHoogland
there are more than ten Dao.search() or Dao.searchAndCount() in ManagementServerImpl.java, and much more in QueryManagerImpl.java
we do not need to move everything into DAO, especially for the searches in multiple (more than 2) tables.

no, but those single table transactions that can we should.

@DaanHoogland agree .

as you can see in the code changes of this pr, the search on vlans has some join tables (DomainVlanMapDao, AccountVlanMapDao, PodVlanMapDao)

Currently if we try to list vlan ip range by passing domainid then it lists for all domain. Make sure that it lists only for that domain

nvazquez · 2021-09-16T04:28:45Z

@DaanHoogland @weizhouapache let's get a consensus on this PR to try moving it forward as it seems the requested changes may not be applied. Would that be a reason for a -1 on this PR for any of you?
@blueorangutan package

blueorangutan · 2021-09-16T04:29:03Z

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

yadvr · 2021-09-16T05:30:00Z

If it's fixing a bug then +1 (sounds like you can't list Vlans for a domain?) but this would need testing

blueorangutan · 2021-09-16T05:46:36Z

Packaging result: ✖️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1282

davidjumani · 2021-09-16T05:51:12Z

@blueorangutan test suse15 kvm-suse15

blueorangutan · 2021-09-16T05:52:03Z

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

davidjumani

Verified, only returns for the respective domainid if passed and empty set if not reserved

weizhouapache

code lgtm

nvazquez · 2021-09-16T19:19:14Z

@blueorangutan package

blueorangutan · 2021-09-16T19:20:03Z

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

blueorangutan · 2021-09-16T19:46:02Z

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

sureshanaparti · 2021-09-16T20:06:28Z

@blueorangutan test

blueorangutan · 2021-09-16T20:07:03Z

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

nvazquez

LGTM

nvazquez · 2021-09-17T03:25:34Z

Manually tested + 2xLGTM - ready for merge after tests pass

blueorangutan · 2021-09-17T06:25:45Z

Trillian test result (tid-2098)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35205 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4634-t2098-kvm-centos7.zip
Smoke tests completed. 84 look OK, 5 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_nic	`Error`	135.77	test_nic.py
test_01_add_primary_storage_disabled_host	`Error`	0.57	test_primary_storage.py
test_01_primary_storage_nfs	`Error`	0.12	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.21	test_primary_storage.py
test_02_list_snapshots_with_removed_data_store	`Error`	1.17	test_snapshots.py
test_01_secure_vm_migration	`Error`	156.38	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	271.97	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	148.02	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	43.69	test_vm_life_cycle.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	305.13	test_hostha_kvm.py

weizhouapache · 2021-09-17T06:52:21Z

@blueorangutan test

blueorangutan · 2021-09-17T06:53:03Z

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

DaanHoogland · 2021-09-17T09:43:57Z

-0 if we allow for a mix of concerns in manager implementations we will increase the number of code conflict we will get over time. We should strive for better separation as much as possible. I'm not going to block this simple fix over this but it is an increasing concern. Calls to DAOs could have been made.

blueorangutan · 2021-09-17T17:13:09Z

Trillian test result (tid-2107)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35409 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4634-t2107-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

sureshanaparti · 2021-09-20T05:05:59Z

-0 if we allow for a mix of concerns in manager implementations we will increase the number of code conflict we will get over time. We should strive for better separation as much as possible. I'm not going to block this simple fix over this but it is an increasing concern. Calls to DAOs could have been made.

@DaanHoogland want to add this to cwiki / issues (as improvement) to track it later?

sureshanaparti · 2021-09-20T05:07:02Z

Manually tested + 2xLGTM - ready for merge after tests pass

Merging based on LGTMs, and manual / smoke tests.

DaanHoogland reviewed Feb 1, 2021

View reviewed changes

shwstppr added this to the 4.16.0.0 milestone Feb 8, 2021

ravening force-pushed the list-domain-vlan branch from a4586bf to f06cca6 Compare August 18, 2021 08:30

ravening force-pushed the list-domain-vlan branch from f06cca6 to fc121ab Compare September 14, 2021 07:19

ravening and others added 2 commits September 15, 2021 12:31

Display vlan ip range for specified domainid

9437b50

Currently if we try to list vlan ip range by passing domainid then it lists for all domain. Make sure that it lists only for that domain

orgazine imports

9e322c2

ravening force-pushed the list-domain-vlan branch from fc121ab to 9e322c2 Compare September 15, 2021 10:42

nvazquez added the status:needs-testing label Sep 16, 2021

davidjumani removed the status:needs-testing label Sep 16, 2021

davidjumani approved these changes Sep 16, 2021

View reviewed changes

weizhouapache approved these changes Sep 16, 2021

View reviewed changes

weizhouapache closed this Sep 16, 2021

weizhouapache reopened this Sep 16, 2021

nvazquez added the type:bug label Sep 16, 2021

nvazquez approved these changes Sep 17, 2021

View reviewed changes

sureshanaparti merged commit 3b4523f into apache:main Sep 20, 2021

Conversation

ravening commented Feb 1, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

DaanHoogland commented Feb 1, 2021

Uh oh!

ravening commented Feb 1, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jun 17, 2021

Uh oh!

ravening commented Jul 30, 2021

Uh oh!

DaanHoogland commented Jul 30, 2021

Uh oh!

DaanHoogland commented Jul 30, 2021

Uh oh!

nvazquez commented Aug 12, 2021

Uh oh!

ravening commented Aug 18, 2021

Uh oh!

DaanHoogland commented Aug 18, 2021

Uh oh!

yadvr commented Sep 14, 2021

Uh oh!

weizhouapache commented Sep 14, 2021

Uh oh!

ravening commented Sep 14, 2021

Uh oh!

DaanHoogland commented Sep 15, 2021

Uh oh!

weizhouapache commented Sep 15, 2021

Uh oh!

nvazquez commented Sep 16, 2021

Uh oh!

blueorangutan commented Sep 16, 2021

Uh oh!

yadvr commented Sep 16, 2021

Uh oh!

blueorangutan commented Sep 16, 2021

Uh oh!

davidjumani commented Sep 16, 2021

Uh oh!

blueorangutan commented Sep 16, 2021

Uh oh!

davidjumani left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Sep 16, 2021

Uh oh!

blueorangutan commented Sep 16, 2021

Uh oh!

blueorangutan commented Sep 16, 2021

Uh oh!

sureshanaparti commented Sep 16, 2021

Uh oh!

blueorangutan commented Sep 16, 2021

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Sep 17, 2021

Uh oh!

blueorangutan commented Sep 17, 2021

Uh oh!

weizhouapache commented Sep 17, 2021

Uh oh!

blueorangutan commented Sep 17, 2021

Uh oh!