Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -231,27 +231,27 @@ public class NetworkOrchestrator extends ManagerBase implements NetworkOrchestra
@Inject
EntityManager _entityMgr;
@Inject
DataCenterDao _dcDao = null;
DataCenterDao _dcDao;
@Inject
VlanDao _vlanDao = null;
VlanDao _vlanDao;
@Inject
IPAddressDao _ipAddressDao = null;
IPAddressDao _ipAddressDao;
@Inject
AccountDao _accountDao = null;
AccountDao _accountDao;
@Inject
ConfigurationDao _configDao;
@Inject
UserVmDao _userVmDao = null;
UserVmDao _userVmDao;
@Inject
AlertManager _alertMgr;
@Inject
ConfigurationManager _configMgr;
@Inject
NetworkOfferingDao _networkOfferingDao = null;
NetworkOfferingDao _networkOfferingDao;
@Inject
NetworkDao _networksDao = null;
NetworkDao _networksDao;
@Inject
NicDao _nicDao = null;
NicDao _nicDao;
@Inject
RulesManager _rulesMgr;
@Inject
Expand Down Expand Up @@ -860,6 +860,11 @@ public Pair<NicProfile, Integer> allocateNic(final NicProfile requested, final N

NicVO vo = new NicVO(guru.getName(), vm.getId(), network.getId(), vm.getType());

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

@GabrielBrascher GabrielBrascher Aug 23, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}

deviceId = applyProfileToNic(vo, profile, deviceId);

vo = _nicDao.persist(vo);
Expand All @@ -871,6 +876,85 @@ public Pair<NicProfile, Integer> allocateNic(final NicProfile requested, final N
return new Pair<NicProfile, Integer>(vmNic, Integer.valueOf(deviceId));
}

/**
* If the requested IPv4 address from the NicProfile was configured then it configures the IPv4 address, Netmask and Gateway to deploy the VM with the requested IP.
*/
protected void configureNicProfileBasedOnRequestedIp(NicProfile requestedNicProfile, NicProfile nicProfile, Network network) {
if (requestedNicProfile == null) {
return;
}
String requestedIpv4Address = requestedNicProfile.getRequestedIPv4();
if (requestedIpv4Address == null) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}
if (!NetUtils.isValidIp4(requestedIpv4Address)) {
throw new InvalidParameterValueException(String.format("The requested [IPv4 address='%s'] is not a valid IP address", requestedIpv4Address));
}

VlanVO vlanVo = _vlanDao.findByNetworkIdAndIpv4(network.getId(), requestedIpv4Address);
if (vlanVo == null) {
throw new InvalidParameterValueException(String.format("Trying to configure a Nic with the requested [IPv4='%s'] but cannot find a Vlan for the [network id='%s']",
requestedIpv4Address, network.getId()));
}

String ipv4Gateway = vlanVo.getVlanGateway();
String ipv4Netmask = vlanVo.getVlanNetmask();

if (!NetUtils.isValidIp4(ipv4Gateway)) {
throw new InvalidParameterValueException(String.format("The [IPv4Gateway='%s'] from [VlanId='%s'] is not valid", ipv4Gateway, vlanVo.getId()));
}
if (!NetUtils.isValidIp4Netmask(ipv4Netmask)) {
throw new InvalidParameterValueException(String.format("The [IPv4Netmask='%s'] from [VlanId='%s'] is not valid", ipv4Netmask, vlanVo.getId()));
}

acquireLockAndCheckIfIpv4IsFree(network, requestedIpv4Address);

nicProfile.setIPv4Address(requestedIpv4Address);
nicProfile.setIPv4Gateway(ipv4Gateway);
nicProfile.setIPv4Netmask(ipv4Netmask);

if (nicProfile.getMacAddress() == null) {
try {
String macAddress = _networkModel.getNextAvailableMacAddressInNetwork(network.getId());
nicProfile.setMacAddress(macAddress);
} catch (InsufficientAddressCapacityException e) {
throw new CloudRuntimeException(String.format("Cannot get next available mac address in [network id='%s']", network.getId()), e);
}
}
}

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense. I will update it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @fmaximus. I will check on that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

IPAddressVO ipVO = _ipAddressDao.findByIpAndSourceNetworkId(network.getId(), requestedIpv4Address);
if (ipVO == null) {
throw new InvalidParameterValueException(
String.format("Cannot find IPAddressVO for guest [IPv4 address='%s'] and [network id='%s']", requestedIpv4Address, network.getId()));
}
try {
IPAddressVO lockedIpVO = _ipAddressDao.acquireInLockTable(ipVO.getId());
validateLockedRequestedIp(ipVO, lockedIpVO);
lockedIpVO.setState(IPAddressVO.State.Allocated);
_ipAddressDao.update(lockedIpVO.getId(), lockedIpVO);
} finally {
_ipAddressDao.releaseFromLockTable(ipVO.getId());
Comment thread
GabrielBrascher marked this conversation as resolved.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

/**
* 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) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (lockedIpVO == null) {
throw new InvalidParameterValueException(String.format("Cannot acquire guest [IPv4 address='%s'] as it was removed while acquiring lock", ipVO.getAddress()));
}
if (lockedIpVO.getState() != IPAddressVO.State.Free) {
throw new InvalidParameterValueException(
String.format("Cannot acquire guest [IPv4 address='%s']; The Ip address is in [state='%s']", ipVO.getAddress(), lockedIpVO.getState().toString()));
}
}

protected Integer applyProfileToNic(final NicVO vo, final NicProfile profile, Integer deviceId) {
if (profile.getDeviceId() != null) {
vo.setDeviceId(profile.getDeviceId());
Expand Down
Loading