You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/01/02 17:55:25 UTC
[GitHub] [cloudstack] skattoju4 opened a new pull request #3794: [WIP]
create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
skattoju4 opened a new pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794
## Description
Previously, volume access groups were assigned with the convention Cloudstack-RandomUUID. This change proposes that that we use Cloudstack-clusterId instead.
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## Screenshots (if appropriate):
## How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] mike-tutkowski commented on a change in pull request
#3794: [WIP] create Volume Access Groups per cluster instead of
CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
mike-tutkowski commented on a change in pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#discussion_r374402204
##########
File path: plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
##########
@@ -683,19 +694,58 @@ private static SolidFireVag getVagForRandomHostInCluster(List<SolidFireUtil.Soli
return null;
}
- public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnection, long sfVolumeId, List<HostVO> hosts) {
+ public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnection, long sfVolumeId, List<HostVO> hosts, String clusterUuId) {
Review comment:
Either way...my initial thoughts were that you could derive the list of hosts from using the cluster ID with the cluster DAO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3794: [WIP] create
Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-570786464
@DaanHoogland you are sure. its a draft. i think there will come some code. you are right we need to find a way that we dont break the existing setup. this is in our interest too. we need to find a way to change it because at the moment its not good as it is because it ensure problems with redeployment, upgrade and move of vms.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] rhtyd commented on issue #3794: [WIP] create Volume
Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
rhtyd commented on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-580662767
@skattoju4 is the PR ready for review/testing? Can you address outstanding review comments?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] rhtyd closed pull request #3794: [WIP] create Volume
Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
rhtyd closed pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] skattoju4 commented on issue #3794: [WIP] create
Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-581468132
@rhtyd waiting on @svenvogel to test in their environment...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] mike-tutkowski commented on issue #3794: [WIP] create
Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
mike-tutkowski commented on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-580455007
@skattoju4 Would you be able to summarize the problem this PR addresses? Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3794: [WIP] create
Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-570545695
@skattoju4 Hi Sid, is this ready for testing?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3794: [WIP] create Volume Access Groups per cluster instead of
CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#discussion_r372335887
##########
File path: plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
##########
@@ -692,39 +699,63 @@ public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnect
throw new CloudRuntimeException(errMsg);
}
+ // Get list of VAGs from solidfire
List<SolidFireUtil.SolidFireVag> sfVags = SolidFireUtil.getAllVags(sfConnection);
- Map<SolidFireUtil.SolidFireVag, List<String>> sfVagToIqnsMap = new HashMap<>();
+ // Populate vag to iqn(host) map
+ Map<SolidFireUtil.SolidFireVag, List<String>> sfVagToIqnsMap = buildVagtoIQNMap(hosts, sfVags);
- for (HostVO hostVO : hosts) {
- String iqn = hostVO.getStorageUrl();
+ // throw exception if more than 4 VAGs
+ if (sfVagToIqnsMap.size() > MAX_NUM_VAGS_PER_VOLUME) {
+ throw new CloudRuntimeException("A SolidFire volume can be in at most four volume access groups simultaneously.");
+ }
- SolidFireUtil.SolidFireVag sfVag = getVolumeAccessGroup(iqn, sfVags);
+ if (sfVagToIqnsMap.containsKey(null)) {
- List<String> iqnsInVag = sfVagToIqnsMap.computeIfAbsent(sfVag, k -> new ArrayList<>());
+ //Check if cluster VAG exists
+ SolidFireVag sfVagMatchingClusterId = sfVags.stream().filter(vag -> vag.getName().equals("CloudStack-"+clusterUuId)).findFirst().orElse(null);
- iqnsInVag.add(iqn);
- }
+ if (sfVagMatchingClusterId == null) {
+ // Create cluster VAG if doesnt exist
+ LOGGER.info("Creating volume access group CloudStack-"+clusterUuId);
+ SolidFireUtil.createVag(sfConnection, "CloudStack-"+clusterUuId, sfVagToIqnsMap.get(null).toArray(new String[0]), new long[] { sfVolumeId });
+ sfVags = SolidFireUtil.getAllVags(sfConnection);
+ sfVagMatchingClusterId = sfVags.stream().filter(vag -> vag.getName().equals("CloudStack-"+clusterUuId)).findFirst().orElse(null);
+ }
- if (sfVagToIqnsMap.size() > MAX_NUM_VAGS_PER_VOLUME) {
- throw new CloudRuntimeException("A SolidFire volume can be in at most four volume access groups simultaneously.");
+ //update null entry in vag to IQN Map
+ sfVagToIqnsMap.put(sfVagMatchingClusterId,sfVagToIqnsMap.get(null));
+ sfVagToIqnsMap.remove(null);
}
+
+ // add volumeId to each VAG if not already present
Review comment:
This comment is a good reason to factor the next bit code out for cleanup.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] mike-tutkowski commented on a change in pull request
#3794: [WIP] create Volume Access Groups per cluster instead of
CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
mike-tutkowski commented on a change in pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#discussion_r373234244
##########
File path: plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
##########
@@ -683,19 +694,58 @@ private static SolidFireVag getVagForRandomHostInCluster(List<SolidFireUtil.Soli
return null;
}
- public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnection, long sfVolumeId, List<HostVO> hosts) {
+ public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnection, long sfVolumeId, List<HostVO> hosts, String clusterUuId) {
if (!SolidFireUtil.hostsSupport_iScsi(hosts)) {
String errMsg = "Not all hosts in the compute cluster support iSCSI.";
LOGGER.warn(errMsg);
throw new CloudRuntimeException(errMsg);
}
-
List<SolidFireUtil.SolidFireVag> sfVags = SolidFireUtil.getAllVags(sfConnection);
+ Map<SolidFireUtil.SolidFireVag, List<String>> sfVagToIqnsMap = buildVagToIQNMap(hosts, sfVags);
+ if (sfVagToIqnsMap.size() > MAX_NUM_VAGS_PER_VOLUME) {
+ throw new CloudRuntimeException("A SolidFire volume can be in at most four volume access groups simultaneously.");
+ }
+ if (sfVagToIqnsMap.containsKey(null)) {
+ sfVagToIqnsMap = updateNullKeyInsfVagToIqnsMap(sfVagToIqnsMap, sfVags, sfConnection, clusterUuId, sfVolumeId);
+ }
+ addVoulumestoVagIfNotPresent(sfVagToIqnsMap.keySet(), sfVolumeId, sfConnection);
Review comment:
"Voulumes" should be "Volumes"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] skattoju4 commented on a change in pull request #3794:
[WIP] create Volume Access Groups per cluster instead of
CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on a change in pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#discussion_r374164919
##########
File path: plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
##########
@@ -683,19 +694,58 @@ private static SolidFireVag getVagForRandomHostInCluster(List<SolidFireUtil.Soli
return null;
}
- public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnection, long sfVolumeId, List<HostVO> hosts) {
+ public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnection, long sfVolumeId, List<HostVO> hosts, String clusterUuId) {
Review comment:
we need the list of hosts to build the vag to iqn map, we need the hostDao to get the list of hosts. I think it is better to just pass the list of hosts in this case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #3794: [WIP] create
Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-570558238
It sounds like an backwards incompatible change, judging by the description. How do I ensure the old behaviour remains, @skattoju4 ? (/cc @andrijapanicsb @svenvogel )
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3794: [WIP] create Volume Access Groups per cluster instead of
CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#discussion_r372341084
##########
File path: plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
##########
@@ -692,39 +699,63 @@ public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnect
throw new CloudRuntimeException(errMsg);
}
+ // Get list of VAGs from solidfire
List<SolidFireUtil.SolidFireVag> sfVags = SolidFireUtil.getAllVags(sfConnection);
- Map<SolidFireUtil.SolidFireVag, List<String>> sfVagToIqnsMap = new HashMap<>();
+ // Populate vag to iqn(host) map
Review comment:
great name of method call below makes this comment obsolete
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] skattoju4 commented on issue #3794: [WIP] create
Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-580466070
@mike-tutkowski i have updated the description let me know if you have questions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3794: [WIP] create Volume Access Groups per cluster instead of
CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#discussion_r372343069
##########
File path: plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
##########
@@ -692,39 +699,63 @@ public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnect
throw new CloudRuntimeException(errMsg);
}
+ // Get list of VAGs from solidfire
List<SolidFireUtil.SolidFireVag> sfVags = SolidFireUtil.getAllVags(sfConnection);
- Map<SolidFireUtil.SolidFireVag, List<String>> sfVagToIqnsMap = new HashMap<>();
+ // Populate vag to iqn(host) map
+ Map<SolidFireUtil.SolidFireVag, List<String>> sfVagToIqnsMap = buildVagtoIQNMap(hosts, sfVags);
- for (HostVO hostVO : hosts) {
- String iqn = hostVO.getStorageUrl();
+ // throw exception if more than 4 VAGs
Review comment:
in this case the code below is more descriptive than the comment above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3794: [WIP] create
Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-577725367
@skattoju4 we will test it. 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3794: [WIP] create Volume Access Groups per cluster instead of
CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#discussion_r372334935
##########
File path: plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
##########
@@ -513,12 +513,18 @@ public static void hostAddedToCluster(long hostId, long clusterId, String storag
// creating a new VAG won't exceed 4 VAGs for the computer cluster).
// If none of the hosts in the cluster are in a VAG, then leave this host out of a VAG.
// Place applicable volume IDs in VAG, if need be (account of volume starts with SF_CS_ACCOUNT_PREFIX).
- private static void handleVagForHost(SolidFireUtil.SolidFireConnection sfConnection, List<SolidFireUtil.SolidFireVag> sfVags, Host host, HostDao hostDao) {
+ private static void handleVagForHost(SolidFireUtil.SolidFireConnection sfConnection, List<SolidFireUtil.SolidFireVag> sfVags, Host host, HostDao hostDao, ClusterDao clusterDao) {
List<HostVO> hostVOs = hostDao.findByClusterId(host.getClusterId());
if (hostVOs != null) {
int numVags = 0;
+ String clusterUuId = clusterDao.findById(host.getClusterId()).getUuid();
+ SolidFireVag sfVagMatchingClusterId = sfVags.stream().filter(vag -> vag.getName().equals("CloudStack-"+clusterUuId)).findFirst().orElse(null);
+ if (sfVagMatchingClusterId != null && sfVagMatchingClusterId.getInitiators().length < MAX_NUM_INITIATORS_PER_VAG) {
+ addInitiatorsToSolidFireVag(sfConnection, sfVagMatchingClusterId.getId(), new String[]{host.getStorageUrl()});
+ }
Review comment:
can you make this a method with a good descriptive name?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] mike-tutkowski commented on a change in pull request
#3794: [WIP] create Volume Access Groups per cluster instead of
CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
mike-tutkowski commented on a change in pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#discussion_r373235404
##########
File path: plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
##########
@@ -683,19 +694,58 @@ private static SolidFireVag getVagForRandomHostInCluster(List<SolidFireUtil.Soli
return null;
}
- public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnection, long sfVolumeId, List<HostVO> hosts) {
+ public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnection, long sfVolumeId, List<HostVO> hosts, String clusterUuId) {
if (!SolidFireUtil.hostsSupport_iScsi(hosts)) {
String errMsg = "Not all hosts in the compute cluster support iSCSI.";
LOGGER.warn(errMsg);
throw new CloudRuntimeException(errMsg);
}
-
List<SolidFireUtil.SolidFireVag> sfVags = SolidFireUtil.getAllVags(sfConnection);
+ Map<SolidFireUtil.SolidFireVag, List<String>> sfVagToIqnsMap = buildVagToIQNMap(hosts, sfVags);
+ if (sfVagToIqnsMap.size() > MAX_NUM_VAGS_PER_VOLUME) {
+ throw new CloudRuntimeException("A SolidFire volume can be in at most four volume access groups simultaneously.");
+ }
+ if (sfVagToIqnsMap.containsKey(null)) {
+ sfVagToIqnsMap = updateNullKeyInsfVagToIqnsMap(sfVagToIqnsMap, sfVags, sfConnection, clusterUuId, sfVolumeId);
Review comment:
Might want to maintain camel case here:
"updateNullKeyInsfVagToIqnsMap" -> "updateNullKeyInSfVagToIqnsMap"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] skattoju4 opened a new pull request #3794: [WIP]
create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
skattoju4 opened a new pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794
## Description
Previously, volume access groups were assigned with the convention Cloudstack-RandomUUID. This change proposes that that we use Cloudstack-clusterUuId instead.
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## Screenshots (if appropriate):
## How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
### Scenario where new random volume access groups are created
Steps to reproduce in test environment with two hosts:
1. - Create VMs
2. - Remove all hosts
3. - Re-Add hosts
4. - Start VMs
_Outcome before applying this patch:_
A new volume access group is created with the convention CloudStack-RandomUUID
_After applying this patch:_
If an existing volume access group with the naming convention CloudStack-clusterUuId exists, it will be used otherwise a volume access group with this naming convention will be created.
### Backward Compatibility test
1. - initial scenario old code, two hosts, volume access group name with random-uuid
2. - switch to new code
3. - remove one host
4. - re-add host
_Outcome before applying this patch:_
no new volume access group created, existing volume access group with random-uuid is used.
_After applying this patch:_
no new volume access group created, existing volume access group with random-uuid is used.
A scenario with one host in the new group and one in the old does not occur for the above tests
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3794: [WIP] create Volume Access Groups per cluster instead of
CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#discussion_r372335531
##########
File path: plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
##########
@@ -692,39 +699,63 @@ public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnect
throw new CloudRuntimeException(errMsg);
}
+ // Get list of VAGs from solidfire
List<SolidFireUtil.SolidFireVag> sfVags = SolidFireUtil.getAllVags(sfConnection);
- Map<SolidFireUtil.SolidFireVag, List<String>> sfVagToIqnsMap = new HashMap<>();
+ // Populate vag to iqn(host) map
+ Map<SolidFireUtil.SolidFireVag, List<String>> sfVagToIqnsMap = buildVagtoIQNMap(hosts, sfVags);
- for (HostVO hostVO : hosts) {
- String iqn = hostVO.getStorageUrl();
+ // throw exception if more than 4 VAGs
+ if (sfVagToIqnsMap.size() > MAX_NUM_VAGS_PER_VOLUME) {
+ throw new CloudRuntimeException("A SolidFire volume can be in at most four volume access groups simultaneously.");
+ }
- SolidFireUtil.SolidFireVag sfVag = getVolumeAccessGroup(iqn, sfVags);
+ if (sfVagToIqnsMap.containsKey(null)) {
- List<String> iqnsInVag = sfVagToIqnsMap.computeIfAbsent(sfVag, k -> new ArrayList<>());
+ //Check if cluster VAG exists
+ SolidFireVag sfVagMatchingClusterId = sfVags.stream().filter(vag -> vag.getName().equals("CloudStack-"+clusterUuId)).findFirst().orElse(null);
- iqnsInVag.add(iqn);
- }
+ if (sfVagMatchingClusterId == null) {
+ // Create cluster VAG if doesnt exist
+ LOGGER.info("Creating volume access group CloudStack-"+clusterUuId);
+ SolidFireUtil.createVag(sfConnection, "CloudStack-"+clusterUuId, sfVagToIqnsMap.get(null).toArray(new String[0]), new long[] { sfVolumeId });
+ sfVags = SolidFireUtil.getAllVags(sfConnection);
+ sfVagMatchingClusterId = sfVags.stream().filter(vag -> vag.getName().equals("CloudStack-"+clusterUuId)).findFirst().orElse(null);
+ }
Review comment:
"Create cluster VAG if doesnt exist", would make for a nice method name ;) CreateClusterVAGIfDoesntExist(); ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] skattoju4 edited a comment on issue #3794: [WIP]
create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
skattoju4 edited a comment on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-571288019
@andrijapanicsb @DaanHoogland this is still a work in progress. Im trying to ensure that it will not break existing setups. Will add some more notes as a I progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel edited a comment on issue #3794: [WIP]
create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
svenvogel edited a comment on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-570786464
@DaanHoogland you are right. its a draft. i think there will come some code. you are right we need to find a way that we dont break the existing setup. this is in our interest too. we need to find a way to change it because at the moment its not good as it is because it ensure problems with redeployment, upgrade and move of vms.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] skattoju4 commented on issue #3794: [WIP] create
Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-571288019
@andrijapanic @DaanHoogland this is still a work in progress. Im trying to ensure that it will not break existing setups. Will add some more notes as a I progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] andrijapanicsb commented on issue #3794: [WIP] create
Volume Access Groups per cluster instead of CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on issue #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#issuecomment-571275671
@skattoju3 @skattoju4 this name change is only applicable for new ACS+SF installations, simply the name of the VAG (that is created when SF is added to ACS for the first time) name will not be Cloudstack-RandomUUID, but Cloudstack-clusterId instead.
If I read this correctly, this:
- will not affect existing ACS+SF setups since VAG is already there, so ACS will just use it via whatever name is there?
- is applicable only for new ACS+SF setups
Can you confirm it doesn't break any existing setups in that sense?
thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] mike-tutkowski commented on a change in pull request
#3794: [WIP] create Volume Access Groups per cluster instead of
CloudStack-RandomUUID()
Posted by GitBox <gi...@apache.org>.
mike-tutkowski commented on a change in pull request #3794: [WIP] create Volume Access Groups per cluster instead of CloudStack-RandomUUID()
URL: https://github.com/apache/cloudstack/pull/3794#discussion_r373234663
##########
File path: plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/util/SolidFireUtil.java
##########
@@ -683,19 +694,58 @@ private static SolidFireVag getVagForRandomHostInCluster(List<SolidFireUtil.Soli
return null;
}
- public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnection, long sfVolumeId, List<HostVO> hosts) {
+ public static void placeVolumeInVolumeAccessGroups(SolidFireConnection sfConnection, long sfVolumeId, List<HostVO> hosts, String clusterUuId) {
Review comment:
Do we want to pass in a List<HostVO> still if we can now retrieve that info from the cluster DAO using the cluster UUID?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services