Refactor of Allocator classes #9074
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9074 +/- ##
============================================
+ Coverage 16.19% 16.23% +0.04%
- Complexity 13051 13098 +47
============================================
Files 5645 5646 +1
Lines 494567 494497 -70
Branches 59955 59905 -50
============================================
+ Hits 80088 80276 +188
+ Misses 405642 405386 -256
+ Partials 8837 8835 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
BryanMLima
force-pushed
the
refactor-allocate-to-method
branch
from
1ff8f4a to
42d16d9
Compare
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
BryanMLima
force-pushed
the
refactor-allocate-to-method
branch
from
537c10f to
952c273
Compare
BryanMLima
changed the title
BryanMLima
changed the title
BryanMLima
requested review from
JoaoJandre and
DaanHoogland
and removed request for
JoaoJandre
server/src/main/java/com/cloud/agent/manager/allocator/impl/TestingAllocator.java
Show resolved
Hide resolved
BryanMLima
force-pushed
the
refactor-allocate-to-method
branch
2 times, most recently
from
dd1eb14 to
952c273
Compare
BryanMLima
force-pushed
the
refactor-allocate-to-method
branch
from
952c273 to
b7cc66f
Compare
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10801 |
|
@blueorangutan LLtest |
|
@JoaoJandre 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11643 |
|
@DaanHoogland @sureshanaparti @shwstppr could we run the CI here? |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
BryanMLima
force-pushed
the
refactor-allocate-to-method
branch
from
1e91276 to
d0e5efd
Compare
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12226 |
| } | ||
| } | ||
| } | ||
| retainHostsMatchingServiceOfferingAndTemplateTags(clusterHosts, type, clusterId, podId, dcId, hostTagOnTemplate, hostTagOnOffering); |
There was a problem hiding this comment.
| retainHostsMatchingServiceOfferingAndTemplateTags(clusterHosts, type, clusterId, podId, dcId, hostTagOnTemplate, hostTagOnOffering); | |
| retainHostsMatchingServiceOfferingAndTemplateTags(clusterHosts, type, dcId, podId, clusterId, hostTagOnOffering, hostTagOnTemplate); |
The parameters are flipped
| } | ||
|
|
||
| if (ObjectUtils.anyNotNull(offeringHostTag, templateTag)) { | ||
| retainHostsMatchingServiceOfferingAndTemplateTags(availableHosts, type, clusterId, podId, dcId, offeringHostTag, templateTag); |
There was a problem hiding this comment.
| retainHostsMatchingServiceOfferingAndTemplateTags(availableHosts, type, clusterId, podId, dcId, offeringHostTag, templateTag); | |
| retainHostsMatchingServiceOfferingAndTemplateTags(availableHosts, type, dcId, podId, clusterId, offeringHostTag, templateTag); |
Same issue here
| String hostTagOnOffering = null; | ||
|
|
||
| Mockito.doReturn(hostsWithMathingTags).when(hostDaoMock).listByHostTag(type, clusterId, podId, dcId, hostTagOnTemplate); | ||
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, clusterId, podId, dcId, hostTagOnTemplate, hostTagOnOffering); |
There was a problem hiding this comment.
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, clusterId, podId, dcId, hostTagOnTemplate, hostTagOnOffering); | |
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, dcId, podId, clusterId, hostTagOnOffering, hostTagOnTemplate); |
| private final Long clusterId = 1L; | ||
|
|
||
| private final Long podId = 1L; | ||
|
|
||
| private final Long dcId = 1L; |
There was a problem hiding this comment.
| private final Long clusterId = 1L; | |
| private final Long podId = 1L; | |
| private final Long dcId = 1L; | |
| private final Long clusterId = 1L; | |
| private final Long podId = 2L; | |
| private final Long dcId = 3L; |
It would be nice to use some different numbers for clusterId, podId and dcId to catch errors like these
|
|
||
| Mockito.doReturn(hostsWithMathingTemplateTags).when(hostDaoMock).listByHostTag(type, clusterId, podId, dcId, hostTagOnTemplate); | ||
| Mockito.doReturn(hostsWithMathingServiceTags).when(hostDaoMock).listByHostTag(type, clusterId, podId, dcId, hostTagOnOffering); | ||
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, clusterId, podId, dcId, hostTagOnTemplate, hostTagOnOffering); |
There was a problem hiding this comment.
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, clusterId, podId, dcId, hostTagOnTemplate, hostTagOnOffering); | |
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, dcId, podId, clusterId, hostTagOnOffering, hostTagOnTemplate); |
| String hostTagOnOffering = "hostTagOnOffering"; | ||
|
|
||
| Mockito.doReturn(hostsWithMathingServiceTags).when(hostDaoMock).listByHostTag(type, clusterId, podId, dcId, hostTagOnOffering); | ||
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, clusterId, podId, dcId, hostTagOnTemplate, hostTagOnOffering); |
There was a problem hiding this comment.
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, clusterId, podId, dcId, hostTagOnTemplate, hostTagOnOffering); | |
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, dcId, podId, clusterId, hostTagOnOffering, hostTagOnTemplate); |
| String hostTagOnTemplate = null; | ||
| String hostTagOnOffering = null; | ||
|
|
||
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, clusterId, podId, dcId, hostTagOnTemplate, hostTagOnOffering); |
There was a problem hiding this comment.
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, clusterId, podId, dcId, hostTagOnTemplate, hostTagOnOffering); | |
| baseAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(suitableHosts, type, dcId, podId, clusterId, hostTagOnOffering, hostTagOnTemplate); |
| return; | ||
| } | ||
|
|
||
| logger.info("Found hosts %s with tag rules matching the compute offering tag [{}].", hostsWithTagRules, hostTagOnOffering); |
There was a problem hiding this comment.
| logger.info("Found hosts %s with tag rules matching the compute offering tag [{}].", hostsWithTagRules, hostTagOnOffering); | |
| logger.info("Found hosts {} with tag rules matching the compute offering tag [{}].", hostsWithTagRules, hostTagOnOffering); |
| private final Long clusterId = 1L; | ||
|
|
||
| private final Long podId = 1L; | ||
|
|
||
| private final Long zoneId = 1L; |
There was a problem hiding this comment.
| private final Long clusterId = 1L; | |
| private final Long podId = 1L; | |
| private final Long zoneId = 1L; | |
| private final Long clusterId = 1L; | |
| private final Long podId = 2L; | |
| private final Long zoneId = 3L; |
| // No template tagged host | ||
| ArrayList<HostVO> noTemplateTaggedHosts = new ArrayList<>(Arrays.asList(host1, host2)); | ||
| Mockito.when(hostDao.listByHostTag(type, id, id, id, templateTag)).thenReturn(new ArrayList<>()); | ||
| randomAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(noTemplateTaggedHosts, type, id, id, id, offeringTag, templateTag); |
There was a problem hiding this comment.
| randomAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(noTemplateTaggedHosts, type, id, id, id, offeringTag, templateTag); | |
| randomAllocator.retainHostsMatchingServiceOfferingAndTemplateTags(noTemplateTaggedHosts, type, zoneId, podId, clusterId, offeringTag, templateTag); |
Also need to adjust for some other calls in this file
|
|
||
| // No template tagged host | ||
| ArrayList<HostVO> noTemplateTaggedHosts = new ArrayList<>(Arrays.asList(host1, host2)); | ||
| Mockito.when(hostDao.listByHostTag(type, id, id, id, templateTag)).thenReturn(new ArrayList<>()); |
There was a problem hiding this comment.
| Mockito.when(hostDao.listByHostTag(type, id, id, id, templateTag)).thenReturn(new ArrayList<>()); | |
| Mockito.when(hostDao.listByHostTag(type, clusterId, podId, zoneId, templateTag)).thenReturn(new ArrayList<>()); |
Also need to adjust for some other calls in this file
DaanHoogland
requested review from
DaanHoogland
and removed request for
DaanHoogland
| @@ -1606,10 +1602,10 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho | |||
|
|
|||
| _dpMgr.reorderHostsByPriority(plan.getHostPriorities(), suitableHosts); | |||
There was a problem hiding this comment.
I'm getting a NPE here when opening the migration pop-up because suitableHosts is null (no hosts were found for allocation). Need to check whether suitableHosts is null before proceeding with the reorder.
Description
This PR refactors some
*Allocatorclasses, improving modularity and code legibility. This PR also made some changes to logs across these classes.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
I tested the allocation process in my personal lab, using both the
RandomAllocatorandFirstFitAllocatorallocators. 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?