CLOUDSTACK-8865: Adding SR doesn't create Storage_pool_host_ref entry… #876
Conversation
|
cloudstack-pull-rats #702 FAILURE |
|
cloudstack-pull-analysis #652 FAILURE |
SudharmaJain
force-pushed
the
cs-8865
branch
from
00d921e
to
e080d74
Compare
|
cloudstack-pull-rats #704 FAILURE |
|
cloudstack-pull-analysis #654 SUCCESS |
SudharmaJain
force-pushed
the
cs-8865
branch
from
e080d74
to
b434333
Compare
|
cloudstack-pull-rats #754 FAILURE |
|
cloudstack-pull-analysis #705 ABORTED |
SudharmaJain
force-pushed
the
cs-8865
branch
from
b434333
to
f4f7a7e
Compare
|
cloudstack-pull-rats #773 FAILURE |
|
cloudstack-pull-analysis #725 ABORTED |
SudharmaJain
force-pushed
the
cs-8865
branch
5 times, most recently
from
31b354e
to
9bade18
Compare
| @@ -2378,6 +2378,23 @@ public boolean maintenanceFailed(final long hostId) { | |||
| } | |||
|
|
|||
| @Override | |||
| public List<HostVO> listAllUpHosts(Type type, Long clusterId, Long podId, long dcId) { | |||
There was a problem hiding this comment.
could you also write an unit test for this new method you created since it brings new functionalities to the code?
There was a problem hiding this comment.
ping @SudharmaJain , did you see this question?
There was a problem hiding this comment.
@DaanHoogland This method involves a DB query execution. I don't think this can be achieved in a unit test.
|
Rebased with master. |
|
@alexandrelimassantana This method is about a query execution on database. Could you suggest how can i add a unit test for it? |
|
@SudharmaJain sorry but can you again please rebase against latest master and push -f, update on status of your PR I would suggest you can add a marvin test that can be verified against Travis (simulator/mysql) that is more realistic than writing a unit test that mocks tcp stack and/or mysql server |
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests: Skipped tests: Passed test suits: |
|
LGTM on the code changes |
|
Test LGTM based on manual testing: Steps used for testing the fix:
Before the fix, the entry of primary storage was only added for the host in 'Enabled' state. |
|
@rhtyd As suggested I have added a marvin test case. |
| @@ -359,7 +359,7 @@ public boolean attachCluster(DataStore store, ClusterScope scope) { | |||
|
|
|||
| PrimaryDataStoreInfo primarystore = (PrimaryDataStoreInfo) store; | |||
| // Check if there is host up in this cluster | |||
| List<HostVO> allHosts = _resourceMgr.listAllUpAndEnabledHosts(Host.Type.Routing, primarystore.getClusterId(), primarystore.getPodId(), primarystore.getDataCenterId()); | |||
| List<HostVO> allHosts = _resourceMgr.listAllUpHosts(Host.Type.Routing, primarystore.getClusterId(), primarystore.getPodId(), primarystore.getDataCenterId()); | |||
There was a problem hiding this comment.
What about hosts which are in maintenance? Do we expect to add an SR to those hosts as well?
There was a problem hiding this comment.
@syed We cannot send commands to the host in maintenance mode. So it is not possible to add an SR to those host.
SudharmaJain
force-pushed
the
cs-8865
branch
2 times, most recently
from
b4f9e85
to
698adc3
Compare
|
@karuturi I had rebased the branch. |
|
tag:mergeready |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1060 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1473)
|
|
Failures are not related to this PR, test LGTM. |
|
The idea behind the PR makes sense to me and the code LGTM. |
|
tag:MergeReady |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1092 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1521)
|
… for disabled host
SudharmaJain
force-pushed
the
cs-8865
branch
from
4ae87ba
to
2bfe3f7
Compare
|
@SudharmaJain please check the failing tests |
|
@rhtyd All the tests failed here, except the skipped ones. Even the listSystemVms command returned response None. Looks like this was a setup issue. |
|
@SudharmaJain I can rekick the tests, will rebuild packages as well |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1107 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1533)
|
|
Okay test LGTM, give this has enough code review/LGTM from others I'll merge this. Thanks. |
* Handle redirect to sub menu when click parent menu * renamed the function * allow click on menu for desktop * reset cache path when click menu item without submenu * Fixes click parent menu with full expanded * removed the expand submenu block segment Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
… for disabled host
This causes VM deployment failure on the host that was disabled while adding the storage repository.
In the attachCluster function of the PrimaryDataStoreLifeCycle, we were only selecting hosts that are up and are in enabled state. Here if we select all up hosts, it will populate the DB properly and will fix this issue.
Also added a unit test for attachCluster function.