server: Search zone-wide storage pool when allocation algothrim is firstfitleastconsumed#4002
Merged
andrijapanicsb merged 1 commit intoapache:4.13from Apr 6, 2020
Conversation
…rstfitleastconsumed
Contributor
|
@andrijapanicsb can you test this? |
Contributor
|
will do, unless I forget..! |
Contributor
|
@andrijapanicsb can you test this? |
Contributor
|
@blueorangutan package |
|
@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✔debian. JID-1121 |
Contributor
|
@blueorangutan test |
|
@andrijapanicsb a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Contributor
|
@weizhouapache is this "if ALL pools are zone-wide" - i.e. in mixed cluster/zone-wide pools you did NOT observe the issue? |
Contributor
|
@blueorangutan test |
|
@andrijapanicsb a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Contributor
|
Single zone-wide - VM started fine Added another zone-wide, and was able to spring VM as expected: Have not reproduced before the fix - but the fix is working fine LGTM |
andrijapanicsb
approved these changes
Apr 3, 2020
Member
@andrijapanicsb Have you restarted the management server after vm.allocation.algorithm is changed to firstfitleastconsumed (it is 'random' by default) ? |
|
Trillian test result (tid-1355)
|
|
Trillian test result (tid-1358)
|
Contributor
|
@ustcweizhou @weizhouapache yes, of course |
DaanHoogland
approved these changes
Apr 6, 2020
Contributor
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
one possible improvement but code looks good. let's merge if testing is good.
|
|
||
| @Override | ||
| public List<Long> orderHostsByFreeCapacity(Long clusterId, short capacityTypeForOrdering){ | ||
| public List<Long> orderHostsByFreeCapacity(Long zoneId, Long clusterId, short capacityTypeForOrdering){ |
Contributor
There was a problem hiding this comment.
@weizhouapache wouldn't a cluster imply a zone? we wouldn't have to pass a zone if we already know the cluster.
Member
There was a problem hiding this comment.
@DaanHoogland if we search storage pools by clusterid, we would not get the list of zone-wide storage pools, because cluster_id is NULL.
Contributor
There was a problem hiding this comment.
makes sense, but if we have a cluster we can find the zone it is in and use that to find zone wide pools for it?
Contributor
There was a problem hiding this comment.
to be clear, I think this is aesthetic, not a potential bug.
Member
There was a problem hiding this comment.
@DaanHoogland
please look at the code, there is only 'AND cluster_id = ' in the sql.
you can also execute the query in mysql
select * from op_host_capacity where capacity_type=3 and cluster_id=??
it lists only the cluster-wide storage pools.
Contributor
There was a problem hiding this comment.
yes, i read the code. Either zone_id or cluster_id should be null. What is the use case of both being not null?
My point is; pass the cluster and find all storage pools for that cluster either cluster wide or for the zone the cluster is in.
Member
There was a problem hiding this comment.
@DaanHoogland what if the storage pool is used for another zone ?
Contributor
There was a problem hiding this comment.
if we pass the cluster and deduce the zone from that, or if we deliberately pass another zone than the cluster we are looking to deploy in? I don't see how can it ever be another zone,except for a bad bug. Or do you mean a storage pool that is used for multiple zones? I have not seen that before.
Member
There was a problem hiding this comment.
@DaanHoogland as I said, if we pass the cluster id, it lists only cluster-wide storage pools.
To list zone-wide storage pools, we should pass the zoneId/data_center_id (and cluster_id should be NULL).
Member
|
@andrijapanicsb if you cannot reproduce the issue, please do not merge this PR. ps: I am still able to reproduce the issue. We had the issue in 4.7 as well. |
Contributor
|
@weizhouapache - I have not TRIED to reproduce the issue - but I can if you find it needed? |
Contributor
|
@weizhouapache which version should I test? |
Member
@andrijapanicsb 4.13 or 4.14/master |
Contributor
|
@blueorangutan package |
|
@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✔debian. JID-1131 |
nvazquez
approved these changes
Apr 6, 2020
Contributor
nvazquez
left a comment
nvazquez
left a comment
There was a problem hiding this comment.
LGTM
Tested manually on KVM env with zone-wide storage pool and vm.allocation.algorithm = firstfitleastconsumed
Member
@nvazquez thanks for approval. Have you tested without this PR ? |
Contributor
|
@weizhouapache yes, tested in master and failed with this error: |
Member
@nvazquez good. We got same issue and it's fixed by this PR. |
Contributor
|
3 x LGTM, automated tests fine (3 known issues in 4,13, fixed in 4,14), manual tests fine Merging |
Member
|
thanks @andrijapanicsb |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When vm.allocation.algorithm is set to 'firstfitleastconsumed', we are unable to create vm or volume if all storage pools are zone-wide (which have cluster_id=NULL)
Types of changes
Screenshots (if appropriate):
How Has This Been Tested?