Uh oh!
There was an error while loading. Please reload this page.
CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Make NFS version changeable in UI - #1615
Merged
Conversation
nvazquezforce-pushed
the
nfsConfigKey
branch
6 times, most recently
from
July 25, 2016 18:18
aa02196 to
f3a5e12Compare| @@ -16,7 +16,7 @@ | |||
| * specific language governing permissions and limitations | |||
| * under the License. | |||
| */ | |||
| package org.apache.cloudstack.storage.image.db; | |||
Contributor
There was a problem hiding this comment.
What is the reason for moving this file to a new location?
ContributorAuthor
There was a problem hiding this comment.
Hi @koushik-das, the reason was to avoid a dependency cycle on Maven poms, I tried explaining it under Important section in this PR's description
Contributor
There was a problem hiding this comment.
Missed that out previously, thanks
koushik-das
commented
Aug 3, 2016
Contributor
LGTM based on code review |
nvazquez
commented
Aug 3, 2016
ContributorAuthor
Thanks @koushik-das! |
serg38
commented
Aug 4, 2016
Ping for second review -- @GabrielBrascher, @rhtyd, @wido, @rafaelweingartner |
blueorangutan
commented
Aug 5, 2016
Alright, your PR did not break packaging! Packages built and available at: http://packages.shapeblue.com/cloudstack/custom/github-1615 |
nvazquezforce-pushed
the
nfsConfigKey
branch
2 times, most recently
from
August 5, 2016 19:14
bb5c7c0 to
a8b856cCompareyadvr
commented
Aug 5, 2016
Member
@blueorangutan kick |
blueorangutan
commented
Aug 5, 2016
A Trillian-Jenkins job has been kicked to build packages and start testing. I'll keep you posted as I make progress. |
| } | ||
| if (getImageStoreId() != null) { | ||
| response.setScope("imagestore"); | ||
| } |
Contributor
There was a problem hiding this comment.
Please remove this check and adding validations = PositiveNumber to the imageStoreId @Parameter annotation.
blueorangutan
commented
Aug 5, 2016
Packaging result: ✔centos6 ✔centos7 ✔debian repo: http://packages.shapeblue.com/cloudstack/pr/1615 |
jburwell
commented
Aug 9, 2016
Contributor
@nvazquez looking good. One small item on the |
nvazquez
commented
Aug 9, 2016
ContributorAuthor
Done, thanks @jburwell for your review! |
Contributor
@nvazquez thanks for updates. The Jenkins build failed. It appears to be caused by a timeout (91 minutes). Could you please force a rebuild by executing |
nvazquez
commented
Aug 9, 2016
ContributorAuthor
Sure, done |
jburwell
commented
Aug 9, 2016
Contributor
@blueorangutan help |
koushik-das
commented
Oct 14, 2016
Contributor
@jburwell Was there a discussion thread for this on dev@, didn't remember seeing one which mentions about freezing master. These issues of test stability are known and were raised multiple times in the past as well, so not sure what you mean by severity of the situation. Anyways lets wait for the next 24 hours and after that merge the commits that have the required LGTMs. |
jburwell
commented
Oct 14, 2016
Contributor
@koushik-das please see this thread regarding the testing freeze discussion. Also, per our community release schedule, the 4.8, 4.9, and master branches are frozen for testing pre-RC. |
karuturi
commented
Oct 24, 2016
Member
@blueorangutan help |
blueorangutan
commented
Oct 24, 2016
@karuturi I understand these words: "help", "hello", "thanks", "package", "test" |
karuturi
commented
Oct 24, 2016
Member
@blueorangutan test centos7 vmware-60u2 |
blueorangutan
commented
Oct 24, 2016
@karuturi a Trillian-Jenkins test job (centos7 mgmt + vmware-60u2) has been kicked to run smoke tests |
karuturi
commented
Oct 25, 2016
Member
@blueorangutan @rhtyd any update on the tests? Is the job running? |
yadvr
commented
Oct 25, 2016
Member
@karuturi there was a capacity issue at the backend, the tests have been re-fired. |
karuturi
commented
Oct 26, 2016
Member
@rhtyd any update on the tests? |
yadvr
commented
Oct 26, 2016
Member
@karuturi tests are about to finish |
karuturi
commented
Oct 26, 2016
Member
ok. Thank you. |
karuturi
commented
Oct 26, 2016
Member
@blueorangutan test |
blueorangutan
commented
Oct 26, 2016
@karuturi a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Oct 26, 2016
Trillian test result (tid-191)
|
blueorangutan
commented
Oct 26, 2016
Trillian test result (tid-193)
|
cloudmonger
commented
Oct 27, 2016
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: |
karuturi
commented
Oct 27, 2016
Member
reran the test for delete account and it passes. |
asfgit pushed a commit
that referenced
this pull request
Oct 27, 2016
CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Make NFS version changeable in UIJIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-9438 ### Introduction From #1361 it was possible to configure NFS version for secondary storage mount. However, changing NFS version requires inserting an new detail on `image_store_details` table, with `name = 'nfs.version'` and `value = X` where X is desired NFS version, and then restarting management server for changes to take effect. Our improvement aims to make NFS version changeable from UI, instead of previously described workflow. ### Proposed solution Basically, NFS version is defined as an image store ConfigKey, this implied: * Adding a new Config scope: **ImageStore** * Make `ImageStoreDetailsDao` class to extend `ResourceDetailsDaoBase` and `ImageStoreDetailVO` implement `ResourceDetail` * Insert `'display'` column on `image_store_details` table * Extending `ListCfgsCmd` and `UpdateCfgCmd` to support **ImageStore** scope, which implied: ** Injecting `ImageStoreDetailsDao` and `ImageStoreDao` on `ConfigurationManagerImpl` class, on `cloud-server` module. ### Important It is important to mention that `ImageStoreDaoImpl` and `ImageStoreDetailsDaoImpl` classes were moved from `cloud-engine-storage` to `cloud-engine-schema` module in order to Spring find those beans to inject on `ConfigurationManagerImpl` in `cloud-server` module. We had this maven dependencies between modules: * `cloud-server --> cloud-engine-schema` * `cloud-engine-storage --> cloud-secondary-storage --> cloud-server` As `ImageStoreDaoImpl` and `ImageStoreDetailsDao` were defined in `cloud-engine-storage`, and they needed in `cloud-server` module, to be injected on `ConfigurationManagerImpl`, if we added dependency from `cloud-server` to `cloud-engine-storage` we would introduce a dependency cycle. To avoid this cycle, we moved those classes to `cloud-engine-schema` module * pr/1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Make NFS version changeable in UI Signed-off-by: Rajani Karuturi <rajani.karuturi@accelerite.com>
milamberspace
commented
Dec 13, 2016
Contributor
Hello @nvazquez |
serg38
commented
Dec 13, 2016
@milamberspace Column details is supposed to be added during 4.8 to 4.9 schema upgrade. Most likely when you upgraded to 4.9 this PR hadn't been merged yet. You can simply apply this statement before starting the 4.9 to 4.10 upgrade: |
milamberspace
commented
Dec 13, 2016
Contributor
@serg38 My CS 4.9.1.0 installation is a fresh installation not an upgrade from 4.8. |
serg38
commented
Dec 13, 2016
@milamberspace Can you check if file /usr/share/cloudstack-management/setup/db/schema-481to490.sql in your installation has the SQL alter image_store_details table line ? |
milamberspace
commented
Dec 13, 2016
Contributor
@serg38 Root cause of the issue found. |
serg38
commented
Dec 13, 2016
nvazquez
commented
Dec 13, 2016
ContributorAuthor
Hi @milamberspace, sorry for my late response, I'm out of work this week but please let me know if there's something I can do to help. |
yadvr added a commit
to shapeblue/cloudstack
that referenced
this pull request
Dec 15, 2016
- Fixes issue of failing upgrade paths - Moves schema changes from PR apache#1615 in the 4.9.1.0 to 4.10.0.0 sql path Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
asfgit pushed a commit
that referenced
this pull request
Dec 15, 2016
CLOUDSTACK-9671: Fix sql change to corresponding version paths- Fixes issue of failing upgrade paths - Moves schema changes from PR #1615 in the 4.9.1.0 to 4.10.0.0 sql path * pr/1831: CLOUDSTACK-9671: Fix sql change to corresponding version paths Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
11 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.
JIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-9438
Introduction
From #1361 it was possible to configure NFS version for secondary storage mount.
However, changing NFS version requires inserting an new detail on
image_store_detailstable, withname = 'nfs.version'andvalue = Xwhere X is desired NFS version, and then restarting management server for changes to take effect.Our improvement aims to make NFS version changeable from UI, instead of previously described workflow.
Proposed solution
Basically, NFS version is defined as an image store ConfigKey, this implied:
ImageStoreDetailsDaoclass to extendResourceDetailsDaoBaseandImageStoreDetailVOimplementResourceDetail'display'column onimage_store_detailstableListCfgsCmdandUpdateCfgCmdto support ImageStore scope, which implied:** Injecting
ImageStoreDetailsDaoandImageStoreDaoonConfigurationManagerImplclass, oncloud-servermodule.Important
It is important to mention that
ImageStoreDaoImplandImageStoreDetailsDaoImplclasses were moved fromcloud-engine-storagetocloud-engine-schemamodule in order to Spring find those beans to inject onConfigurationManagerImplincloud-servermodule.We had this maven dependencies between modules:
cloud-server --> cloud-engine-schemacloud-engine-storage --> cloud-secondary-storage --> cloud-serverAs
ImageStoreDaoImplandImageStoreDetailsDaowere defined incloud-engine-storage, and they needed incloud-servermodule, to be injected onConfigurationManagerImpl, if we added dependency fromcloud-servertocloud-engine-storagewe would introduce a dependency cycle. To avoid this cycle, we moved those classes tocloud-engine-schemamodule