PR multi tags in compute offering [#4398] #4399

DK101010 · 2020-10-13T06:42:42Z

Description

only changed HostDaoImpl.java to handle with comma separated multi host string
old behavior with one tag works like the old implementation

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)

How Has This Been Tested?

manually tested (run cloudstack in my vmware environment)

PR: #4399

engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java

DaanHoogland · 2020-11-12T12:08:15Z

@DV101010 can you invest some time in an automated test?

DK101010 · 2020-11-12T13:56:20Z

Hi Dahn, when you talk about automation tests then mean you marvin and similar things, correct? I'm a newbie in cloudstack and haven't experience with marvin. I can try it but I think it will need some time. Exists next to this side https://cwiki.apache.org/confluence/display/CLOUDSTACK/Marvin+-+Testing+with+Python other useful documents for the first steps? Am Do., 12. Nov. 2020 um 13:08 Uhr schrieb dahn <[email protected]>:

…

@DV101010 can you invest some time in an automated test? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4399 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANW3U4XYLKCT4HIUIO7V7HDSPPF33ANCNFSM4SOGQP3Q> .

DaanHoogland · 2020-11-12T14:46:49Z

@DK101010 All necesary steps/items are in the cwiki in that article and in others, but if you have specific questions the [email protected] list can give more feedback.

Update Fork

DK101010 · 2020-12-04T12:49:40Z

Hi @DaanHoogland,
I have added a marvin test and tried to implement a generic solution, but a couple of things must be adapt like host, network name and hypervisor as well I couldn't found a option to test the ingest feature with marvin.

Let me know if I can improve the test.

DaanHoogland · 2020-12-08T07:19:55Z

@blueorangutan test centos7 kvm-centos7 keepEnv

encapsulate multi tag check from Ingest Feature, DepolymentPlanningManager into HostDaoImpl to prevent code duplicates

DK101010 · 2020-12-09T14:23:10Z

Hi,
yesterday we had a problem during our deployment and I've found a additional tag check in the DeploymentPlanningManager. To prevent code duplicates I've encapsulate tag handling in HostDaoImpl. But I afraid I can not write test for this because we use currently a older version.

DaanHoogland · 2020-12-10T08:30:39Z

engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java

+    @Override
+    public boolean checkHostServiceOfferingTags(HostVO host, ServiceOffering serviceOffering){
+        if (host == null) {
+            return false;
+        }
+        if (serviceOffering == null) {
+            return false;
+        }
+        if (Strings.isNullOrEmpty(serviceOffering.getHostTag())) {
+            return true;
+        }
+
+        List<String> serviceOfferingTags = Arrays.asList(serviceOffering.getHostTag().split(","));
+        if(host.getHostTags() != null && host.getHostTags().containsAll(serviceOfferingTags)){
+            return true;
+        }
+        return false;
+    }


trying to isolate this check is fine but the DAO is for DB interaction, and this is really business logic. personally I would make it a default method for the Host interface, but a member of HostVO could also be.
Can you (shortly) explain why you put it here, @DK101010 ?

Hi,
In the HostDaoImpl there are a couple of other methods that are check host tags, therefore i thought will be good to add this also here. So will be encapsulate common logic in one class, but my experience is still not very well in CS and when it destroy other guidelines then will be also ok to move this method. For me sounds HostVO good, because it seems to be only relevant for this object.

as said I prefer a default method on the interface but it maight require undesirable imports so HostVO is good for my part.

will you be moving this method @DK101010 ?

is moved ;)

DaanHoogland · 2020-12-10T08:32:48Z

Hi,
yesterday we had a problem during our deployment and I've found a additional tag check in the DeploymentPlanningManager. To prevent code duplicates I've encapsulate tag handling in HostDaoImpl. But I afraid I can not write test for this because we use currently a older version.

very good to avoid duplicates, but I'd put it in the Host object hierarchy somewhere. For this piece of code a unit test would do, @DK101010 . no need for an integration test as far as I can see.

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

@DK101010 yes, I agree it should not be allowed. but, since it is allowed in 4.15 and previous versions, it might already exists in some/few user environments that a host have a same tag multiple times (who knows...).
IMHO options are
(1) check and throw an exception (as you said) or silently ignoring the duplicated tags, and remove duplications in host_tags in next cloudstack upgrade, or
(2) change your sql to take the scenario into consideration.
I would go for (1), but it looks (2) is much easier to you.

@weizhouapache I'm also for (1), it is cleaner. But we need also checks in the frontend for future actions to prevent doubles. Also I think a DB upgrade is not necessary. In my opinion it is for a user a small thing to fix this mistake. Especially as I can't imagine that there are so many duplicate tags in use.

weizhouapache · 2021-07-26T12:37:50Z

@DK101010 yes, I agree it should not be allowed. but, since it is allowed in 4.15 and previous versions, it might already exists in some/few user environments that a host have a same tag multiple times (who knows...).
IMHO options are
(1) check and throw an exception (as you said) or silently ignoring the duplicated tags, and remove duplications in host_tags in next cloudstack upgrade, or
(2) change your sql to take the scenario into consideration.
I would go for (1), but it looks (2) is much easier to you.

@weizhouapache I'm also for (1), it is cleaner. But we need also checks in the frontend for future actions to prevent doubles. Also I think a DB upgrade is not necessary. In my opinion it is for a user a small thing to fix this mistake. Especially as I can't imagine that there are so many duplicate tags in use.

@DK101010
if we choose (1), we should fix it in api/java code as well as gui.

IMHO DB upgrade is also required, as we need to remove the duplications in cloudstack upgrade (otherwise it needs user intervention to remove them from DB manually ). for example, we need to remove record with id=62 from below

mysql> select * from host_tags;
+----+---------+------+
| id | host_id | tag  |
+----+---------+------+
| 61 |       4 | tag1 |
| 62 |       4 | tag1 |
+----+---------+------+
2 rows in set (0.00 sec)

it is not a small change. I confirm that we can set same storage tag two times on a primary storage as well.

DK101010 · 2021-07-29T11:54:50Z

@DK101010 yes, I agree it should not be allowed. but, since it is allowed in 4.15 and previous versions, it might already exists in some/few user environments that a host have a same tag multiple times (who knows...).
IMHO options are
(1) check and throw an exception (as you said) or silently ignoring the duplicated tags, and remove duplications in host_tags in next cloudstack upgrade, or
(2) change your sql to take the scenario into consideration.
I would go for (1), but it looks (2) is much easier to you.

@weizhouapache I'm also for (1), it is cleaner. But we need also checks in the frontend for future actions to prevent doubles. Also I think a DB upgrade is not necessary. In my opinion it is for a user a small thing to fix this mistake. Especially as I can't imagine that there are so many duplicate tags in use.

@DK101010
if we choose (1), we should fix it in api/java code as well as gui.

IMHO DB upgrade is also required, as we need to remove the duplications in cloudstack upgrade (otherwise it needs user intervention to remove them from DB manually ). for example, we need to remove record with id=62 from below
mysql> select * from host_tags;
+----+---------+------+
| id | host_id | tag  |
+----+---------+------+
| 61 |       4 | tag1 |
| 62 |       4 | tag1 |
+----+---------+------+
2 rows in set (0.00 sec)
it is not a small change. I confirm that we can set same storage tag two times on a primary storage as well.

OK, I thought the user can change the tags over the UI.

So we need additional
- Checks for UI Input for Host and SO
- Checks for API input for Host and SO
- Exception for findHostByComputeOfferings
- SQL Upgrade script

weizhouapache · 2021-07-29T14:05:24Z

@DK101010 yes, I agree it should not be allowed. but, since it is allowed in 4.15 and previous versions, it might already exists in some/few user environments that a host have a same tag multiple times (who knows...).
IMHO options are
(1) check and throw an exception (as you said) or silently ignoring the duplicated tags, and remove duplications in host_tags in next cloudstack upgrade, or
(2) change your sql to take the scenario into consideration.
I would go for (1), but it looks (2) is much easier to you.

@weizhouapache I'm also for (1), it is cleaner. But we need also checks in the frontend for future actions to prevent doubles. Also I think a DB upgrade is not necessary. In my opinion it is for a user a small thing to fix this mistake. Especially as I can't imagine that there are so many duplicate tags in use.

@DK101010
if we choose (1), we should fix it in api/java code as well as gui.
IMHO DB upgrade is also required, as we need to remove the duplications in cloudstack upgrade (otherwise it needs user intervention to remove them from DB manually ). for example, we need to remove record with id=62 from below
mysql> select * from host_tags;
+----+---------+------+
| id | host_id | tag  |
+----+---------+------+
| 61 |       4 | tag1 |
| 62 |       4 | tag1 |
+----+---------+------+
2 rows in set (0.00 sec)
it is not a small change. I confirm that we can set same storage tag two times on a primary storage as well.
OK, I thought the user can change the tags over the UI.

So we need additional

Checks for UI Input for Host and SO

Checks for API input for Host and SO

Exception for findHostByComputeOfferings

SQL Upgrade script

@DK101010 yes, it looks a bit complicated. I suggest you to change SQL in this pr as 1st step.

nvazquez · 2021-08-03T14:41:36Z

Thanks for start incorporating the suggested changes @DK101010, let us know when it is ready for review

weizhouapache · 2021-08-03T15:08:08Z

@blueorangutan package

blueorangutan · 2021-08-03T15:09:03Z

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

blueorangutan · 2021-08-03T15:11:59Z

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

weizhouapache · 2021-08-03T15:16:13Z

@DK101010 could you please fix the conflicts ?

weizhouapache · 2021-08-04T07:22:49Z

@blueorangutan package

blueorangutan · 2021-08-04T07:23:03Z

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

blueorangutan · 2021-08-04T07:48:47Z

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

weizhouapache · 2021-08-04T07:50:40Z

@blueorangutan test

blueorangutan · 2021-08-04T07:51:02Z

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

weizhouapache · 2021-08-04T12:05:54Z

tested ok

blueorangutan · 2021-08-04T21:24:55Z

Trillian test result (tid-1471)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47094 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4399-t1471-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_resource_accounting.py
Intermittent failure detected: /marvin/tests/smoke/test_router_dnsservice.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 86 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true	`Failure`	144.54	test_routers_network_ops.py
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false	`Failure`	355.92	test_routers_network_ops.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	552.49	test_vpc_redundant.py
test_01_vpc_site2site_vpn_multiple_options	`Failure`	589.62	test_vpc_vpn.py
test_01_vpc_site2site_vpn	`Failure`	256.46	test_vpc_vpn.py

nvazquez · 2021-08-16T03:50:41Z

@blueorangutan package

blueorangutan · 2021-08-16T03:51:03Z

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

blueorangutan · 2021-08-16T04:43:46Z

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

nvazquez · 2021-08-16T04:57:15Z

@blueorangutan test

blueorangutan · 2021-08-16T04:58:03Z

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

blueorangutan · 2021-08-16T14:58:06Z

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

Test	Result	Time (s)	Test File

nvazquez

LGTM

[apache#4398] adapt code to handle multi tag string with commas

ab4eb9b

DaanHoogland requested changes Oct 13, 2020

View reviewed changes

engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java Outdated Show resolved Hide resolved

engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java Outdated Show resolved Hide resolved

[apache#4398] remove trailing spaces

2957af8

apache deleted a comment from blueorangutan Oct 14, 2020

DaanHoogland requested review from DaanHoogland, sureshanaparti and weizhouapache October 16, 2020 07:10

PaulAngus added the type:enhancement label Oct 29, 2020

[apache#4398] add multi host tag support for ingest process

c43684c

DaanHoogland added this to the 4.16.0.0 milestone Nov 12, 2020

DK101010 and others added 2 commits November 16, 2020 16:18

Merge pull request #1 from apache/master

611fc43

Update Fork

Merge branch 'master' into feat/multihost_tag_support_compute_offerings

ea3072f

DaanHoogland added the Severity:Major label Nov 24, 2020

[apache#4398] add test for multi tag support in offerings

4c93714

[apache#4398] update multitag support for DeploymentPlanningManagerImpl

f3a709c

encapsulate multi tag check from Ingest Feature, DepolymentPlanningManager into HostDaoImpl to prevent code duplicates

DaanHoogland reviewed Dec 10, 2020

View reviewed changes

apache deleted a comment from blueorangutan Dec 12, 2020

adapt sql for double tags

8258de0

merge main

4ad259d

weizhouapache approved these changes Aug 4, 2021

View reviewed changes

weizhouapache closed this Aug 4, 2021

weizhouapache reopened this Aug 4, 2021

nvazquez approved these changes Aug 16, 2021

View reviewed changes

nvazquez merged commit 664a46a into apache:main Aug 16, 2021

weizhouapache mentioned this pull request Sep 28, 2021

server: cannot deploy/start vm if service offering has multiple tags #5521

Merged

12 tasks

PR multi tags in compute offering [#4398] #4399

PR multi tags in compute offering [#4398] #4399

Uh oh!

Conversation

DK101010 commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

How Has This Been Tested?

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Nov 12, 2020

Uh oh!

DK101010 commented Nov 12, 2020 via email

Uh oh!

DaanHoogland commented Nov 12, 2020

Uh oh!

DK101010 commented Dec 4, 2020

Uh oh!

DaanHoogland commented Dec 8, 2020

Uh oh!

DK101010 commented Dec 9, 2020

Uh oh!

DaanHoogland Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

DK101010 Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

DK101010 Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Dec 10, 2020

Uh oh!

DK101010 commented Jul 26, 2021

Uh oh!

weizhouapache commented Jul 26, 2021

Uh oh!

DK101010 commented Jul 29, 2021

Uh oh!

weizhouapache commented Jul 29, 2021

Uh oh!

nvazquez commented Aug 3, 2021

Uh oh!

weizhouapache commented Aug 3, 2021

Uh oh!

blueorangutan commented Aug 3, 2021

Uh oh!

blueorangutan commented Aug 3, 2021

Uh oh!

weizhouapache commented Aug 3, 2021

Uh oh!

weizhouapache commented Aug 4, 2021

Uh oh!

blueorangutan commented Aug 4, 2021

Uh oh!

blueorangutan commented Aug 4, 2021

Uh oh!

weizhouapache commented Aug 4, 2021

Uh oh!

blueorangutan commented Aug 4, 2021

Uh oh!

weizhouapache commented Aug 4, 2021

Uh oh!

blueorangutan commented Aug 4, 2021

Uh oh!

nvazquez commented Aug 16, 2021

Uh oh!

blueorangutan commented Aug 16, 2021

Uh oh!

blueorangutan commented Aug 16, 2021

Uh oh!

nvazquez commented Aug 16, 2021

DK101010 commented Oct 13, 2020 •

edited

Loading