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/10/23 16:46:57 UTC
[GitHub] [cloudstack] kioie opened a new pull request #4010: New API endpoint to update size of Pod Management IP Range.
kioie opened a new pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010
## Description
<!--- Describe your changes in detail -->
**New API endpoint to update size of Pod Management IP Range.**
Currently, pod management IP range can only be expanded under the same subnet. Editing the current pod IP management range is a challenge, especially as the list of IP ranges is greater than one. The purpose of this feature is to allow ADMINS to be able to update size of IP range of management pod, upwards or downwards.
PR #2048 and [FS](https://cwiki.apache.org/confluence/display/CLOUDSTACK/Expansion+of+Management+IP+Range) added two APIs(`createManagementNetworkIpRange` & `deleteManagementNetworkIpRange`). This feature is now in addition to the two APIs.
**Brief description**
- Updatepodmanagementnetworkiprange will update already existing IP ranges. Will not create a new IP range.
- Will increase/reduce available IPs within the range.
- PodID/Currentstartip/CurrentendIp are mandatory fields. Newstartip and Newendip are optional fields and when left blank, Newstartip will be set to Currentstartip and Newendip will be set to Currentendip.
- Api only available to root admin and does not have any sensitive information passed.
- Will output success response -true/false
- Does not alter structure of DB, but will update contents.
**When to use**
- Reducing size of existing IP range
- Increasing size of existing IP range
**How to use**
Run `list pods` command to see existing IP ranges
```
> list pods
{
"count": 1,
"pod": [
{
"allocationstate": "Enabled",
"endip": [
"172.16.15.210",
"172.16.15.204"
],
"forsystemvms": [
"0",
"0"
],
"gateway": "172.16.15.1",
"id": "1407da22-a131-4dbd-86a3-45210a26897e",
"name": "POD0",
"netmask": "255.255.255.0",
"startip": [
"172.16.15.205",
"172.16.15.2"
],
"vlanid": [
"vlan://untagged",
"vlan://untagged"
],
"zoneid": "3ab731d6-efd5-418c-ab2b-cacbfc4b88f0",
"zonename": "Sandbox-simulator"
}
]
}
```
2. Update the IP range you'd like to change. In our case, we want to increase the range `172.16.15.205-172.16.15.210` to `172.16.15.205-172.16.15.215`. Use the command `update podmanagementnetworkiprange`
```
> update podmanagementnetworkiprange podid=1407da22-a131-4dbd-86a3-45210a26897e currentstartip=172.16.15.205 currentendip=172.16.15.210 newendip=172.16.15.215
{
"success": true
}
```
To reduce size of range to `172.16.15.205-172.16.15.207`
```
> update podmanagementnetworkiprange podid=1407da22-a131-4dbd-86a3-45210a26897e currentstartip=172.16.15.205 currentendip=172.16.15.215 newendip=172.16.15.207
{
"success": true
}
```
Confirm by running `list pods` again
```
> list pods
{
"count": 1,
"pod": [
{
"allocationstate": "Enabled",
"endip": [
"172.16.15.207",
"172.16.15.204"
],
"forsystemvms": [
"0",
"0"
],
"gateway": "172.16.15.1",
"id": "1407da22-a131-4dbd-86a3-45210a26897e",
"name": "POD0",
"netmask": "255.255.255.0",
"startip": [
"172.16.15.205",
"172.16.15.2"
],
"vlanid": [
"vlan://untagged",
"vlan://untagged"
],
"zoneid": "3ab731d6-efd5-418c-ab2b-cacbfc4b88f0",
"zonename": "Sandbox-simulator"
}
]
}
```
Touches on #2797 #2735 #3292
<!-- 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)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## Tests
<!-- 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. -->
**Manual Tests**
Test for invalid parameters for Currentstartip/Currentendip/Newstartip/Newendip
Test for invalid current IP range
Test for overlapping IP range extensions
Test for non-existent IP range
Test for Newstartip>Newendip
Test for duplicate IP range
Test for invalid Pod ID
Test- Reduce existing IP range to not include already allocated IPs
<!-- 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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-712791939
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2215
----------------------------------------------------------------
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
[GitHub] [cloudstack] kioie closed pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
kioie closed pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-653679477
Packaging result: ✖centos7 ✖debian. JID-1518
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713050941
@blueorangutan package
----------------------------------------------------------------
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
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-653955629
@blueorangutan package
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713096400
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland closed pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland closed pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-712767324
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#discussion_r661346396
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/network/UpdatePodManagementNetworkIpRangeCmd.java
##########
@@ -0,0 +1,150 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.api.command.admin.network;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiArgValidator;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseAsyncCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.PodResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.log4j.Logger;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.user.Account;
+
+@APICommand(name = UpdatePodManagementNetworkIpRangeCmd.APINAME,
+ description = "Updates a management network IP range. Only allowed when no IPs are allocated.",
+ responseObject = SuccessResponse.class,
+ since = "4.15.0.0",
Review comment:
```suggestion
since = "4.16.0.0",
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] kioie closed pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
kioie closed pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713352866
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make 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
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-658183292
I will see if I can do a few tests on this one this week. And then I will give an official `+1`.
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713435480
@blueorangutan package
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713902505
<b>Trillian test result (tid-3034)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39322 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4010-t3034-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 85 look OK, 0 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
----------------------------------------------------------------
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
[GitHub] [cloudstack] sureshanaparti commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-906182782
Hi @kioie, any update on the outstanding 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-724922980
@blueorangutan package
----------------------------------------------------------------
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
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-658180971
@kioie sorry for the late reply. I picked the branch to do a few checks (it is easier than reviewing solely via git diff). Most of the code is already concise. So ignore the part of being too big.
Additionally, in theory, methods that you are using should already have their own test cases (not your fault, so feel free to ignore it), Thus testing updatePodIpRange would be a matter of running a verify checking if the methods are called on the correct order..
```
verifyIpRangeParameters(currentStartIP,currentEndIP);
verifyIpRangeParameters(newStartIP,newEndIP);
checkIpRangeContainsTakenAddresses(pod,currentStartIP,currentEndIP,newStartIP,newEndIP);
String vlan = verifyPodIpRangeExists(podId,existingPodIpRanges,currentStartIP,currentEndIP,newStartIP,newEndIP);
```
I would suggest adding a few test cases on `verifyPodIpRangeExists`, if possible. And maybe changing the name of `verifyPodIpRangeExists` to something as `retrievePodIpRangeVlan` or `validatePodIpRangeAndRetrieveVlan`.
But overall looks good.
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-653959383
Packaging result: ✔centos7 ✔debian. JID-1535
----------------------------------------------------------------
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
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#discussion_r660547507
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/network/UpdatePodManagementNetworkIpRangeCmd.java
##########
@@ -0,0 +1,150 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.api.command.admin.network;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiArgValidator;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseAsyncCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.PodResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.log4j.Logger;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.user.Account;
+
+@APICommand(name = UpdatePodManagementNetworkIpRangeCmd.APINAME,
+ description = "Updates a management network IP range. Only allowed when no IPs are allocated.",
+ responseObject = SuccessResponse.class,
+ since = "4.15.0.0",
Review comment:
Update release here, as applicable to this PR (may be 4.16.0.0) ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-653678177
@blueorangutan package
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713474353
@blueorangutan test
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713436089
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make 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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-725244734
<b>Trillian test result (tid-3145)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32779 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4010-t3145-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Smoke tests completed. 86 look OK, 0 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
----------------------------------------------------------------
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
[GitHub] [cloudstack] ravening commented on a change in pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#discussion_r521254025
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -1463,6 +1464,175 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
}
}
+ @Override
+ @DB
+ public void updatePodIpRange(final UpdatePodManagementNetworkIpRangeCmd cmd) throws ConcurrentOperationException {
+ final long podId = cmd.getPodId();
+ final String currentStartIP = cmd.getCurrentStartIP();
+ final String currentEndIP = cmd.getCurrentEndIP();
+ final HostPodVO pod = _podDao.findById(podId);
+
+ String newStartIP = cmd.getNewStartIP();
+ String newEndIP = cmd.getNewEndIP();
+
+ if (newStartIP == null) {
+ newStartIP = currentStartIP;
+ }
+
+ if (newEndIP == null) {
+ newEndIP = currentEndIP;
+ }
+
+ if (pod == null) {
+ throw new InvalidParameterValueException("Unable to find pod by id " + podId);
+ }
+
+ final String[] existingPodIpRanges = pod.getDescription().split(",");
+ if (existingPodIpRanges.length == 0) {
+ throw new InvalidParameterValueException("The IP range cannot be found since the existing IP range is empty.");
+ }
+
+ verifyIpRangeParameters(currentStartIP,currentEndIP);
+ verifyIpRangeParameters(newStartIP,newEndIP);
+ checkIpRangeContainsTakenAddresses(pod,currentStartIP,currentEndIP,newStartIP,newEndIP);
+
+ String vlan = verifyPodIpRangeExists(podId,existingPodIpRanges,currentStartIP,currentEndIP,newStartIP,newEndIP);
+
+ List<Long> currentIpRange = listAllIPsWithintheRange(currentStartIP,currentEndIP);
+ List<Long> newIpRange = listAllIPsWithintheRange(newStartIP,newEndIP);
+
+ try {
+ final String finalNewEndIP = newEndIP;
+ final String finalNewStartIP = newStartIP;
+ final Integer vlanId = vlan.equals(Vlan.UNTAGGED) ? null : Integer.parseInt(vlan);
+
+ Transaction.execute(new TransactionCallbackNoReturn() {
+ @Override
+ public void doInTransactionWithoutResult(final TransactionStatus status) {
+ final long zoneId = pod.getDataCenterId();
+ pod.setDescription(pod.getDescription().replace(currentStartIP + "-",
+ finalNewStartIP + "-").replace(currentEndIP, finalNewEndIP));
+ updatePodIpRangeInDb(zoneId,podId,vlanId,pod,newIpRange,currentIpRange);
+ }
+ });
+ } catch (final Exception e) {
+ s_logger.error("Unable to update Pod " + podId + " IP range due to " + e.getMessage(), e);
+ throw new CloudRuntimeException("Failed to update Pod " + podId + " IP range. Please contact Cloud Support.");
+ }
+ }
+
+ private String verifyPodIpRangeExists(long podId,String[] existingPodIpRanges, String currentStartIP,
+ String currentEndIP, String newStartIP, String newEndIP) {
+ boolean foundRange = false;
+ String vlan = null;
+
+ for (String podIpRange: existingPodIpRanges) {
+ final String[] existingPodIpRange = podIpRange.split("-");
+
+ if (existingPodIpRange.length > 1) {
+ if (!NetUtils.isValidIp4(existingPodIpRange[0]) || !NetUtils.isValidIp4(existingPodIpRange[1])) {
+ continue;
+ }
+ if (currentStartIP.equals(existingPodIpRange[0]) && currentEndIP.equals(existingPodIpRange[1])) {
+ foundRange = true;
+ vlan = existingPodIpRange[3];
+ }
+ if (!foundRange && NetUtils.ipRangesOverlap(newStartIP, newEndIP, existingPodIpRange[0], existingPodIpRange[1])) {
+ throw new InvalidParameterValueException("The Start IP and EndIP address range overlap with private IP :" + existingPodIpRange[0] + "-" + existingPodIpRange[1]);
Review comment:
Small optimization is you can just display `podIpRange` instead of [0] and [1]
----------------------------------------------------------------
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
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713823658
@svenvogel @DaanHoogland any thoughts on this PR? It looks like a good one to be merged. I will see if I get some time to run a couple of manual tests.
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-712767716
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make 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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-724923504
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make 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
[GitHub] [cloudstack] kioie commented on a change in pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
kioie commented on a change in pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#discussion_r510874088
##########
File path: engine/schema/src/main/java/com/cloud/dc/dao/DataCenterIpAddressDaoImpl.java
##########
@@ -238,6 +237,17 @@ public void releaseIpAddress(long nicId) {
return listBy(sc);
}
+ @Override
+ public List<DataCenterIpAddressVO> listIpAddressUsage(final long podId, final long dcId, final boolean onlyListAllocated) {
+ SearchCriteria<DataCenterIpAddressVO> sc = createSearchCriteria();
+ if(onlyListAllocated) {
+ sc.addAnd("takenAt", SearchCriteria.Op.NNULL);
+ }
+ sc.addAnd("podId", SearchCriteria.Op.EQ, podId);
+ sc.addAnd("dataCenterId", SearchCriteria.Op.EQ, dcId);
+ return listBy(sc);
+ }
Review comment:
changed!
----------------------------------------------------------------
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
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#discussion_r454362083
##########
File path: engine/schema/src/main/java/com/cloud/dc/dao/DataCenterIpAddressDaoImpl.java
##########
@@ -238,6 +237,17 @@ public void releaseIpAddress(long nicId) {
return listBy(sc);
}
+ @Override
+ public List<DataCenterIpAddressVO> listIpAddressUsage(final long podId, final long dcId, final boolean onlyListAllocated) {
+ SearchCriteria<DataCenterIpAddressVO> sc = createSearchCriteria();
+ if(onlyListAllocated) {
+ sc.addAnd("takenAt", SearchCriteria.Op.NNULL);
+ }
+ sc.addAnd("podId", SearchCriteria.Op.EQ, podId);
+ sc.addAnd("dataCenterId", SearchCriteria.Op.EQ, dcId);
+ return listBy(sc);
+ }
Review comment:
I just noticed a minor adjustment on line 246.
```
if (onlyListAllocated) {
sc.addAnd("takenAt", SearchCriteria.Op.NNULL);
}
sc.addAnd("podId", SearchCriteria.Op.EQ, podId);
sc.addAnd("dataCenterId", SearchCriteria.Op.EQ, dcId);
return listBy(sc);
```
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713464096
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2247
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713475135
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-712767324
@blueorangutan package
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-724973516
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
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
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#discussion_r660547507
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/network/UpdatePodManagementNetworkIpRangeCmd.java
##########
@@ -0,0 +1,150 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.api.command.admin.network;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiArgValidator;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseAsyncCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.PodResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.log4j.Logger;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.user.Account;
+
+@APICommand(name = UpdatePodManagementNetworkIpRangeCmd.APINAME,
+ description = "Updates a management network IP range. Only allowed when no IPs are allocated.",
+ responseObject = SuccessResponse.class,
+ since = "4.15.0.0",
Review comment:
Update release here, as applicable to this PR (may be 4.16.0.0) ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713352866
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make 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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713372878
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2240
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-921616696
good @sureshanaparti
> @nvazquez @DaanHoogland Created PR #5458 (with all commits here) and addressed the outstanding comments there.
> If no objections, Can we close this PR.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] kioie commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
kioie commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-654101067
> This is an interesting addition, thanks for the PR @kioie 👍.
>
> I have a few points that I would like to be checked:
>
> 1. check code style, a few code pieces are not aligned with CloudStack code conventions
>
> 2. I think that this code can be even better if breaking the update method into smaller methods
>
> 3. if possible, can you please add JUnit test cases on [ConfigurationManagerTest.java](https://github.com/apache/cloudstack/blob/master/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java)?
>
>
> Regarding 2: small and concise methods, with self-explaining names and Javadoc, helps when reviewing, debugging, and maintaining the code base; additionally, with smaller methods, it is easy to create JUnit test case methods.
Thanks.
Regarding the tests cases, any suggestions on which methods I should consider?
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-715899242
ok, this is pending volunteers to test. @kioie a marvin test would probably by fine as well.
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland removed a comment on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713352609
@blueorangutan package
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713051421
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make 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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713352609
@blueorangutan package
----------------------------------------------------------------
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
[GitHub] [cloudstack] sureshanaparti commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-876958904
ping @kioie can you fix the outstanding 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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-653678257
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make 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
[GitHub] [cloudstack] sureshanaparti commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-920610709
@nvazquez @DaanHoogland Created PR #5458 (with all commits here) and addressed the outstanding comments there.
If no objections, Can we close this PR.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713096400
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2233
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-724972704
@blueorangutan test
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-724969475
Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2354
----------------------------------------------------------------
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
[GitHub] [cloudstack] ravening commented on a change in pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#discussion_r521248962
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -1463,6 +1464,175 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
}
}
+ @Override
+ @DB
+ public void updatePodIpRange(final UpdatePodManagementNetworkIpRangeCmd cmd) throws ConcurrentOperationException {
+ final long podId = cmd.getPodId();
+ final String currentStartIP = cmd.getCurrentStartIP();
+ final String currentEndIP = cmd.getCurrentEndIP();
+ final HostPodVO pod = _podDao.findById(podId);
+
+ String newStartIP = cmd.getNewStartIP();
+ String newEndIP = cmd.getNewEndIP();
+
+ if (newStartIP == null) {
+ newStartIP = currentStartIP;
+ }
+
+ if (newEndIP == null) {
+ newEndIP = currentEndIP;
+ }
+
+ if (pod == null) {
+ throw new InvalidParameterValueException("Unable to find pod by id " + podId);
+ }
+
+ final String[] existingPodIpRanges = pod.getDescription().split(",");
+ if (existingPodIpRanges.length == 0) {
+ throw new InvalidParameterValueException("The IP range cannot be found since the existing IP range is empty.");
+ }
+
+ verifyIpRangeParameters(currentStartIP,currentEndIP);
+ verifyIpRangeParameters(newStartIP,newEndIP);
+ checkIpRangeContainsTakenAddresses(pod,currentStartIP,currentEndIP,newStartIP,newEndIP);
+
+ String vlan = verifyPodIpRangeExists(podId,existingPodIpRanges,currentStartIP,currentEndIP,newStartIP,newEndIP);
+
+ List<Long> currentIpRange = listAllIPsWithintheRange(currentStartIP,currentEndIP);
+ List<Long> newIpRange = listAllIPsWithintheRange(newStartIP,newEndIP);
+
+ try {
+ final String finalNewEndIP = newEndIP;
+ final String finalNewStartIP = newStartIP;
+ final Integer vlanId = vlan.equals(Vlan.UNTAGGED) ? null : Integer.parseInt(vlan);
+
+ Transaction.execute(new TransactionCallbackNoReturn() {
+ @Override
+ public void doInTransactionWithoutResult(final TransactionStatus status) {
+ final long zoneId = pod.getDataCenterId();
+ pod.setDescription(pod.getDescription().replace(currentStartIP + "-",
+ finalNewStartIP + "-").replace(currentEndIP, finalNewEndIP));
+ updatePodIpRangeInDb(zoneId,podId,vlanId,pod,newIpRange,currentIpRange);
+ }
+ });
+ } catch (final Exception e) {
+ s_logger.error("Unable to update Pod " + podId + " IP range due to " + e.getMessage(), e);
+ throw new CloudRuntimeException("Failed to update Pod " + podId + " IP range. Please contact Cloud Support.");
+ }
+ }
+
+ private String verifyPodIpRangeExists(long podId,String[] existingPodIpRanges, String currentStartIP,
+ String currentEndIP, String newStartIP, String newEndIP) {
+ boolean foundRange = false;
+ String vlan = null;
+
+ for (String podIpRange: existingPodIpRanges) {
+ final String[] existingPodIpRange = podIpRange.split("-");
+
+ if (existingPodIpRange.length > 1) {
+ if (!NetUtils.isValidIp4(existingPodIpRange[0]) || !NetUtils.isValidIp4(existingPodIpRange[1])) {
+ continue;
+ }
+ if (currentStartIP.equals(existingPodIpRange[0]) && currentEndIP.equals(existingPodIpRange[1])) {
+ foundRange = true;
+ vlan = existingPodIpRange[3];
+ }
+ if (!foundRange && NetUtils.ipRangesOverlap(newStartIP, newEndIP, existingPodIpRange[0], existingPodIpRange[1])) {
+ throw new InvalidParameterValueException("The Start IP and EndIP address range overlap with private IP :" + existingPodIpRange[0] + "-" + existingPodIpRange[1]);
+ }
+ }
+ }
+
+ if (!foundRange) {
+ throw new InvalidParameterValueException("The input IP range: " + currentStartIP + "-" + currentEndIP + " of pod: " + podId + " is not present. Please input an existing range.");
+ }
+
+ return vlan;
+ }
+
+ private void updatePodIpRangeInDb (long zoneId, long podId, Integer vlanId, HostPodVO pod, List<Long> newIpRange, List<Long> currentIpRange) {
+ HostPodVO lock = null;
+ try {
+ lock = _podDao.acquireInLockTable(podId);
+ if (lock == null) {
+ String msg = "Unable to acquire lock on table to update the ip range of POD: " + pod.getName() + ", Update failed.";
+ s_logger.warn(msg);
+ throw new CloudRuntimeException(msg);
+ }
+ List<Long> iPaddressesToAdd = new ArrayList(newIpRange);
+ iPaddressesToAdd.removeAll(currentIpRange);
+ if (iPaddressesToAdd.size() > 0) {
+ for (Long startIP : iPaddressesToAdd) {
+ _zoneDao.addPrivateIpAddress(zoneId, podId, NetUtils.long2Ip(startIP), NetUtils.long2Ip(startIP), false, vlanId);
+ }
+ } else {
+ currentIpRange.removeAll(newIpRange);
+ if (currentIpRange.size() > 0) {
+ for (Long startIP: currentIpRange) {
+ if (!_privateIpAddressDao.deleteIpAddressByPodDc(NetUtils.long2Ip(startIP),podId,zoneId)) {
+ throw new CloudRuntimeException("Failed to remove private ip address: " + NetUtils.long2Ip(startIP) + " of Pod: " + podId + " DC: " + pod.getDataCenterId());
+ }
+ }
+ }
+ }
+ _podDao.update(podId, pod);
+ } catch (final Exception e) {
+ s_logger.error("Unable to update Pod " + podId + " IP range due to database error " + e.getMessage(), e);
+ throw new CloudRuntimeException("Failed to update Pod " + podId + " IP range. Please contact Cloud Support.");
+ } finally {
+ if (lock != null) {
+ _podDao.releaseFromLockTable(podId);
+ }
+ }
+ }
+
+ private List<Long> listAllIPsWithintheRange(String startIp, String endIP) {
+ verifyIpRangeParameters(startIp,endIP);
+ long startIpLong = NetUtils.ip2Long(startIp);
+ long endIpLong = NetUtils.ip2Long(endIP);
+
+ List<Long> listOfIpsinRange = new ArrayList<>();
+ while (startIpLong <= endIpLong) {
+ listOfIpsinRange.add(startIpLong);
+ startIpLong++;
+ }
+ return listOfIpsinRange;
+ }
+
+ private void verifyIpRangeParameters(String startIP, String endIp) {
+
+ if (!Strings.isNullOrEmpty(startIP) && !NetUtils.isValidIp4(startIP)) {
+ throw new InvalidParameterValueException("The current start address of the IP range " + startIP + " is not a valid IP address.");
+ }
+
+ if (!Strings.isNullOrEmpty(endIp) && !NetUtils.isValidIp4(endIp)) {
+ throw new InvalidParameterValueException("The current end address of the IP range " + endIp + " is not a valid IP address.");
+ }
+
+ if (NetUtils.ip2Long(startIP) > NetUtils.ip2Long(endIp)) {
+ throw new InvalidParameterValueException("The start IP address must have a lower value than the end IP address.");
+ }
+ }
+
+ private void checkIpRangeContainsTakenAddresses(final HostPodVO pod,final String currentStartIP,
+ final String currentEndIP,final String newStartIp, final String newEndIp) {
+
+ List<Long> newIpRange = listAllIPsWithintheRange(newStartIp,newEndIp);
Review comment:
If `newStartIp ` is null and `newEndIp ` is null then `newStartIp = currentStartIP` and `newEndIp = currentEndIP`
So the two new arrays `newIpRange` and `currentIpRange` are effecitvely same. Do you need to create two arrays in that 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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-725288545
ping @PaulAngus @rhtyd this looks ready, merge?
@GabrielBrascher @wido @ravening @weizhouapache anybody wants to verify more?
----------------------------------------------------------------
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
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#discussion_r449692454
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -1463,6 +1464,162 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
}
}
+ @Override
+ @DB
+ public void updatePodIpRange(final UpdatePodManagementNetworkIpRangeCmd cmd) throws ConcurrentOperationException {
+ final long podId = cmd.getPodId();
+ final String currentStartIP= cmd.getCurrentStartIP();
+ final String currentEndIP= cmd.getCurrentEndIP();
+ final HostPodVO pod = _podDao.findById(podId);
+
+ String newStartIP = cmd.getNewStartIP();
+ String newEndIP = cmd.getNewEndIP();
+ String vlan = null;
+
+ if(newStartIP == null){
+ newStartIP= currentStartIP;
+ }
+
+ if(newEndIP == null){
+ newEndIP= currentEndIP;
+ }
+
+ if(pod == null) {
+ throw new InvalidParameterValueException("Unable to find pod by id " + podId);
+ }
+
+ final String[] existingPodIpRanges = pod.getDescription().split(",");
+ if(existingPodIpRanges.length == 0) {
+ throw new InvalidParameterValueException("The IP range cannot be found since the existing IP range is empty.");
+ }
+
+ verifyIPRangeParameters(currentStartIP,currentEndIP);
+ verifyIPRangeParameters(newStartIP,newEndIP);
+ checkIpRangeContainsTakenAddresses(pod,currentStartIP,currentEndIP,newStartIP,newEndIP);
+
+ boolean foundRange = false;
+
+ for(String podIpRange: existingPodIpRanges) {
+ final String[] existingPodIpRange = podIpRange.split("-");
+
+ if(existingPodIpRange.length > 1) {
+ if (!NetUtils.isValidIp4(existingPodIpRange[0]) || !NetUtils.isValidIp4(existingPodIpRange[1])) {
+ continue;
+ }
+ if (currentStartIP.equals(existingPodIpRange[0]) && currentEndIP.equals(existingPodIpRange[1])) {
+ foundRange = true;
+ vlan = existingPodIpRange[3];
+ }
+ if (!foundRange && NetUtils.ipRangesOverlap(newStartIP, newEndIP, existingPodIpRange[0], existingPodIpRange[1])) {
+ throw new InvalidParameterValueException("The Start IP and EndIP address range overlap with private IP :" + existingPodIpRange[0] + "-" + existingPodIpRange[1]);
+ }
+ }
+ }
+
+ if(!foundRange) {
+ throw new InvalidParameterValueException("The input IP range: " + currentStartIP + "-" + currentEndIP + " of pod: " + podId + " is not present. Please input an existing range.");
+ }
+
+ List<Long> currentIPRange = listAllIPsWithintheRange(currentStartIP,currentEndIP);
+ List<Long> newIPRange = listAllIPsWithintheRange(newStartIP,newEndIP);
+
+ try {
+ final String finalNewEndIP = newEndIP;
+ final String finalNewStartIP = newStartIP;
+ final Integer vlanId = vlan.equals(Vlan.UNTAGGED) ? null : Integer.parseInt(vlan);
+
+ Transaction.execute(new TransactionCallbackNoReturn() {
+ @Override
+ public void doInTransactionWithoutResult(final TransactionStatus status) {
+ final long zoneId = pod.getDataCenterId();
+ pod.setDescription(pod.getDescription().replace(currentStartIP+"-", finalNewStartIP +"-").replace(currentEndIP,
+ finalNewEndIP));
+
+ HostPodVO lock = null;
+
+ try {
+ lock = _podDao.acquireInLockTable(podId);
+ if (lock == null) {
+ String msg = "Unable to acquire lock on table to update the ip range of POD: " + pod.getName() + ", Update failed.";
+ s_logger.warn(msg);
+ throw new CloudRuntimeException(msg);
+ }
+ List<Long> iPAddressesToAdd = new ArrayList(newIPRange);
+ iPAddressesToAdd.removeAll(currentIPRange);
+ if (iPAddressesToAdd.size()>0){
+ for(Long startIP : iPAddressesToAdd){
+ _zoneDao.addPrivateIpAddress(zoneId, podId, NetUtils.long2Ip(startIP), NetUtils.long2Ip(startIP), false, vlanId);
+ }
+ }else {
+ currentIPRange.removeAll(newIPRange);
+ if(currentIPRange.size()>0){
+ for (Long startIP: currentIPRange){
+ if(!_privateIpAddressDao.deleteIpAddressByPodDc(NetUtils.long2Ip(startIP),podId,zoneId)){
+ throw new CloudRuntimeException("Failed to remove private ip address: " + NetUtils.long2Ip(startIP) + " of Pod: " + podId + " DC: " + pod.getDataCenterId());
+ }
+ }
+ }
+ }
+ _podDao.update(podId, pod);
+ } finally {
Review comment:
Can you please update the code to reflect the [CloudStack coding conventions](https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions)?
Example of code formated:
```
if (iPAddressesToAdd.size() > 0) {
for (Long startIP : iPAddressesToAdd) {
_zoneDao.addPrivateIpAddress(zoneId, podId, NetUtils.long2Ip(startIP), NetUtils.long2Ip(startIP), false, vlanId);
}
} else {
currentIPRange.removeAll(newIPRange);
if (currentIPRange.size() > 0) {
for (Long startIP : currentIPRange) {
if (!_privateIpAddressDao.deleteIpAddressByPodDc(NetUtils.long2Ip(startIP), podId, zoneId)) {
throw new CloudRuntimeException(
"Failed to remove private ip address: " + NetUtils.long2Ip(startIP) + " of Pod: " + podId + " DC: " + pod.getDataCenterId());
}
}
}
}
```
Documentation: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -1463,6 +1464,162 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
}
}
+ @Override
+ @DB
+ public void updatePodIpRange(final UpdatePodManagementNetworkIpRangeCmd cmd) throws ConcurrentOperationException {
+ final long podId = cmd.getPodId();
+ final String currentStartIP= cmd.getCurrentStartIP();
+ final String currentEndIP= cmd.getCurrentEndIP();
+ final HostPodVO pod = _podDao.findById(podId);
+
+ String newStartIP = cmd.getNewStartIP();
+ String newEndIP = cmd.getNewEndIP();
+ String vlan = null;
+
+ if(newStartIP == null){
+ newStartIP= currentStartIP;
+ }
+
+ if(newEndIP == null){
+ newEndIP= currentEndIP;
+ }
+
+ if(pod == null) {
+ throw new InvalidParameterValueException("Unable to find pod by id " + podId);
+ }
+
+ final String[] existingPodIpRanges = pod.getDescription().split(",");
+ if(existingPodIpRanges.length == 0) {
+ throw new InvalidParameterValueException("The IP range cannot be found since the existing IP range is empty.");
+ }
+
+ verifyIPRangeParameters(currentStartIP,currentEndIP);
+ verifyIPRangeParameters(newStartIP,newEndIP);
+ checkIpRangeContainsTakenAddresses(pod,currentStartIP,currentEndIP,newStartIP,newEndIP);
+
+ boolean foundRange = false;
+
+ for(String podIpRange: existingPodIpRanges) {
+ final String[] existingPodIpRange = podIpRange.split("-");
+
+ if(existingPodIpRange.length > 1) {
+ if (!NetUtils.isValidIp4(existingPodIpRange[0]) || !NetUtils.isValidIp4(existingPodIpRange[1])) {
+ continue;
+ }
+ if (currentStartIP.equals(existingPodIpRange[0]) && currentEndIP.equals(existingPodIpRange[1])) {
+ foundRange = true;
+ vlan = existingPodIpRange[3];
+ }
+ if (!foundRange && NetUtils.ipRangesOverlap(newStartIP, newEndIP, existingPodIpRange[0], existingPodIpRange[1])) {
+ throw new InvalidParameterValueException("The Start IP and EndIP address range overlap with private IP :" + existingPodIpRange[0] + "-" + existingPodIpRange[1]);
+ }
+ }
+ }
+
+ if(!foundRange) {
+ throw new InvalidParameterValueException("The input IP range: " + currentStartIP + "-" + currentEndIP + " of pod: " + podId + " is not present. Please input an existing range.");
+ }
+
+ List<Long> currentIPRange = listAllIPsWithintheRange(currentStartIP,currentEndIP);
+ List<Long> newIPRange = listAllIPsWithintheRange(newStartIP,newEndIP);
+
+ try {
+ final String finalNewEndIP = newEndIP;
+ final String finalNewStartIP = newStartIP;
+ final Integer vlanId = vlan.equals(Vlan.UNTAGGED) ? null : Integer.parseInt(vlan);
+
+ Transaction.execute(new TransactionCallbackNoReturn() {
+ @Override
+ public void doInTransactionWithoutResult(final TransactionStatus status) {
+ final long zoneId = pod.getDataCenterId();
+ pod.setDescription(pod.getDescription().replace(currentStartIP+"-", finalNewStartIP +"-").replace(currentEndIP,
+ finalNewEndIP));
+
+ HostPodVO lock = null;
+
+ try {
+ lock = _podDao.acquireInLockTable(podId);
+ if (lock == null) {
+ String msg = "Unable to acquire lock on table to update the ip range of POD: " + pod.getName() + ", Update failed.";
+ s_logger.warn(msg);
+ throw new CloudRuntimeException(msg);
+ }
+ List<Long> iPAddressesToAdd = new ArrayList(newIPRange);
+ iPAddressesToAdd.removeAll(currentIPRange);
+ if (iPAddressesToAdd.size()>0){
+ for(Long startIP : iPAddressesToAdd){
+ _zoneDao.addPrivateIpAddress(zoneId, podId, NetUtils.long2Ip(startIP), NetUtils.long2Ip(startIP), false, vlanId);
+ }
+ }else {
+ currentIPRange.removeAll(newIPRange);
+ if(currentIPRange.size()>0){
+ for (Long startIP: currentIPRange){
+ if(!_privateIpAddressDao.deleteIpAddressByPodDc(NetUtils.long2Ip(startIP),podId,zoneId)){
+ throw new CloudRuntimeException("Failed to remove private ip address: " + NetUtils.long2Ip(startIP) + " of Pod: " + podId + " DC: " + pod.getDataCenterId());
+ }
+ }
+ }
+ }
+ _podDao.update(podId, pod);
+ } finally {
+ if (lock != null) {
+ _podDao.releaseFromLockTable(podId);
+ }
+ }
+ }
+ });
+ } catch (final Exception e) {
+ s_logger.error("Unable to update Pod " + podId + " IP range due to " + e.getMessage(), e);
+ throw new CloudRuntimeException("Failed to update Pod " + podId + " IP range. Please contact Cloud Support.");
+ }
+ }
+
+ public List<Long> listAllIPsWithintheRange(String startIp, String endIP){
+ verifyIPRangeParameters(startIp,endIP);
+ long startIPLong = NetUtils.ip2Long(startIp);
+ long endIPLong = NetUtils.ip2Long(endIP);
+
+ List<Long> listOfIpsinRange = new ArrayList<>();
+ while (startIPLong<=endIPLong){
+ listOfIpsinRange.add(startIPLong);
+ startIPLong++;
+ }
+ return listOfIpsinRange;
+ }
+
+ private void verifyIPRangeParameters(String startIP, String endIp){
+
+ if (!Strings.isNullOrEmpty(startIP) && !NetUtils.isValidIp4(startIP)) {
+ throw new InvalidParameterValueException("The current start address of the IP range "+startIP+" is not a valid IP address.");
+ }
+
+ if (!Strings.isNullOrEmpty(endIp) && !NetUtils.isValidIp4(endIp)) {
+ throw new InvalidParameterValueException("The current end address of the IP range "+endIp +" is not a valid IP address.");
+ }
+
+ if (NetUtils.ip2Long(startIP) > NetUtils.ip2Long(endIp)) {
+ throw new InvalidParameterValueException("The start IP address must have a lower value than the end IP address.");
+ }
+
+ }
+
+ private void checkIpRangeContainsTakenAddresses(final HostPodVO pod,final String currentStartIP, final String currentEndIP,final String newStartIp, final String newEndIp ){
+ List<Long> newIPRange = listAllIPsWithintheRange(newStartIp,newEndIp);
+ List<Long> currentIPRange = listAllIPsWithintheRange(currentStartIP,currentEndIP);
+ List<Long> takenIpsList = new ArrayList<>();
+ final List<DataCenterIpAddressVO> takenIps = _privateIpAddressDao.listIpAddressUsage(pod.getId(),pod.getDataCenterId(),true);
+
+ for (DataCenterIpAddressVO takenIp : takenIps) {
+ takenIpsList.add(NetUtils.ip2Long(takenIp.getIpAddress()));
+ }
+
+ takenIpsList.retainAll(currentIPRange);
+ if(!newIPRange.containsAll(takenIpsList)){
Review comment:
Can you please update the code to reflect the [CloudStack coding conventions](https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions)?
##########
File path: api/src/main/java/com/cloud/configuration/ConfigurationService.java
##########
@@ -203,6 +204,15 @@
*/
void deletePodIpRange(DeleteManagementNetworkIpRangeCmd cmd) throws ResourceUnavailableException, ConcurrentOperationException;
+ /**
+ * Updates a mutually exclusive IP range in the pod.
+ * @param cmd - The command specifying pod ID, current Start IP, current End IP, new Start IP, new End IP.
+ * @throws com.cloud.exception.ConcurrentOperationException
+ * @return Success
+ */
+
Review comment:
Thanks for adding a pice of documentation, that is a `+1` :-)
Can you please delete this empty line between the method and the Javadoc block?
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -1463,6 +1464,162 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
}
}
+ @Override
+ @DB
+ public void updatePodIpRange(final UpdatePodManagementNetworkIpRangeCmd cmd) throws ConcurrentOperationException {
+ final long podId = cmd.getPodId();
+ final String currentStartIP= cmd.getCurrentStartIP();
+ final String currentEndIP= cmd.getCurrentEndIP();
+ final HostPodVO pod = _podDao.findById(podId);
+
+ String newStartIP = cmd.getNewStartIP();
+ String newEndIP = cmd.getNewEndIP();
+ String vlan = null;
+
+ if(newStartIP == null){
+ newStartIP= currentStartIP;
+ }
Review comment:
Can you please update the code to reflect the [CloudStack coding conventions](https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions)?
```
if (newStartIP == null) {
newStartIP= currentStartIP;
}
```
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -1463,6 +1464,162 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
}
}
+ @Override
+ @DB
+ public void updatePodIpRange(final UpdatePodManagementNetworkIpRangeCmd cmd) throws ConcurrentOperationException {
+ final long podId = cmd.getPodId();
+ final String currentStartIP= cmd.getCurrentStartIP();
+ final String currentEndIP= cmd.getCurrentEndIP();
+ final HostPodVO pod = _podDao.findById(podId);
+
+ String newStartIP = cmd.getNewStartIP();
+ String newEndIP = cmd.getNewEndIP();
+ String vlan = null;
+
+ if(newStartIP == null){
+ newStartIP= currentStartIP;
+ }
+
+ if(newEndIP == null){
+ newEndIP= currentEndIP;
+ }
+
+ if(pod == null) {
+ throw new InvalidParameterValueException("Unable to find pod by id " + podId);
+ }
+
+ final String[] existingPodIpRanges = pod.getDescription().split(",");
+ if(existingPodIpRanges.length == 0) {
+ throw new InvalidParameterValueException("The IP range cannot be found since the existing IP range is empty.");
+ }
+
+ verifyIPRangeParameters(currentStartIP,currentEndIP);
+ verifyIPRangeParameters(newStartIP,newEndIP);
+ checkIpRangeContainsTakenAddresses(pod,currentStartIP,currentEndIP,newStartIP,newEndIP);
+
+ boolean foundRange = false;
+
+ for(String podIpRange: existingPodIpRanges) {
+ final String[] existingPodIpRange = podIpRange.split("-");
+
+ if(existingPodIpRange.length > 1) {
+ if (!NetUtils.isValidIp4(existingPodIpRange[0]) || !NetUtils.isValidIp4(existingPodIpRange[1])) {
+ continue;
+ }
+ if (currentStartIP.equals(existingPodIpRange[0]) && currentEndIP.equals(existingPodIpRange[1])) {
+ foundRange = true;
+ vlan = existingPodIpRange[3];
+ }
+ if (!foundRange && NetUtils.ipRangesOverlap(newStartIP, newEndIP, existingPodIpRange[0], existingPodIpRange[1])) {
+ throw new InvalidParameterValueException("The Start IP and EndIP address range overlap with private IP :" + existingPodIpRange[0] + "-" + existingPodIpRange[1]);
+ }
+ }
+ }
Review comment:
This method (`updatePodIpRange`) is quite big, what do you think of extracting a few lines into smaller methods?
For instance, lines `1502 - 1517` might be a good block to be extracted into a validation method, which would allow creating test methods asserting the behavior of this section.
----------------------------------------------------------------
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
[GitHub] [cloudstack] ravening commented on a change in pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#discussion_r521255335
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -1463,6 +1464,175 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
}
}
+ @Override
+ @DB
+ public void updatePodIpRange(final UpdatePodManagementNetworkIpRangeCmd cmd) throws ConcurrentOperationException {
+ final long podId = cmd.getPodId();
+ final String currentStartIP = cmd.getCurrentStartIP();
+ final String currentEndIP = cmd.getCurrentEndIP();
+ final HostPodVO pod = _podDao.findById(podId);
+
+ String newStartIP = cmd.getNewStartIP();
+ String newEndIP = cmd.getNewEndIP();
+
+ if (newStartIP == null) {
+ newStartIP = currentStartIP;
+ }
+
+ if (newEndIP == null) {
+ newEndIP = currentEndIP;
+ }
+
+ if (pod == null) {
+ throw new InvalidParameterValueException("Unable to find pod by id " + podId);
+ }
+
+ final String[] existingPodIpRanges = pod.getDescription().split(",");
+ if (existingPodIpRanges.length == 0) {
+ throw new InvalidParameterValueException("The IP range cannot be found since the existing IP range is empty.");
+ }
+
+ verifyIpRangeParameters(currentStartIP,currentEndIP);
+ verifyIpRangeParameters(newStartIP,newEndIP);
+ checkIpRangeContainsTakenAddresses(pod,currentStartIP,currentEndIP,newStartIP,newEndIP);
+
+ String vlan = verifyPodIpRangeExists(podId,existingPodIpRanges,currentStartIP,currentEndIP,newStartIP,newEndIP);
+
+ List<Long> currentIpRange = listAllIPsWithintheRange(currentStartIP,currentEndIP);
Review comment:
Same case here also. if `newStartIP` and `newEndIP` are same current ones then you are creating two arrays with same contents
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-907042086
see my last comment on #3997 , @sureshanaparti (cc @nvazquez )
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-653955774
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make 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
[GitHub] [cloudstack] blueorangutan removed a comment on pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#issuecomment-713372878
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2240
----------------------------------------------------------------
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
[GitHub] [cloudstack] kioie commented on a change in pull request #4010: New API endpoint to update size of Pod Management IP Range.
Posted by GitBox <gi...@apache.org>.
kioie commented on a change in pull request #4010:
URL: https://github.com/apache/cloudstack/pull/4010#discussion_r453535391
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -1463,6 +1464,162 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
}
}
+ @Override
+ @DB
+ public void updatePodIpRange(final UpdatePodManagementNetworkIpRangeCmd cmd) throws ConcurrentOperationException {
+ final long podId = cmd.getPodId();
+ final String currentStartIP= cmd.getCurrentStartIP();
+ final String currentEndIP= cmd.getCurrentEndIP();
+ final HostPodVO pod = _podDao.findById(podId);
+
+ String newStartIP = cmd.getNewStartIP();
+ String newEndIP = cmd.getNewEndIP();
+ String vlan = null;
+
+ if(newStartIP == null){
+ newStartIP= currentStartIP;
+ }
+
+ if(newEndIP == null){
+ newEndIP= currentEndIP;
+ }
+
+ if(pod == null) {
+ throw new InvalidParameterValueException("Unable to find pod by id " + podId);
+ }
+
+ final String[] existingPodIpRanges = pod.getDescription().split(",");
+ if(existingPodIpRanges.length == 0) {
+ throw new InvalidParameterValueException("The IP range cannot be found since the existing IP range is empty.");
+ }
+
+ verifyIPRangeParameters(currentStartIP,currentEndIP);
+ verifyIPRangeParameters(newStartIP,newEndIP);
+ checkIpRangeContainsTakenAddresses(pod,currentStartIP,currentEndIP,newStartIP,newEndIP);
+
+ boolean foundRange = false;
+
+ for(String podIpRange: existingPodIpRanges) {
+ final String[] existingPodIpRange = podIpRange.split("-");
+
+ if(existingPodIpRange.length > 1) {
+ if (!NetUtils.isValidIp4(existingPodIpRange[0]) || !NetUtils.isValidIp4(existingPodIpRange[1])) {
+ continue;
+ }
+ if (currentStartIP.equals(existingPodIpRange[0]) && currentEndIP.equals(existingPodIpRange[1])) {
+ foundRange = true;
+ vlan = existingPodIpRange[3];
+ }
+ if (!foundRange && NetUtils.ipRangesOverlap(newStartIP, newEndIP, existingPodIpRange[0], existingPodIpRange[1])) {
+ throw new InvalidParameterValueException("The Start IP and EndIP address range overlap with private IP :" + existingPodIpRange[0] + "-" + existingPodIpRange[1]);
+ }
+ }
+ }
Review comment:
Updated this
----------------------------------------------------------------
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