CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Make NFS version changeable in UI #1615
Conversation
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
nvazquez
force-pushed
the
nfsConfigKey
branch
6 times, most recently
from
aa02196 to
f3a5e12
Compare
| @@ -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?
Contributor
Author
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
Contributor
|
LGTM based on code review |
Contributor
Author
|
Thanks @koushik-das! |
|
Ping for second review -- @GabrielBrascher, @rhtyd, @wido, @rafaelweingartner |
|
Alright, your PR did not break packaging! Packages built and available at: http://packages.shapeblue.com/cloudstack/custom/github-1615 |
nvazquez
force-pushed
the
nfsConfigKey
branch
2 times, most recently
from
bb5c7c0 to
a8b856c
Compare
Member
|
@blueorangutan kick |
|
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.
|
Packaging result: ✔centos6 ✔centos7 ✔debian repo: http://packages.shapeblue.com/cloudstack/pr/1615 |
Contributor
|
@nvazquez looking good. One small item on the |
Contributor
Author
|
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 |
Contributor
Author
|
Sure, done |
Contributor
|
@blueorangutan help |
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. |
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. |
Member
|
@blueorangutan help |
|
@karuturi I understand these words: "help", "hello", "thanks", "package", "test" |
Member
|
@blueorangutan test centos7 vmware-60u2 |
|
@karuturi a Trillian-Jenkins test job (centos7 mgmt + vmware-60u2) has been kicked to run smoke tests |
Member
|
@blueorangutan @rhtyd any update on the tests? Is the job running? |
Member
|
@karuturi there was a capacity issue at the backend, the tests have been re-fired. |
Member
|
@rhtyd any update on the tests? |
Member
|
@karuturi tests are about to finish |
Member
|
ok. Thank you. |
Member
|
@blueorangutan test |
|
@karuturi a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-191)
|
|
Trillian test result (tid-193)
|
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: |
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 <[email protected]>
Contributor
|
Hello @nvazquez |
|
@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: |
Contributor
|
@serg38 My CS 4.9.1.0 installation is a fresh installation not an upgrade from 4.8. |
|
@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 ? |
Contributor
|
@serg38 Root cause of the issue found. |
Contributor
Author
|
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 <[email protected]>
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 <[email protected]>
Sign up for free
to 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