CLOUDSTACK-10199: Support requesting a specific IPv4 address by GabrielBrascher · Pull Request #2595 · apache/cloudstack

GabrielBrascher · 2018-04-23T20:51:23Z

This pull request allows deploying VMs with a specific IPv4 address.

DirectPodBasedNetworkGuru does not support requesting a custom IP-Address while creating a new NIC/Instance, throwing the following error:

Error 530: Does not support custom ip allocation at this time: NicProfile[0-0-null-null-null
Unknown macro: { "cserrorcode"}

Some use-cases prefer the ability to request the IPv4 address which the Instance will get.

This implementation adds unit test cases to cover and it was manually tested in Basic Networking. I can perform more tests if requested.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
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)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

marcaurele · 2018-04-24T06:20:23Z

+        List<VlanVO> vlanVoList = listVlansByNetworkId(networkId);
+        VlanVO vlanVo = null;
+        for (VlanVO vlan : vlanVoList) {
+            if (vlan.getRemoved() == null) {


listVlansByNetworkId won't return removed entry since it's using the normal DAO listBy().
Another question: why picking the first one when the result could return many?

I have the same doubt. It looks like a logic that should be somewhere else. If the network can use multiple Vlan, then should not you look for the correct one (the one that is in the same CIDR) as the requested IPv4?

Well noticed, thanks :)

marcaurele · 2018-04-24T06:21:52Z

+        verifyAndAssert(null, null, null, nicProfile, 1, 0);
+    }
+
+    @Test//(expected = CloudRuntimeException.class)


typo: the comment //... can be removed

marcaurele · 2018-04-24T06:30:16Z

+        if (ipVO == null) {
+            throw new CloudRuntimeException(String.format("Cannot find IPAddressVO for guest [IPv4 address='%s'] and [network id=%s]", requestedIpv4Address, network.getId()));
+        }
+        IPAddressVO lockedIpVO = _ipAddressDao.acquireInLockTable(ipVO.getId());


lockedIpVO can be null (https://github.com/apache/cloudstack/blob/master/framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java#L1049). Moreover, it would be better IMO to put the lock/release operation with a try/finally block to ensure to release the lock all the time (when it could get it).

@marcaurele are you LGTM on this one?
Thanks for your reviews and hard work 👍

marcaurele · 2018-04-24T06:32:19Z

+        }
+        IPAddressVO lockedIpVO = _ipAddressDao.acquireInLockTable(ipVO.getId());
+        if (lockedIpVO.getState() != IPAddressVO.State.Free) {
+            String ipState = lockedIpVO.getState().toString();


You don't need to have a variable holding the state, you can use lockedIpVo.getState() directly in the String.format() function, its value won't change after releasing the lock on the VO.

marcaurele · 2018-04-24T06:33:26Z

+                String macAddress = _networkModel.getNextAvailableMacAddressInNetwork(network.getId());
+                nicProfile.setMacAddress(macAddress);
+            } catch (InsufficientAddressCapacityException e) {
+                e.printStackTrace();


Require a logger here and to throw an exception

marcaurele · 2018-04-24T06:34:03Z

+    protected void configureNicProfileBasedOnRequestedIp(NicProfile requestedNicProfile, NicProfile nicProfile, Network network) {
+        String requestedIpv4Address = requestedNicProfile.getRequestedIPv4();
+        if (requestedIpv4Address == null) {
+            s_logger.debug(String.format("The requested [IPv4 address=%s] is null", requestedIpv4Address));


Could be simplified since requestedIpv4Address is always null to avoid parameter.

rafaelweingartner · 2018-04-23T20:54:19Z

+    protected void configureNicProfileBasedOnRequestedIp(NicProfile requestedNicProfile, NicProfile nicProfile, Network network) {
+        String requestedIpv4Address = requestedNicProfile.getRequestedIPv4();
+        if (requestedIpv4Address == null) {
+            s_logger.debug(String.format("The requested [IPv4 address=%s] is null", requestedIpv4Address));


If requestedIpv4Address is null, then it is null. I do not think that we need to log it then. I mean, the value of requestedIpv4Address

rafaelweingartner · 2018-04-24T10:20:01Z

+     */
+    protected void configureNicProfileBasedOnRequestedIp(NicProfile requestedNicProfile, NicProfile nicProfile, Network network) {
+        String requestedIpv4Address = requestedNicProfile.getRequestedIPv4();
+        if (requestedIpv4Address == null) {


You can extract lines 893-902 to a specific method that validates requestedIpv4Address

@GabrielBrascher, @rafaelweingartner, I think the last if condition covers both null and blank check. (isValidIp4 method)

Thanks, you are right @nitin-maharana; however, the null check should exist to return without an exception and consider that the user did not request a specific IPv4.

rafaelweingartner · 2018-04-24T10:21:15Z

+                    requestedIpv4Address, network.getId()));
+        }
+
+        acquireLockAndCheckIfIpv4IsFree(network, requestedIpv4Address);


Should not you validate the vlanVo before trying to acquire the IP?
I mean, you are executing the validation on vlanVo afterwards.

Thanks for pointing that @rafaelweingartner 👍

rafaelweingartner · 2018-04-24T10:28:57Z

+        List<VlanVO> vlanVoList = listVlansByNetworkId(networkId);
+        VlanVO vlanVo = null;
+        for (VlanVO vlan : vlanVoList) {
+            if (vlan.getRemoved() == null) {


I have the same doubt. It looks like a logic that should be somewhere else. If the network can use multiple Vlan, then should not you look for the correct one (the one that is in the same CIDR) as the requested IPv4?

nitin-maharana · 2018-04-25T09:51:12Z

+     */
+    protected void configureNicProfileBasedOnRequestedIp(NicProfile requestedNicProfile, NicProfile nicProfile, Network network) {
+        String requestedIpv4Address = requestedNicProfile.getRequestedIPv4();
+        if (requestedIpv4Address == null) {


@GabrielBrascher, @rafaelweingartner, I think the last if condition covers both null and blank check. (isValidIp4 method)

yadvr · 2018-05-01T15:13:25Z

In advanced zone, I can see the API allows acceptance of ipaddress or ip6address parameters. @GabrielBrascher how do I reproduce the issue (env details please)?

GabrielBrascher · 2018-05-02T01:57:48Z

@rhtyd I am using basic network and KVM. It might be a problem only on the basic network then.
I tested with cloudmonkey, using the following command:
deploy virtualmachine zoneid=<id> serviceofferingid=<id> templateid=<id> startvm=true ipaddress=<requested ipv4address> name=<name> displayname=<“name”>

rafaelweingartner · 2018-05-03T01:11:48Z

+            throw new InvalidParameterValueException(String.format("The requested [IPv4 address='%s'] is not a valid IP address", requestedIpv4Address));
+        }
+
+        VlanVO vlanVo = _vlanDao.findByNetworkIdAndIpv4Range(network.getId(), requestedIpv4Address);


This name findByNetworkIdAndIpv4Range does not seem to be adequate. It seems that you are going to look for a VlanVo by ipv4 range, which is not the case. You are looking for a VlanVo of a network that "supports" the specified IPv4. Therefore, something like findVlanByNetworkIdAndIpv4 seems better.

Makes sense, I will change it. Thanks!

convention says to leave out the vlan (we are already in the vlandao. so findByNetworkIdAndIpv4Range actually makes sense.

I can update it to findByNetworkIdAndIpv4 then. What do you guys think of it?

It makes sense. 👍

rafaelweingartner · 2018-05-03T01:15:01Z

+    /**
+     *  Acquires lock of in table "user_ip_address" and checks if the requested IPv4 address is Free.
+     */
+    protected void acquireLockAndCheckIfIpv4IsFree(Network network, String requestedIpv4Address) {


These exceptions that you throw here, I think it might be better to throw an InvalidParameterValueException instead. These exception will happen when the user enters an "invalid/already used" IP, right?

Yes, that makes sense. I will update it.

rafaelweingartner · 2018-05-03T01:15:51Z

+        try {
+            testOrchastrator.configureNicProfileBasedOnRequestedIp(requestedNicProfile, nicProfile, network);
+        } catch (InvalidParameterValueException e) {
+            assertEquals(exceptionAssert, e.getMessage());


why not use the @Test with the expected parameter?

I always use @test(expected=Exception); however, it is not working here now, I will check again what might be causing it.

rafaelweingartner · 2018-05-03T01:19:07Z

+            String[] ipRangeParts = ipRange.split("-");
+            String startIP = ipRangeParts[0];
+            String endIP = ipRangeParts[1];
+            if (NetUtils.isIpInRange(ipv4Address, startIP, endIP)) {


What about using something similar to isIp6InNetwork?
I also get a feeling that this method you implemented is doing the same as isIpWithInCidrRange

The problem is that I do not have a CIDR. The network CIDR (at least in the basic network) is null. I like the idea of isIp6InNetwork, but I will need to ignore CIDR at this stage and use only ipv4 range.

fmaximus · 2018-06-06T08:40:41Z

+    /**
+     *  Acquires lock of in table "user_ip_address" and checks if the requested IPv4 address is Free.
+     */
+    protected void acquireLockAndCheckIfIpv4IsFree(Network network, String requestedIpv4Address) {


Will this check work for requested IP addresses in case of Advanced networking?
I believe if the network is of type Guest, then you won't be able to find an IPAddressVO.
So the method should only be called if the network is public or shared.

Thanks, @fmaximus. I will check on that.

@fmaximus cannot users define a guest IP? I mean, as long as the guest IP defined by the user is within the bounds of the guest network, it should be possible, right?

@rafaelweingartner The issue is that when a user selects a guest IP, it won't be found by _ipAddressDao, because IPAddressVO is for public IP's, not for guest IP's.

@fmaximus thanks for pointing it out and sorry for the delay. I did some testing in advanced network and confirmed that a few modifications in this PR will be necessary. When I finish them I will ping you.

GabrielBrascher · 2018-08-23T10:33:35Z


+        DataCenterVO dcVo = _dcDao.findById(network.getDataCenterId());
+        if (dcVo.getNetworkType() == NetworkType.Basic) {
+            configureNicProfileBasedOnRequestedIp(requested, profile, network);


@fmaximus I changed the execution flow; now this workflow will be executed just in case of a basic network.

After this change I could add IPv4 either in an advanced network, with a 'guest IP', and in a basic network with a 'public IP' (from the IPAddressVO in this case). Thanks again for pointing that issue.

yadvr · 2018-09-18T11:45:03Z

@blueorangutan test

blueorangutan · 2018-09-18T11:45:54Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

GabrielBrascher · 2018-09-20T02:35:31Z

@rafaelweingartner I have checked and could not find any point indicating that the errors presented here are caused by my changes.

marcaurele

LGTM

DaanHoogland · 2018-11-12T13:00:14Z

@blueorangutan package

blueorangutan · 2018-11-12T13:01:19Z

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-11-12T13:31:40Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2430

GabrielBrascher · 2018-11-12T18:14:30Z

@blueorangutan package

blueorangutan · 2018-11-12T18:15:20Z

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

GabrielBrascher

@DaanHoogland @marcaurele @nitin-maharana @rafaelweingartner @fmaximus please note that I introduced a minor change. The updated code follows detailed in the comments.

There is no logic changes, just extracted and enhanced the code, allowing JUnit tests and proper documentation.

GabrielBrascher · 2018-11-12T18:19:22Z

+            lockedIpVO.setState(IPAddressVO.State.Allocated);
+            _ipAddressDao.update(lockedIpVO.getId(), lockedIpVO);
+        } finally {
+            _ipAddressDao.releaseFromLockTable(ipVO.getId());


The finally block will release the lock in case of success or an exception.

GabrielBrascher · 2018-11-12T18:19:29Z

+    /**
+     * Validates the locked IP, throwing an exeption if the locked IP is null or the locked IP is not in 'Free' state.
+     */
+    protected void validateLockedRequestedIp(IPAddressVO ipVO, IPAddressVO lockedIpVO) {


Created validateLockedRequestedIp to extract some verifications, documenting it and creating test cases. The logic stills the same. as the caller (method acquireLockAndCheckIfIpv4IsFree) does have a finally block that releases the lock, it is ok to throw the exceptions in this validation.

blueorangutan · 2018-11-12T18:45:50Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-2433

GabrielBrascher · 2018-11-12T20:54:47Z

@blueorangutan package

blueorangutan · 2018-11-12T20:55:20Z

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-11-12T21:28:05Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-2434

blueorangutan · 2018-11-12T21:58:38Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2435

This commit allows deploying VMs with a specific IPv4 address. DirectPodBasedNetworkGuru does not support requesting a custom IP-Address while creating a new NIC/Instance, throwing the following error: Error 530: Does not support custom ip allocation at this time: NicProfile[0-0-null-null-null Unknown macro: { "cserrorcode"} Some use-cases prefer the ability to request the IPv4 address which the Instance will get. This implementation adds unit test cases to cover and it was manually tested in Basic Networking. I can perform more tests if requested.

GabrielBrascher · 2018-11-20T13:10:51Z

@DaanHoogland @nvazquez @rhtyd @borisstoyanov All ok after packaging 🥇
Can you please run Integration test on this one? Thanks in advance 👍

nvazquez · 2018-11-21T13:13:30Z

Sure @GabrielBrascher
@blueorangutan test matrix

blueorangutan · 2018-11-21T13:13:58Z

@nvazquez a Trillian-Jenkins matrix job (centos6 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

blueorangutan · 2018-11-21T20:12:48Z

Trillian test result (tid-3197)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 23626 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2595-t3197-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 67 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	31.76	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	499.95	test_vpc_redundant.py

blueorangutan · 2018-11-21T21:03:57Z

Trillian test result (tid-3196)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 26835 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2595-t3196-xenserver-71.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_scale_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 66 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	56.55	test_multipleips_per_nic.py
test_01_scale_vm	`Error`	13.28	test_scale_vm.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	507.14	test_vpc_redundant.py

blueorangutan · 2018-11-21T22:55:02Z

Trillian test result (tid-3198)
Environment: vmware-65 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33596 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2595-t3198-vmware-65.zip
Intermittent failure detected: /marvin/tests/smoke/test_deploy_vgpu_enabled_vm.py
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 67 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	79.01	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	574.32	test_vpc_redundant.py

borisstoyanov

LGTM, based on marvin tests results

GabrielBrascher changed the title ~~Support requesting a specific IPv4 address~~ CLOUDSTACK-10199: Support requesting a specific IPv4 address Apr 23, 2018

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch from bdf1163 to 03acbd2 Compare April 23, 2018 21:01

marcaurele reviewed Apr 24, 2018

View reviewed changes

rafaelweingartner reviewed Apr 24, 2018

View reviewed changes

nitin-maharana suggested changes Apr 25, 2018

View reviewed changes

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch 2 times, most recently from 2ebdb34 to 32bd12c Compare May 2, 2018 20:47

rafaelweingartner reviewed May 3, 2018

View reviewed changes

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch from 32bd12c to 4ac2b74 Compare May 3, 2018 15:08

GabrielBrascher added this to the 4.12.0.0 milestone May 4, 2018

GabrielBrascher added the type:bug label May 4, 2018

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch 5 times, most recently from 82e6a68 to f8ebd6b Compare May 10, 2018 17:12

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch 2 times, most recently from a63d506 to 5344e18 Compare May 16, 2018 20:46

fmaximus requested changes Jun 6, 2018

View reviewed changes

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch 3 times, most recently from 981ed87 to e138034 Compare August 23, 2018 10:12

GabrielBrascher commented Aug 23, 2018

View reviewed changes

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch from e138034 to 723c054 Compare November 12, 2018 11:39

marcaurele approved these changes Nov 12, 2018

View reviewed changes

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch from 723c054 to a38d7db Compare November 12, 2018 13:38

GabrielBrascher self-assigned this Nov 12, 2018

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch from a38d7db to 6825df7 Compare November 12, 2018 18:12

GabrielBrascher commented Nov 12, 2018

View reviewed changes

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch from 6825df7 to b52eab6 Compare November 12, 2018 20:53

GabrielBrascher force-pushed the CLOUDSTACK-10199 branch from b52eab6 to 04f73b9 Compare November 12, 2018 22:24

borisstoyanov approved these changes Nov 26, 2018

View reviewed changes

GabrielBrascher merged commit 2334145 into apache:master Nov 27, 2018

Uh oh!

Conversation

GabrielBrascher commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented May 1, 2018

Uh oh!

GabrielBrascher commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Apr 23, 2018 •

edited

Loading

GabrielBrascher commented May 2, 2018 •

edited

Loading

GabrielBrascher Aug 23, 2018 •

edited

Loading

GabrielBrascher left a comment •

edited

Loading