Uh oh!
There was an error while loading. Please reload this page.
CLOUDSTACK-9827: Storage tags stored in multiple places - #1994
Conversation
serg38
commented
Mar 8, 2017
@mike-tutkowski@karuturi@koushik-das@rafaelweingartner Please review the fix for 4.10 blocker |
serg38
commented
Mar 8, 2017
@nvazquez Should we simply replace storage_tag_view instead of switching queries over to another view? |
borisstoyanov
commented
Mar 9, 2017
@blueorangutan package |
blueorangutan
commented
Mar 9, 2017
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Mar 9, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-578 |
borisstoyanov
commented
Mar 9, 2017
@blueorangutan test |
blueorangutan
commented
Mar 9, 2017
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Mar 9, 2017
Trillian test result (tid-940)
|
rafaelweingartner
commented
Mar 9, 2017
@nvazquez I am assuming the change needed to fix the bug is line 135 at PrimaryDataStoreHelper.java? |
nvazquez
commented
Mar 10, 2017
Hi @rafaelweingartner, you're right, it was basically that fix. I have an NFS SR as primary storage under CloudStack with a storage tag of NFS-A. I have a compute offering with a matching storage tag. I can’t seem to get a VM to spin up, however: It says insufficient capacity. The CPU, MHz, and memory are all low (and what I typically use), so I think the problem is with matching the storage tag. |
serg38
commented
Mar 10, 2017
@nvazquez Is storage_tag_view still in use after this change that reference old way of retrieving the tags? If not should we remove it? E.g. host tags don't have a separate view. |
@serg38 actually is not being used anymore, I'll add removal of the view on last commit |
bd1ba78 to
e618253CompareAdded these items to description:
|
serg38
commented
Mar 10, 2017
@rhtyd @borisstoyanov Can you kick off new tests for this release blocker? |
borisstoyanov
commented
Mar 10, 2017
@blueorangutan package |
blueorangutan
commented
Mar 10, 2017
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Mar 10, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-585 |
borisstoyanov
commented
Mar 10, 2017
@blueorangutan test |
blueorangutan
commented
Mar 10, 2017
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Mar 10, 2017
Trillian test result (tid-946)
|
serg38
commented
Mar 10, 2017
@mike-tutkowski Can you test this fix in your environment? |
mike-tutkowski
commented
Mar 10, 2017
via email
I should be able to do so over the weekend.
On Mar 10, 2017, at 11:26 AM, serg38 <notifications@github.com<mailto:notifications@github.com>> wrote:
@mike-tutkowski<https://github.com/mike-tutkowski> Can you test this fix in your environment?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#1994 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AC4SH9oz_8gnRm3QrHySxROzwA3Rw2Toks5rkZXJgaJpZM4MXSCZ>. |
rafaelweingartner
left a comment
There was a problem hiding this comment.
@nvazquez I have checked the code, and I had a few suggestions to improve the code quality. I know it is a blocker, and we are in RC. So, I will not be picky with the code. If it is not possible to make those changes, I am ok (at least for now).
| @Override | ||
| public List<StoragePoolTagVO> searchByIds(Long... stIds) { | ||
| String batchCfg = _configDao.getValue("detail.batch.query.size"); |
There was a problem hiding this comment.
@nvazquez what about extracting lines 95-97 to a method?
Then, we can have test cases and some doc for it. Especially for that 2000 magic number there.
There was a problem hiding this comment.
About number 2000, I assumed it was a default value for that configuration
| ids[k] = stIds[j]; | ||
| } | ||
| SearchCriteria<StoragePoolTagVO> sc = StoragePoolIdsSearch.create(); |
There was a problem hiding this comment.
@nvazquez lines 105-119 are the same as 128-142.
What about extracting them for a method?
There was a problem hiding this comment.
Done, created method searchForStoragePoolIdsInternal
| Map<String, String> details = tagsToDetails(tags); | ||
| StringBuilder sql = new StringBuilder(ZoneWideDetailsSqlPrefix); | ||
| StringBuilder sql = new StringBuilder(ZoneWideTagsSqlPrefix); |
There was a problem hiding this comment.
@nvazquez what about extracting lines 463-469 to a method? Then, we can create doc and most of all test cases. This is a pretty tricky bit of code (Especially line 468).
There was a problem hiding this comment.
Thanks for pointing this out, I had missed it out. Created methods getSqlPreparedStatement and searchStoragePoolsPreparedStatement which are called from many methods and allow storage pool retrieval
mike-tutkowski
commented
Mar 11, 2017
I have run the following use cases successfully: PS = Primary Storage Create PS_1 with ST NFS-A Create CO_1 with ST NFS-A Create VM_1 with CO_1 (VM starts) Edit ST of PS_1 from NFS-A to NFS-B Create VM_2 with CO_1 (VM fails to find storage) Create DO_1 with ST NFS-A Create V_1 with DO_1 and attach to VM_1 (fails to find storage) Edit ST of PS_1 from NFS-B to NFS-A Create V_1 with DO_1 and attach to VM_1 Create PS_2 with no ST Edit ST of PS_2 to NFS-B Create V_3 with DO_1 and attach to VM_2 (lands on expected PS) Create PS_3 with ST NFS-B Migrate V_4 from PS_2 to PS_3 (PS_1 is not an option) Migrate V_3 from PS_1 to PS_2 (Neither PS_2 nor PS_3 is an option) Migrate root V of VM_1 from PS_1 to PS_2 (Neither PS_2 nor PS_3 is an option) Migrate root V of VM_2 from PS_1 to PS_3 (PS_2 and PS_3 are options) listStorageTags API returned expected results (I ran this at several points along the way) |
karuturi
commented
Mar 13, 2017
@nvazquez I think any of the existing marvin tests didnt catch this bug. Is it possible to add a marvin test for it? |
nvazquez
commented
Mar 13, 2017
@mike-tutkowski awesome, thanks for testing this PR! @rafaelweingartner thanks for reviewing, I'll work on changes proposed @karuturi sure, I'll work on it, thanks! |
rafaelweingartner
left a comment
There was a problem hiding this comment.
@nvazquez I have been thinking a while about this conditional. what do you think about my comment?
| List<StoragePoolTagVO> uvList = new ArrayList<StoragePoolTagVO>(); | ||
| int curr_index = 0; | ||
| if (stIds.length > detailsBatchSize) { |
There was a problem hiding this comment.
Correct me if I am wrong, but this if is not needed.
Let's assume the configuration:
batchSize=2000
current_index=0
lengthOfStIds=100
curr_index + detailsBatchSize = 0 + 2000, which is not less than the size of the array (100). Therefore, the while is not executed. Then, the pools will be loaded at lines 100-112
There was a problem hiding this comment.
I think that if is needed, we need it to control the max query size. I agree with the example you provided, but let's say for example that lengthOfStIds is greater that batchSize. If we remove that if, we will load pools on lines 100-112 but with a query size greater that batchSize (defined in line 111)
There was a problem hiding this comment.
I mistyped the lines in my last sentence, it should have been 110-112.
Will it be loaded with a bigger batch size? From my reading, even without the if at line 103, if stIds.length is smaller than the batch size, the while is not executed (not even once). Then, the flow proceeds the same as if the if had been executed. Then, at line 111 the batch_size is calculated using stIds.length - curr_index.
There was a problem hiding this comment.
Hi @rafaelweingartner, I assumed that you meant deleting the whole if block (lines 103-108), is it correct or you mean just deleting line 103 (and 108)?
There was a problem hiding this comment.
Ah, no this is not what I meant.
I meant only removing the if and letting the while directly there.
Just deleting line 103
There was a problem hiding this comment.
Nice, I agree with you that the line can be removed. Sorry for the confusion, I'll try adding some unit tests for this change. Thanks!
There was a problem hiding this comment.
No problem ;)
It is awesome the changes you have done here. The code became cleaner, with good documentation and on top of that, we now have unit tests to guarantee that the methods are workings as expected.
Great job @nvazquez, well done!
PS: Sorry if I have been too picky with the code changes.
rafaelweingartner
left a comment
There was a problem hiding this comment.
Other static variable that was left behind in the test case
| private static PrimaryDataStoreDaoImpl primaryDataStoreDao = new PrimaryDataStoreDaoImpl(); | ||
| @Mock | ||
| static StoragePoolVO storagePoolVO; |
There was a problem hiding this comment.
Here you have other static variable in a test case that does not need to be static.
karuturi
commented
Mar 17, 2017
@nvazquez any update? |
nvazquez
commented
Mar 17, 2017
Hi @karuturi, I've been working on marvin tests, I hope posting them today |
nvazquez
commented
Mar 17, 2017
@karuturi I added marvin tests to simulate tests performed by @mike-tutkowski. This are results in our env: @rafaelweingartner@mike-tutkowski@karuturi can you please review marvin tests added? |
karuturi
commented
Mar 19, 2017
Thanks @nvazquez Waiting for LGTMs |
serg38
commented
Mar 20, 2017
@nvazquez Looks like Travis cant SSH into management server during the test . 2017-03-17 19:26:58,672 - DEBUG - mount -t nfs nfs:/export/automation/1/testprimary /mnt/marvin-1b4190cb-b9f5-4441-9bdc-bd89e9b7567b |
nvazquez
commented
Mar 21, 2017
Thanks @serg38, we are using |
karuturi
commented
Mar 22, 2017
@mike-tutkowski can you do a final review please? |
mike-tutkowski
commented
Mar 22, 2017
I should have a chance to look through this later tonight. In the meanwhile, perhaps we can re-push the code to kick off Travis again (since its most recent run has a failure). Thanks! |
mike-tutkowski
commented
Mar 23, 2017
OK, this LGTM (as long as Travis shows green before we merge). Thanks! |
nvazquez
commented
Mar 23, 2017
Thanks @mike-tutkowski! I pushed force to kick off Travis again |
b6f0d76 to
84ce38fComparenvazquez
commented
Mar 24, 2017
@karuturi I refactored last marvin test which was failing on Travis. These are results in our env: @rhtyd @borisstoyanov@jburwell on |
nvazquez
commented
Mar 24, 2017
@karuturi Travis is now failing as it doesn't find key "nfs2" It is introduced in PR #1961, can these two PRs be merged together? |
karuturi
commented
Mar 28, 2017
CLOUDSTACK-9827: Storage tags stored in multiple placesIssue description: https://issues.apache.org/jira/browse/CLOUDSTACK-9827 ### Fixes - Create Primary Storage: Persist tags into `storage_pool_tags` instead of `storage_pool_details` - List Storage Tags: Queries `storage_pool_tags` table instead of `storage_tag_view` - Find Storage Pools by Tags using proper DAO - Remove storage tags after deleting Primary Storage - Remove unused `StorageTagDao`, `StorageTagDaoImpl`, `StorageTagVO` and `storage_tag_view` * pr/1994: CLOUDSTACK-9827: Storage tags stored in multiple places Signed-off-by: Rajani Karuturi <rajani.karuturi@accelerite.com>
| verify(primaryDataStoreDao).getSqlPreparedStatement( | ||
| primaryDataStoreDao.TagsSqlPrefix, primaryDataStoreDao.TagsSqlSuffix, SQL_VALUES, CLUSTER_ID); | ||
| String expectedSql = primaryDataStoreDao.TagsSqlPrefix + SQL_VALUES + primaryDataStoreDao.TagsSqlSuffix; | ||
| verify(primaryDataStoreDao).searchStoragePoolsPreparedStatement(expectedSql, DATACENTER_ID, POD_ID, CLUSTER_ID, SCOPE, STORAGE_TAGS_ARRAY.length); |
There was a problem hiding this comment.
@nvazquez With these types of test we are kind of forcing the SQL statement to use in implementation. Do you think this is correct thing to test? What if somebody wants to rewrite in different way altogether to achieve the same result?
There was a problem hiding this comment.
Hi @anshul1886,
I understand your point of view and I think it is valid. However, from my point of view these unit tests are covering internal methods used in the implementation. For example, for method String getSqlValuesFromStorageTags(String[] tags) tests provided cover cases when parameter is null, empty or not null. As this method returns a string, it is the goal of unit tests to check if the string returned by the method is the one we expect for each of those cases, and in my opinion, it is being tested properly. I mention this particular method as string returned is a SQL statement.
The last two tests in this file are testing findPoolsByDetailsOrTagsInternal, comparing returned list with the one expected and also verifying that internal methods are invoked with expected parameters, but these parametes are strings (SQL statements). From my point of view, we are not forcing SQL statement to use, we are checking that internal methods are being invoked properly.
In this way, if method is refactored but achieves the same result, we have to modify/remove lines which verifies if internal methods are invoked for the ones that belong to new implementation.
Please let me know if I didn't explain myself, what do you think? I don't mean to be stubborn with this, it was my point of view, and of course I might be wrong. What do you think it has to be refactored in these tests?
anshul1886
commented
May 29, 2017
nvazquez
commented
May 30, 2017
Hi @anshul1886, you can modify |
anshul1886
commented
Jun 5, 2017
@nvazquez Is it ok for you to remove those tests? |
nvazquez
commented
Jun 7, 2017
@anshul1886 I would prefer that you delete |
Issue description: https://issues.apache.org/jira/browse/CLOUDSTACK-9827
Fixes
storage_pool_tagsinstead ofstorage_pool_detailsstorage_pool_tagstable instead ofstorage_tag_viewStorageTagDao,StorageTagDaoImpl,StorageTagVOandstorage_tag_view