Refactor of Allocator classes #9074

BryanMLima · 2024-05-10T20:49:48Z

Description

This PR refactors some *Allocator classes, improving modularity and code legibility. This PR also made some changes to logs across these classes.

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)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

I tested the allocation process in my personal lab, using both the RandomAllocator and FirstFitAllocator allocators. I tried some variation of tags and offerings, and everything looks good. Furthermore, I also added a lot of unit tests for the methods that I refactored.

How did you try to break this feature and the system with this change?

codecov · 2024-05-10T20:58:21Z

Codecov Report

Attention: Patch coverage is 68.89764% with 79 lines in your changes missing coverage. Please review.

Project coverage is 15.85%. Comparing base (1d4700a) to head (b93e998).

Files with missing lines	Patch %	Lines
...gent/manager/allocator/impl/FirstFitAllocator.java	60.26%	58 Missing and 2 partials ⚠️
.../agent/manager/allocator/impl/RandomAllocator.java	84.48%	9 Missing ⚠️
...agent/manager/allocator/impl/TestingAllocator.java	0.00%	3 Missing ⚠️
...in/java/com/cloud/server/ManagementServerImpl.java	0.00%	3 Missing ⚠️
.../src/main/java/com/cloud/host/dao/HostDaoImpl.java	0.00%	2 Missing ⚠️
...i/command/admin/host/FindHostsForMigrationCmd.java	0.00%	1 Missing ⚠️
...loudstack/api/command/admin/host/ListHostsCmd.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9074      +/-   ##
============================================
+ Coverage     15.81%   15.85%   +0.04%     
- Complexity    12554    12601      +47     
============================================
  Files          5629     5630       +1     
  Lines        492029   491963      -66     
  Branches      62293    63884    +1591     
============================================
+ Hits          77811    77999     +188     
+ Misses       405894   405642     -252     
+ Partials       8324     8322       -2

Flag	Coverage Δ
uitests	`4.48% <ø> (ø)`
unittests	`16.64% <68.89%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-06-19T04:16:42Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

github-actions · 2024-07-12T08:04:09Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

DaanHoogland · 2024-07-15T14:51:38Z

server/src/main/java/com/cloud/agent/manager/allocator/impl/TestingAllocator.java

 import com.cloud.utils.component.AdapterBase;
-import com.cloud.vm.VirtualMachine;
 import com.cloud.vm.VirtualMachineProfile;

 public class TestingAllocator extends AdapterBase implements HostAllocator {


why is this not a BaseAllocator-child, @BryanMLima ?

@DaanHoogland, because TestingAllocator does not have any methods in common with BaseAllocator. Is there a reason that it should extend it?

No, but it seems an outlier and the name also suggests it is intended to be a mock version for testing what would make less sense if it is not based on the same. I'd have to study it's use to give a definite answer.

JoaoJandre

LGTM, I did some basic testing, changed the allocator to FirstFit and created some VMs, the VM allocation worked fine with and without tags. However, my tests were limited, further testing would be good.

DaanHoogland · 2024-08-26T08:11:09Z

@blueorangutan package

blueorangutan · 2024-08-26T08:12:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-08-26T09:30:59Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10801

DaanHoogland · 2024-08-26T13:06:42Z

@blueorangutan LLtest

blueorangutan · 2024-09-06T12:54:45Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11039

DaanHoogland · 2024-09-09T06:51:54Z

@blueorangutan test

blueorangutan · 2024-09-09T06:54:03Z

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

blueorangutan · 2024-09-09T23:13:19Z

[SF] Trillian test result (tid-11424)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 56478 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9074-t11424-kvm-ol8.zip
Smoke tests completed. 138 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_isolate_network_FW_PF_default_routes_egress_true	`Failure`	110.59	test_routers_network_ops.py
test_02_restore_vm_strict_tags_failure	`Failure`	58.19	test_vm_strict_host_tags.py
test_02_scale_vm_strict_tags_failure	`Failure`	60.31	test_vm_strict_host_tags.py
test_06_deploy_vm_on_any_host_with_strict_tags_failure	`Failure`	5.54	test_vm_strict_host_tags.py
test_hostha_enable_ha_when_host_disabled	`Error`	3.93	test_hostha_kvm.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	302.07	test_hostha_kvm.py

DaanHoogland

clgtm

BryanMLima · 2024-09-10T13:43:31Z

@blueorangutan package

blueorangutan · 2024-09-10T13:44:03Z

@BryanMLima a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

BryanMLima · 2024-09-10T13:45:02Z

@DaanHoogland, could you trigger the smoke tests again? The failed tests in test_vm_strict_host_tags.py were related to the exception message, it should be fixed now. The others do not seem to be related to this PR.

blueorangutan · 2024-09-10T15:37:21Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11082

DaanHoogland · 2024-09-11T07:30:37Z

@blueorangutan test

blueorangutan · 2024-09-11T07:32:03Z

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

blueorangutan · 2024-09-11T22:28:07Z

[SF] Trillian test result (tid-11448)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 51366 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9074-t11448-kvm-ol8.zip
Smoke tests completed. 140 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_restore_vm_strict_tags_failure	`Failure`	52.04	test_vm_strict_host_tags.py
test_02_scale_vm_strict_tags_failure	`Failure`	55.24	test_vm_strict_host_tags.py

BryanMLima · 2024-09-16T13:29:25Z

@blueorangutan package

blueorangutan · 2024-09-16T13:30:03Z

@BryanMLima a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-09-16T14:47:06Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11121

github-actions bot added the status:has-conflicts label Jun 19, 2024

BryanMLima force-pushed the refactor-allocate-to-method branch from 1ff8f4a to 42d16d9 Compare July 11, 2024 15:47

boring-cyborg bot added component:api component:build component:compute component:management-server component:orchestration labels Jul 11, 2024

github-actions bot added status:has-conflicts and removed status:has-conflicts labels Jul 11, 2024

BryanMLima force-pushed the refactor-allocate-to-method branch from 537c10f to 952c273 Compare July 15, 2024 13:26

github-actions bot removed the status:has-conflicts label Jul 15, 2024

BryanMLima changed the title ~~Refactoring FirstFitAllocator class~~ Refactoring Allocator classes Jul 15, 2024

BryanMLima marked this pull request as ready for review July 15, 2024 13:44

BryanMLima changed the title ~~Refactoring Allocator classes~~ Refactor of Allocator classes Jul 15, 2024

BryanMLima requested review from JoaoJandre and DaanHoogland and removed request for JoaoJandre July 15, 2024 13:45

DaanHoogland reviewed Jul 15, 2024

View reviewed changes

JoaoJandre approved these changes Jul 18, 2024

View reviewed changes

BryanMLima force-pushed the refactor-allocate-to-method branch 2 times, most recently from dd1eb14 to 952c273 Compare July 24, 2024 14:28

BryanMLima force-pushed the refactor-allocate-to-method branch from 952c273 to b7cc66f Compare August 23, 2024 19:11

DaanHoogland approved these changes Sep 10, 2024

View reviewed changes

DaanHoogland added this to the 4.20.0.0 milestone Sep 10, 2024

boring-cyborg bot added component:integration-test Python Warning... Python code Ahead! labels Sep 10, 2024

JoaoJandre modified the milestones: 4.20.0.0, 4.21.0.0 Sep 10, 2024

BryanMLima added 9 commits September 16, 2024 10:27

Refactoring Allocator classes

9d06be4

Break into smaller methods random and firfit allocators.

f690c75

Added unit tests for random and firstfit allocators

c914f27

Move random allocator from cloud-plugins to cloud-server

82d807c

Add BaseAllocator abstract class for duplicate code

ecc0f01

Add missing license

3b76879

Add missing license to unit test file

85b59fe

Remove host allocator random dependency

ec9b30e

Change exception message on smoke tests

b93e998

BryanMLima force-pushed the refactor-allocate-to-method branch from 2368371 to b93e998 Compare September 16, 2024 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of Allocator classes #9074

Refactor of Allocator classes #9074

BryanMLima commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

github-actions bot commented Jun 19, 2024

github-actions bot commented Jul 12, 2024

DaanHoogland Jul 15, 2024

BryanMLima Jul 15, 2024

DaanHoogland Jul 16, 2024

JoaoJandre left a comment

DaanHoogland commented Aug 26, 2024

blueorangutan commented Aug 26, 2024

blueorangutan commented Aug 26, 2024

DaanHoogland commented Aug 26, 2024

blueorangutan commented Sep 6, 2024

DaanHoogland commented Sep 9, 2024

blueorangutan commented Sep 9, 2024

blueorangutan commented Sep 9, 2024

DaanHoogland left a comment

BryanMLima commented Sep 10, 2024

blueorangutan commented Sep 10, 2024

BryanMLima commented Sep 10, 2024

blueorangutan commented Sep 10, 2024

DaanHoogland commented Sep 11, 2024

blueorangutan commented Sep 11, 2024

blueorangutan commented Sep 11, 2024

BryanMLima commented Sep 16, 2024

blueorangutan commented Sep 16, 2024

blueorangutan commented Sep 16, 2024

Refactor of Allocator classes #9074

Are you sure you want to change the base?

Refactor of Allocator classes #9074

Conversation

BryanMLima commented May 10, 2024 • edited Loading

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov bot commented May 10, 2024 • edited Loading

Codecov Report

github-actions bot commented Jun 19, 2024

github-actions bot commented Jul 12, 2024

DaanHoogland Jul 15, 2024

Choose a reason for hiding this comment

BryanMLima Jul 15, 2024

Choose a reason for hiding this comment

DaanHoogland Jul 16, 2024

Choose a reason for hiding this comment

JoaoJandre left a comment

Choose a reason for hiding this comment

DaanHoogland commented Aug 26, 2024

blueorangutan commented Aug 26, 2024

blueorangutan commented Aug 26, 2024

DaanHoogland commented Aug 26, 2024

blueorangutan commented Sep 6, 2024

DaanHoogland commented Sep 9, 2024

blueorangutan commented Sep 9, 2024

blueorangutan commented Sep 9, 2024

DaanHoogland left a comment

Choose a reason for hiding this comment

BryanMLima commented Sep 10, 2024

blueorangutan commented Sep 10, 2024

BryanMLima commented Sep 10, 2024

blueorangutan commented Sep 10, 2024

DaanHoogland commented Sep 11, 2024

blueorangutan commented Sep 11, 2024

blueorangutan commented Sep 11, 2024

BryanMLima commented Sep 16, 2024

blueorangutan commented Sep 16, 2024

blueorangutan commented Sep 16, 2024

BryanMLima commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading