[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1578


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[fmaximus]

Github user fmaximus commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77851893
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -631,195 +753,257 @@ def check_Router_state(self, router, state=None):
 self.assertEqual(routers[0].state, state,
  "Virtual router is not in the expected state"
  )
-self.debug("Successfully validated the deployment and state of 
Router - %s" % router.name)
+self.debug("Successfully validated the deployment and state of 
Router "
+   "- %s" % router.name)
 
-# validate_PublicIPAddress - Validates if the given public IP address 
is in the expected state form the list of
-# fetched public IP addresses
-def validate_PublicIPAddress(self, public_ip, network, 
static_nat=False, vm=None):
+# validate_PublicIPAddress - Validates if the given public IP address 
is in
+# the expected state form the list of fetched public IP addresses
+def validate_PublicIPAddress(self, public_ip, network, 
static_nat=False,
+ vm=None):
 """Validates the Public IP Address"""
-self.debug("Validating the assignment and state of public IP 
address - %s" % public_ip.ipaddress.ipaddress)
+self.debug("Validating the assignment and state of public IP 
address "
+   "- %s" % public_ip.ipaddress.ipaddress)
 public_ips = PublicIPAddress.list(self.api_client,
   id=public_ip.ipaddress.id,
   networkid=network.id,
   isstaticnat=static_nat,
   listall=True
   )
 self.assertEqual(isinstance(public_ips, list), True,
- "List public IP for network should return a valid 
list"
+ "List public IP for network should return a "
+ "valid list"
  )
-self.assertEqual(public_ips[0].ipaddress, 
public_ip.ipaddress.ipaddress,
- "List public IP for network should list the 
assigned public IP address"
+self.assertEqual(public_ips[0].ipaddress,
+ public_ip.ipaddress.ipaddress,
+ "List public IP for network should list the 
assigned "
+ "public IP address"
  )
 self.assertEqual(public_ips[0].state, "Allocated",
  "Assigned public IP is not in the allocated state"
  )
 if static_nat and vm:
 self.assertEqual(public_ips[0].virtualmachineid, vm.id,
- "Static NAT rule is not enabled for the VM on 
the assigned public IP"
+ "Static NAT rule is not enabled for the VM on 
"
+ "the assigned public IP"
  )
-self.debug("Successfully validated the assignment and state of 
public IP address - %s" %
-   public_ip.ipaddress.ipaddress)
+self.debug("Successfully validated the assignment and state of 
public "
+   "IP address - %s" % public_ip.ipaddress.ipaddress)
 
-# VSD verifications
-# VSD is a programmable policy and analytics engine of Nuage VSP SDN 
platform
+# VSD verifications; VSD is a programmable policy and analytics engine 
of
+# Nuage VSP SDN platform
 
-# get_externalID_filter - Returns corresponding external ID filter of 
the given object in VSD
+# get_externalID_filter - Returns corresponding external ID filter of 
the
+# given object in VSD
 def get_externalID_filter(self, object_id):
 ext_id = object_id + "@" + self.cms_id
 return self.vsd.set_externalID_filter(ext_id)
 
 # fetch_by_externalID - Returns VSD object with the given external ID
 def fetch_by_externalID(self, fetcher, *cs_objects):
-""" Fetches a child object by external id using the given fetcher, 
and uuids of the given cloudstack objects.
+""" Fetches a child object by external id using the given fetcher, 
and
+uuids of the given cloudstack objects.
 E.G.
-  - 
fetch_by_external_id(vsdk.NUSubnet(id="954de425-b860-410b-be09-c560e7dbb474").vms,
 cs_vm)
-  - fetch_by_external_id(session.user.floating_ips, cs_network, 
cs_public_ip)
+  - fetch_by_external_id(vsdk.NUSubnet
+  (id="954de425-b860-410b-be09-c560e7dbb474").vms, 

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[fmaximus]

Github user fmaximus commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77831210
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -175,45 +234,44 @@ def tearDown(self):
 return
 
 # create_VpcOffering - Creates VPC offering
-def create_VpcOffering(self, vpc_offering, suffix=None):
-self.debug("Creating VPC offering")
+@needscleanup
+def create_VpcOffering(cls, vpc_offering, suffix=None):
+cls.debug("Creating VPC offering")
 if suffix:
 vpc_offering["name"] = "VPC_OFF-" + str(suffix)
-vpc_off = VpcOffering.create(self.api_client,
+vpc_off = VpcOffering.create(cls.api_client,
  vpc_offering
  )
 # Enable VPC offering
-vpc_off.update(self.api_client, state="Enabled")
-self.cleanup.append(vpc_off)
-self.debug("Created and Enabled VPC offering")
+vpc_off.update(cls.api_client, state="Enabled")
+cls.debug("Created and Enabled VPC offering")
 return vpc_off
 
 # create_Vpc - Creates VPC with the given VPC offering
-def create_Vpc(self, vpc_offering, cidr='10.1.0.0/16', testdata=None, 
account=None, networkDomain=None,
-   cleanup=True):
+@needscleanup
+def create_Vpc(cls, vpc_offering, cidr='10.1.0.0/16', testdata=None,
+   account=None, networkDomain=None):
 if not account:
-account = self.account
-self.debug("Creating a VPC in the account - %s" % account.name)
+account = cls.account
+cls.debug("Creating a VPC in the account - %s" % account.name)
 if not testdata:
-testdata = self.test_data["vpc"]
+testdata = cls.test_data["vpc"]
 testdata["name"] = "TestVPC-" + cidr + "-" + 
str(vpc_offering.name)
 testdata["displaytext"] = "Test VPC"
 testdata["cidr"] = cidr
-vpc = VPC.create(self.api_client,
+vpc = VPC.create(cls.api_client,
  testdata,
  vpcofferingid=vpc_offering.id,
- zoneid=self.zone.id,
+ zoneid=cls.zone.id,
  account=account.name,
  domainid=account.domainid,
  networkDomain=networkDomain
  )
-self.debug("Created VPC with ID - %s" % vpc.id)
-if cleanup:
-self.cleanup.append(vpc)
+cls.debug("Created VPC with ID - %s" % vpc.id)
 return vpc
 
 # restart_Vpc - Restarts the given VPC with/without cleanup
-def restart_Vpc(self, vpc, cleanup=None):
+def restart_Vpc(self, vpc, cleanup=False):
--- End diff --

It's just a parameter passed into the restartVPC command.
I'm more concerned about the fact that we need to set makeredundant to 
false, 
as the default seems to be true.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[fmaximus]

Github user fmaximus commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77818579
  
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -988,6 +1016,21 @@ public boolean addDhcpEntry(final Network network, 
final NicProfile nic, final V
 return result;
 }
 
+protected boolean deleteDhcpSupportForSubnet(Network network, 
Network.Service service) throws ResourceUnavailableException {
+if (canHandle(network, service)) {
+final List routers = 
_routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
+if (routers == null || routers.size() == 0) {
+throw new ResourceUnavailableException("Can't find at 
least one router!", DataCenter.class, network.getDataCenterId());
+}
+try {
+return _routerMgr.removeDhcpSupportForSubnet(network, 
routers);
+} catch (final ResourceUnavailableException e) {
+s_logger.debug("Router resource unavailable ");
--- End diff --

Here we moved existing code from removeDhcpSupportForSubnet to a separate 
method, containing the service. 
Another question is, why catch ResourceUnavailableException, if it is also 
present in the method's throws clause? It then would be logged by the invoking 
methods.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[fmaximus]

Github user fmaximus commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77800584
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -77,85 +117,104 @@ def setUpClass(cls, zone=None):
 cls.test_data["virtual_machine"]["zoneid"] = cls.zone.id
 cls.test_data["virtual_machine"]["template"] = cls.template.id
 
-# Create service offering
-cls.service_offering = ServiceOffering.create(cls.api_client,
-  
cls.test_data["service_offering"]
-  )
-cls._cleanup = [cls.service_offering]
-
 # Check if the host hypervisor type is simulator
-cls.isSimulator = Hypervisor.list(cls.api_client, 
zoneid=cls.zone.id)[0].name == "Simulator"
+cls.isSimulator = Hypervisor.list(
+cls.api_client,
+zoneid=cls.zone.id)[0].name == "Simulator"
 
 # Get configured Nuage VSP device details
 try:
-physical_networks = PhysicalNetwork.list(cls.api_client, 
zoneid=cls.zone.id)
+physical_networks = PhysicalNetwork.list(
+cls.api_client,
+zoneid=cls.zone.id
+)
 for pn in physical_networks:
 if pn.isolationmethods == "VSP":
 cls.vsp_physical_network = pn
 break
-cls.nuage_vsp_device = Nuage.list(cls.api_client,
-  
physicalnetworkid=cls.vsp_physical_network.id
-  )[0]
+cls.nuage_vsp_device = Nuage.list(
+cls.api_client,
+physicalnetworkid=cls.vsp_physical_network.id)[0]
 pns = cls.config.zones[0].physical_networks
-providers = filter(lambda physical_network: "VSP" in 
physical_network.isolationmethods, pns)[0].providers
-devices = filter(lambda provider: provider.name == "NuageVsp", 
providers)[0].devices
+providers = filter(lambda physical_network:
+   "VSP" in physical_network.isolationmethods,
+   pns)[0].providers
+devices = filter(lambda provider:
+ provider.name == "NuageVsp",
+ providers)[0].devices
 cls.nuage_vsp_device.username = devices[0].username
 cls.nuage_vsp_device.password = devices[0].password
 cls.cms_id = cls.nuage_vsp_device.cmsid
 except Exception as e:
 cls.tearDownClass()
-raise unittest.SkipTest("Warning: Could not get configured 
Nuage VSP device details - %s" % e)
+raise unittest.SkipTest("Warning: Could not get configured "
+"Nuage VSP device details - %s" % e)
--- End diff --

If the datacenter, against which the tests are run, is not configured with 
Nuage VSP
we want to skip the Nuage VSP related tests instead of failing.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77756715
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -631,195 +753,257 @@ def check_Router_state(self, router, state=None):
 self.assertEqual(routers[0].state, state,
  "Virtual router is not in the expected state"
  )
-self.debug("Successfully validated the deployment and state of 
Router - %s" % router.name)
+self.debug("Successfully validated the deployment and state of 
Router "
+   "- %s" % router.name)
 
-# validate_PublicIPAddress - Validates if the given public IP address 
is in the expected state form the list of
-# fetched public IP addresses
-def validate_PublicIPAddress(self, public_ip, network, 
static_nat=False, vm=None):
+# validate_PublicIPAddress - Validates if the given public IP address 
is in
+# the expected state form the list of fetched public IP addresses
+def validate_PublicIPAddress(self, public_ip, network, 
static_nat=False,
+ vm=None):
 """Validates the Public IP Address"""
-self.debug("Validating the assignment and state of public IP 
address - %s" % public_ip.ipaddress.ipaddress)
+self.debug("Validating the assignment and state of public IP 
address "
+   "- %s" % public_ip.ipaddress.ipaddress)
 public_ips = PublicIPAddress.list(self.api_client,
   id=public_ip.ipaddress.id,
   networkid=network.id,
   isstaticnat=static_nat,
   listall=True
   )
 self.assertEqual(isinstance(public_ips, list), True,
- "List public IP for network should return a valid 
list"
+ "List public IP for network should return a "
+ "valid list"
  )
-self.assertEqual(public_ips[0].ipaddress, 
public_ip.ipaddress.ipaddress,
- "List public IP for network should list the 
assigned public IP address"
+self.assertEqual(public_ips[0].ipaddress,
+ public_ip.ipaddress.ipaddress,
+ "List public IP for network should list the 
assigned "
+ "public IP address"
  )
 self.assertEqual(public_ips[0].state, "Allocated",
  "Assigned public IP is not in the allocated state"
  )
 if static_nat and vm:
 self.assertEqual(public_ips[0].virtualmachineid, vm.id,
- "Static NAT rule is not enabled for the VM on 
the assigned public IP"
+ "Static NAT rule is not enabled for the VM on 
"
+ "the assigned public IP"
  )
-self.debug("Successfully validated the assignment and state of 
public IP address - %s" %
-   public_ip.ipaddress.ipaddress)
+self.debug("Successfully validated the assignment and state of 
public "
+   "IP address - %s" % public_ip.ipaddress.ipaddress)
 
-# VSD verifications
-# VSD is a programmable policy and analytics engine of Nuage VSP SDN 
platform
+# VSD verifications; VSD is a programmable policy and analytics engine 
of
+# Nuage VSP SDN platform
 
-# get_externalID_filter - Returns corresponding external ID filter of 
the given object in VSD
+# get_externalID_filter - Returns corresponding external ID filter of 
the
+# given object in VSD
 def get_externalID_filter(self, object_id):
 ext_id = object_id + "@" + self.cms_id
 return self.vsd.set_externalID_filter(ext_id)
 
 # fetch_by_externalID - Returns VSD object with the given external ID
 def fetch_by_externalID(self, fetcher, *cs_objects):
-""" Fetches a child object by external id using the given fetcher, 
and uuids of the given cloudstack objects.
+""" Fetches a child object by external id using the given fetcher, 
and
+uuids of the given cloudstack objects.
 E.G.
-  - 
fetch_by_external_id(vsdk.NUSubnet(id="954de425-b860-410b-be09-c560e7dbb474").vms,
 cs_vm)
-  - fetch_by_external_id(session.user.floating_ips, cs_network, 
cs_public_ip)
+  - fetch_by_external_id(vsdk.NUSubnet
+  (id="954de425-b860-410b-be09-c560e7dbb474").vms, 

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77756445
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -490,57 +581,75 @@ def execute_cmd(self, ssh_client, cmd):
 self.debug("SSH client executed command result is None")
 return ret_data
 
-# wget_from_server - Fetches index.html file from a web server 
listening on the given public IP address and port
-def wget_from_server(self, public_ip, port):
+# wget_from_server - Fetches file with the given file name from a web
+# server listening on the given public IP address and port
+def wget_from_server(self, public_ip, port, file_name="index.html"):
 import urllib
-self.debug("wget index.html file from a http web server listening 
on public IP address - %s and port - %s" %
-   (public_ip.ipaddress.ipaddress, port))
-filename, headers = urllib.urlretrieve("http://%s:%s/index.html; % 
(public_ip.ipaddress.ipaddress, port),
-   filename="index.html"
-   )
+self.debug("wget file - %s from a http web server listening on "
+   "public IP address - %s and port - %s" %
+   (file_name, public_ip.ipaddress.ipaddress, port))
+filename, headers = urllib.urlretrieve(
+"http://%s:%s/%s; %
+(public_ip.ipaddress.ipaddress, port, file_name),
+filename=file_name
+)
--- End diff --

Please add assertions that ``filename`` and ``header`` are as expected 
(e.g. not ``None``).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77756386
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -461,22 +547,27 @@ def create_NetworkAclRule(self, rule, 
traffic_type="Ingress", network=None, acl_
  )
 
 # ssh_into_VM - Gets into the shell of the given VM using its public IP
-def ssh_into_VM(self, vm, public_ip, reconnect=True):
-self.debug("SSH into VM with ID - %s on public IP address - %s" % 
(vm.id, public_ip.ipaddress.ipaddress))
-tries = 0
-while tries < 3:
+def ssh_into_VM(self, vm, public_ip, reconnect=True, 
negative_test=False):
+self.debug("SSH into VM with ID - %s on public IP address - %s" %
+   (vm.id, public_ip.ipaddress.ipaddress))
+tries = 1 if negative_test else 3
+while True:
 try:
-ssh_client = 
vm.get_ssh_client(ipaddress=public_ip.ipaddress.ipaddress, reconnect=reconnect)
+ssh_client = vm.get_ssh_client(
+ipaddress=public_ip.ipaddress.ipaddress,
+reconnect=reconnect,
+retries=3 if negative_test else 30
+)
+self.debug("Successful to SSH into VM with ID - %s on "
+   "public IP address - %s" %
+   (vm.id, public_ip.ipaddress.ipaddress))
+return ssh_client
 except Exception as e:
 self.debug("Failed to SSH into VM: %s" % e)
-self.debug("Waiting for the VM to be fully resolved for 
SSH connection...")
-time.sleep(120)
+tries -= 1
+if tries == 0:
+raise e
--- End diff --

Please consider separating the retry logic into a utility function to make 
the code more readable and promote reuse.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77756322
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -360,60 +427,75 @@ def delete_VM(self, vm, expunge=True):
 
 # get_Router - Returns router for the given network
 def get_Router(self, network):
-self.debug("Finding the virtual router for network with ID - %s" % 
network.id)
+self.debug("Finding the virtual router for network with ID - %s" %
+   network.id)
 routers = Router.list(self.api_client,
   networkid=network.id,
   listall=True
   )
 self.assertEqual(isinstance(routers, list), True,
- "List routers should return a valid virtual 
router for network"
+ "List routers should return a valid virtual 
router "
+ "for network"
  )
 return routers[0]
 
-# acquire_PublicIPAddress - Acquires public IP address for the given 
network/VPC
+# acquire_PublicIPAddress - Acquires public IP address for the given
+# network/VPC
 def acquire_PublicIPAddress(self, network, vpc=None, account=None):
 if not account:
 account = self.account
-self.debug("Associating public IP for network with ID - %s in the 
account - %s" % (network.id, account.name))
+self.debug("Associating public IP for network with ID - %s in the "
+   "account - %s" % (network.id, account.name))
 public_ip = PublicIPAddress.create(self.api_client,
accountid=account.name,
domainid=account.domainid,
zoneid=self.zone.id,
-   networkid=network.id if vpc is 
None else None,
-   vpcid=vpc.id if vpc else 
self.vpc.id if hasattr(self, "vpc") else None
+   networkid=network.id
+   if vpc is None else None,
+   vpcid=vpc.id if vpc else 
self.vpc.id
+   if hasattr(self, "vpc") else 
None
)
-self.debug("Associated public IP address - %s with network with ID 
- %s" %
-   (public_ip.ipaddress.ipaddress, network.id))
+self.debug("Associated public IP address - %s with network with ID 
- "
+   "%s" % (public_ip.ipaddress.ipaddress, network.id))
 return public_ip
 
-# create_StaticNatRule_For_VM - Creates Static NAT rule on the given 
public IP for the given VM in the given network
-def create_StaticNatRule_For_VM(self, vm, public_ip, network, 
vmguestip=None):
-self.debug("Enabling Static NAT rule on public IP - %s for VM with 
ID - %s in network with ID - %s" %
+# create_StaticNatRule_For_VM - Creates Static NAT rule on the given
+# public IP for the given VM in the given network
+def create_StaticNatRule_For_VM(self, vm, public_ip, network,
+vmguestip=None):
+self.debug("Enabling Static NAT rule on public IP - %s for VM with 
ID "
+   "- %s in network with ID - %s" %
(public_ip.ipaddress.ipaddress, vm.id, network.id))
-static_nat_rule = StaticNATRule.enable(self.api_client,
-   
ipaddressid=public_ip.ipaddress.id,
-   virtualmachineid=vm.id,
-   networkid=network.id,
-   vmguestip=vmguestip
-   )
-self.debug("Static NAT rule enabled on public IP - %s for VM with 
ID - %s in network with ID - %s" %
+static_nat_rule = StaticNATRule.enable(
+self.api_client,
+ipaddressid=public_ip.ipaddress.id,
+virtualmachineid=vm.id,
+networkid=network.id,
+vmguestip=vmguestip
+)
--- End diff --

Please add assertions to validate that ``static_nat_rule`` was enabled as 
expected.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at 

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77756212
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -175,45 +234,44 @@ def tearDown(self):
 return
 
 # create_VpcOffering - Creates VPC offering
-def create_VpcOffering(self, vpc_offering, suffix=None):
-self.debug("Creating VPC offering")
+@needscleanup
+def create_VpcOffering(cls, vpc_offering, suffix=None):
+cls.debug("Creating VPC offering")
 if suffix:
 vpc_offering["name"] = "VPC_OFF-" + str(suffix)
-vpc_off = VpcOffering.create(self.api_client,
+vpc_off = VpcOffering.create(cls.api_client,
  vpc_offering
  )
 # Enable VPC offering
-vpc_off.update(self.api_client, state="Enabled")
-self.cleanup.append(vpc_off)
-self.debug("Created and Enabled VPC offering")
+vpc_off.update(cls.api_client, state="Enabled")
+cls.debug("Created and Enabled VPC offering")
 return vpc_off
 
 # create_Vpc - Creates VPC with the given VPC offering
-def create_Vpc(self, vpc_offering, cidr='10.1.0.0/16', testdata=None, 
account=None, networkDomain=None,
-   cleanup=True):
+@needscleanup
+def create_Vpc(cls, vpc_offering, cidr='10.1.0.0/16', testdata=None,
+   account=None, networkDomain=None):
 if not account:
-account = self.account
-self.debug("Creating a VPC in the account - %s" % account.name)
+account = cls.account
+cls.debug("Creating a VPC in the account - %s" % account.name)
 if not testdata:
-testdata = self.test_data["vpc"]
+testdata = cls.test_data["vpc"]
 testdata["name"] = "TestVPC-" + cidr + "-" + 
str(vpc_offering.name)
 testdata["displaytext"] = "Test VPC"
 testdata["cidr"] = cidr
-vpc = VPC.create(self.api_client,
+vpc = VPC.create(cls.api_client,
  testdata,
  vpcofferingid=vpc_offering.id,
- zoneid=self.zone.id,
+ zoneid=cls.zone.id,
  account=account.name,
  domainid=account.domainid,
  networkDomain=networkDomain
  )
-self.debug("Created VPC with ID - %s" % vpc.id)
-if cleanup:
-self.cleanup.append(vpc)
+cls.debug("Created VPC with ID - %s" % vpc.id)
 return vpc
 
 # restart_Vpc - Restarts the given VPC with/without cleanup
-def restart_Vpc(self, vpc, cleanup=None):
+def restart_Vpc(self, vpc, cleanup=False):
--- End diff --

Flag arguments are an anti-pattern.  Please consider refactoring to another 
method such as ``restartVpcWithCleanup``.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77756102
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -77,85 +117,104 @@ def setUpClass(cls, zone=None):
 cls.test_data["virtual_machine"]["zoneid"] = cls.zone.id
 cls.test_data["virtual_machine"]["template"] = cls.template.id
 
-# Create service offering
-cls.service_offering = ServiceOffering.create(cls.api_client,
-  
cls.test_data["service_offering"]
-  )
-cls._cleanup = [cls.service_offering]
-
 # Check if the host hypervisor type is simulator
-cls.isSimulator = Hypervisor.list(cls.api_client, 
zoneid=cls.zone.id)[0].name == "Simulator"
+cls.isSimulator = Hypervisor.list(
+cls.api_client,
+zoneid=cls.zone.id)[0].name == "Simulator"
 
 # Get configured Nuage VSP device details
 try:
-physical_networks = PhysicalNetwork.list(cls.api_client, 
zoneid=cls.zone.id)
+physical_networks = PhysicalNetwork.list(
+cls.api_client,
+zoneid=cls.zone.id
+)
 for pn in physical_networks:
 if pn.isolationmethods == "VSP":
 cls.vsp_physical_network = pn
 break
-cls.nuage_vsp_device = Nuage.list(cls.api_client,
-  
physicalnetworkid=cls.vsp_physical_network.id
-  )[0]
+cls.nuage_vsp_device = Nuage.list(
+cls.api_client,
+physicalnetworkid=cls.vsp_physical_network.id)[0]
 pns = cls.config.zones[0].physical_networks
-providers = filter(lambda physical_network: "VSP" in 
physical_network.isolationmethods, pns)[0].providers
-devices = filter(lambda provider: provider.name == "NuageVsp", 
providers)[0].devices
+providers = filter(lambda physical_network:
+   "VSP" in physical_network.isolationmethods,
+   pns)[0].providers
+devices = filter(lambda provider:
+ provider.name == "NuageVsp",
+ providers)[0].devices
 cls.nuage_vsp_device.username = devices[0].username
 cls.nuage_vsp_device.password = devices[0].password
 cls.cms_id = cls.nuage_vsp_device.cmsid
 except Exception as e:
 cls.tearDownClass()
-raise unittest.SkipTest("Warning: Could not get configured 
Nuage VSP device details - %s" % e)
+raise unittest.SkipTest("Warning: Could not get configured "
+"Nuage VSP device details - %s" % e)
+return
 
-# VSD is a programmable policy and analytics engine of Nuage VSP 
SDN platform
-# vspk is a Python SDK for Nuage VSP's VSD
-# libVSD is a library that wraps vspk package
+@classmethod
+def configureVSDSessions(cls):
+# VSD is a programmable policy and analytics engine of Nuage VSP 
SDN
+# platform; vspk is a Python SDK for Nuage VSP's VSD; libVSD is a
+# library that wraps vspk package
 try:
-vspk_module = "vspk." + cls.nuage_vsp_device.apiversion if 
int(cls.nuage_vsp_device.apiversion[1]) >= 4 \
+vspk_module = "vspk." + cls.nuage_vsp_device.apiversion \
+if int(cls.nuage_vsp_device.apiversion[1]) >= 4 \
 else "vspk.vsdk." + cls.nuage_vsp_device.apiversion
 cls.vsdk = importlib.import_module(vspk_module)
 from libVSD import ApiClient, VSDHelpers
 except Exception as e:
 cls.tearDownClass()
-raise unittest.SkipTest("Warning: vspk (and/or) libVSD package 
import failure - %s" % e)
+raise unittest.SkipTest("Warning: vspk (and/or) libVSD package 
"
+"import failure - %s" % e)
 
 # Configure VSD session
-cls._session = 
cls.vsdk.NUVSDSession(username=cls.nuage_vsp_device.username,
- 
password=cls.nuage_vsp_device.password,
- enterprise="csp",
- api_url="https://%s:%d; % 
(cls.nuage_vsp_device.hostname,
-
cls.nuage_vsp_device.port)
- 

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77755943
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -77,85 +117,104 @@ def setUpClass(cls, zone=None):
 cls.test_data["virtual_machine"]["zoneid"] = cls.zone.id
 cls.test_data["virtual_machine"]["template"] = cls.template.id
 
-# Create service offering
-cls.service_offering = ServiceOffering.create(cls.api_client,
-  
cls.test_data["service_offering"]
-  )
-cls._cleanup = [cls.service_offering]
-
 # Check if the host hypervisor type is simulator
-cls.isSimulator = Hypervisor.list(cls.api_client, 
zoneid=cls.zone.id)[0].name == "Simulator"
+cls.isSimulator = Hypervisor.list(
+cls.api_client,
+zoneid=cls.zone.id)[0].name == "Simulator"
 
 # Get configured Nuage VSP device details
 try:
-physical_networks = PhysicalNetwork.list(cls.api_client, 
zoneid=cls.zone.id)
+physical_networks = PhysicalNetwork.list(
+cls.api_client,
+zoneid=cls.zone.id
+)
 for pn in physical_networks:
 if pn.isolationmethods == "VSP":
 cls.vsp_physical_network = pn
 break
-cls.nuage_vsp_device = Nuage.list(cls.api_client,
-  
physicalnetworkid=cls.vsp_physical_network.id
-  )[0]
+cls.nuage_vsp_device = Nuage.list(
+cls.api_client,
+physicalnetworkid=cls.vsp_physical_network.id)[0]
 pns = cls.config.zones[0].physical_networks
-providers = filter(lambda physical_network: "VSP" in 
physical_network.isolationmethods, pns)[0].providers
-devices = filter(lambda provider: provider.name == "NuageVsp", 
providers)[0].devices
+providers = filter(lambda physical_network:
+   "VSP" in physical_network.isolationmethods,
+   pns)[0].providers
+devices = filter(lambda provider:
+ provider.name == "NuageVsp",
+ providers)[0].devices
 cls.nuage_vsp_device.username = devices[0].username
 cls.nuage_vsp_device.password = devices[0].password
 cls.cms_id = cls.nuage_vsp_device.cmsid
 except Exception as e:
 cls.tearDownClass()
-raise unittest.SkipTest("Warning: Could not get configured 
Nuage VSP device details - %s" % e)
+raise unittest.SkipTest("Warning: Could not get configured "
+"Nuage VSP device details - %s" % e)
--- End diff --

Why shouldn't this exception cause the test to fail?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77755898
  
--- Diff: test/integration/plugins/nuagevsp/nuageTestCase.py ---
@@ -77,85 +117,104 @@ def setUpClass(cls, zone=None):
 cls.test_data["virtual_machine"]["zoneid"] = cls.zone.id
 cls.test_data["virtual_machine"]["template"] = cls.template.id
 
-# Create service offering
-cls.service_offering = ServiceOffering.create(cls.api_client,
-  
cls.test_data["service_offering"]
-  )
-cls._cleanup = [cls.service_offering]
-
 # Check if the host hypervisor type is simulator
-cls.isSimulator = Hypervisor.list(cls.api_client, 
zoneid=cls.zone.id)[0].name == "Simulator"
+cls.isSimulator = Hypervisor.list(
+cls.api_client,
+zoneid=cls.zone.id)[0].name == "Simulator"
--- End diff --

If the hypervisor list retrieval fails or returns an empty list, the error 
returned will be ambiguous.  Please split the retrieval of the list from the 
indexing operation and asserts that the hypervisor list is not ``None`` and 
expected length.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77755754
  
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -988,6 +1016,21 @@ public boolean addDhcpEntry(final Network network, 
final NicProfile nic, final V
 return result;
 }
 
+protected boolean deleteDhcpSupportForSubnet(Network network, 
Network.Service service) throws ResourceUnavailableException {
+if (canHandle(network, service)) {
+final List routers = 
_routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
+if (routers == null || routers.size() == 0) {
+throw new ResourceUnavailableException("Can't find at 
least one router!", DataCenter.class, network.getDataCenterId());
+}
+try {
+return _routerMgr.removeDhcpSupportForSubnet(network, 
routers);
+} catch (final ResourceUnavailableException e) {
+s_logger.debug("Router resource unavailable ");
--- End diff --

Why not log this information to ``ERROR`` or ``WARN``?  Please add context 
information about the ``routers`` and ``network`` to help operational 
debugging.  Finally, please add the exception to the log.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77755671
  
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -988,6 +1016,21 @@ public boolean addDhcpEntry(final Network network, 
final NicProfile nic, final V
 return result;
 }
 
+protected boolean deleteDhcpSupportForSubnet(Network network, 
Network.Service service) throws ResourceUnavailableException {
+if (canHandle(network, service)) {
+final List routers = 
_routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
+if (routers == null || routers.size() == 0) {
--- End diff --

Minor nit: Please consider using ``isEmpty`` rather than a size check as it 
is clearer intent and more idiomatic.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77755594
  
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -922,10 +921,56 @@ public boolean release(final Network network, final 
NicProfile nic, final Virtua
 return true;
 }
 
+
 @Override
 public boolean configDhcpSupportForSubnet(final Network network, final 
NicProfile nic, final VirtualMachineProfile vm, final DeployDestination dest,
-final ReservationContext context) throws 
ConcurrentOperationException, InsufficientCapacityException, 
ResourceUnavailableException {
-if (canHandle(network, Service.Dhcp)) {
+  final ReservationContext 
context) throws ConcurrentOperationException, InsufficientCapacityException, 
ResourceUnavailableException {
+return configureDhcpSupport(network, nic, vm, dest, 
Service.Dhcp);
+}
+
+@Override
+public boolean configDnsSupportForSubnet(Network network, NicProfile 
nic, VirtualMachineProfile vm, DeployDestination dest, ReservationContext 
context) throws ConcurrentOperationException, InsufficientCapacityException, 
ResourceUnavailableException {
+// Ignore if virtual router is already dhcp provider
+if 
(_networkModel.isProviderSupportServiceInNetwork(network.getId(), Service.Dhcp, 
getProvider())) {
+return true;
+} else {
--- End diff --

This ``else`` block is unnecessary since the ``if`` block returns.  Please 
remove it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77755606
  
--- Diff: server/src/com/cloud/network/element/VirtualRouterElement.java ---
@@ -922,10 +921,56 @@ public boolean release(final Network network, final 
NicProfile nic, final Virtua
 return true;
 }
 
+
 @Override
 public boolean configDhcpSupportForSubnet(final Network network, final 
NicProfile nic, final VirtualMachineProfile vm, final DeployDestination dest,
-final ReservationContext context) throws 
ConcurrentOperationException, InsufficientCapacityException, 
ResourceUnavailableException {
-if (canHandle(network, Service.Dhcp)) {
+  final ReservationContext 
context) throws ConcurrentOperationException, InsufficientCapacityException, 
ResourceUnavailableException {
+return configureDhcpSupport(network, nic, vm, dest, 
Service.Dhcp);
+}
+
+@Override
+public boolean configDnsSupportForSubnet(Network network, NicProfile 
nic, VirtualMachineProfile vm, DeployDestination dest, ReservationContext 
context) throws ConcurrentOperationException, InsufficientCapacityException, 
ResourceUnavailableException {
+// Ignore if virtual router is already dhcp provider
+if 
(_networkModel.isProviderSupportServiceInNetwork(network.getId(), Service.Dhcp, 
getProvider())) {
+return true;
+} else {
+return configureDhcpSupport(network, nic, vm, dest, 
Service.Dns);
+}
+}
+
+@Override
+public boolean removeDhcpSupportForSubnet(final Network network) 
throws ResourceUnavailableException {
+return deleteDhcpSupportForSubnet(network, Service.Dhcp);
+}
+
+@Override
+public boolean removeDnsSupportForSubnet(Network network) throws 
ResourceUnavailableException {
+// Ignore if virtual router is already dhcp provider
+if 
(_networkModel.isProviderSupportServiceInNetwork(network.getId(), Service.Dhcp, 
getProvider())) {
+return true;
+} else {
+return deleteDhcpSupportForSubnet(network, Service.Dns);
+}
+}
+
+@Override
+public boolean addDhcpEntry(final Network network, final NicProfile 
nic, final VirtualMachineProfile vm, final DeployDestination dest, final 
ReservationContext context)
+throws ConcurrentOperationException, 
InsufficientCapacityException, ResourceUnavailableException {
+return applyDhcpEntries(network, nic, vm, dest, Service.Dhcp);
+}
+
+@Override
+public boolean addDnsEntry(Network network, NicProfile nic, 
VirtualMachineProfile vm, DeployDestination dest, ReservationContext context) 
throws ConcurrentOperationException, InsufficientCapacityException, 
ResourceUnavailableException {
+// Ignore if virtual router is already dhcp provider
+if 
(_networkModel.isProviderSupportServiceInNetwork(network.getId(), Service.Dhcp, 
getProvider())) {
+return true;
+} else {
--- End diff --

This ``else`` block is unnecessary since the ``if`` block returns.  Please 
remove it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77755409
  
--- Diff: 
plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResource.java
 ---
@@ -386,6 +392,17 @@ private Answer executeRequest(TrashNetworkVspCommand 
cmd) {
 }
 }
 
+private Answer executeRequest(UpdateDhcpOptionVspCommand cmd) {
+try {
+isNuageVspManagerLoaded();
+_nuageVspGuruClient.applyDhcpOptions(cmd.getDhcpOptions(), 
cmd.getNetwork());
+return new Answer(cmd, true, "Update DhcpOptions on VM's in 
network: " + cmd.getNetwork().getName() + " on Nuage VSD " + _hostName);
+} catch (ExecutionException | ConfigurationException e) {
+s_logger.error("Failure during " + cmd + " on Nuage VSD " + 
_hostName, e);
--- End diff --

Please add ``cmd.getDhcpOptions`` to the log message to assist with 
operational debugging.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77754766
  
--- Diff: 
plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java
 ---
@@ -242,6 +256,44 @@ public void reserve(NicProfile nic, Network network, 
VirtualMachineProfile vm, D
 throw new IllegalStateException("The broadcast URI path " 
+ network.getBroadcastUri() + " is empty or in an incorrect format.");
 }
 
+HostVO nuageVspHost = 
getNuageVspHost(network.getPhysicalNetworkId());
+VspNetwork vspNetwork = 
_nuageVspEntityBuilder.buildVspNetwork(network, false);
+
+// Set flags for dhcp options
+boolean networkHasDns = networkHasDns(network);
+
+Map networkHasDnsCache = Maps.newHashMap();
+networkHasDnsCache.put(network.getId(), networkHasDns);
--- End diff --

Is there any reason ``networkHasDnsCache`` can't be an ``ImmutableMap``?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77754469
  
--- Diff: 
plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/ReserveVmInterfaceVspCommand.java
 ---
@@ -19,25 +19,29 @@
 
 package com.cloud.agent.api.guru;
 
-import com.cloud.agent.api.Command;
+import net.nuage.vsp.acs.client.api.model.VspDhcpVMOption;
 import net.nuage.vsp.acs.client.api.model.VspNetwork;
 import net.nuage.vsp.acs.client.api.model.VspNic;
 import net.nuage.vsp.acs.client.api.model.VspStaticNat;
 import net.nuage.vsp.acs.client.api.model.VspVm;
 
+import com.cloud.agent.api.Command;
+
 public class ReserveVmInterfaceVspCommand extends Command {
 
 private final VspNetwork _network;
 private final VspVm _vm;
 private final VspNic _nic;
 private final VspStaticNat _staticNat;
+private final  VspDhcpVMOption _dhcpOption;
--- End diff --

``private final VspNetwork`` and ``private final VspNetwork _network`` 
appear many of these command classes.  Would it make it sense to consolidate 
these commonalities in a VSP abstract command class?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77754362
  
--- Diff: 
plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/guru/ImplementNetworkVspCommand.java
 ---
@@ -56,7 +56,7 @@ public boolean equals(Object o) {
 
 ImplementNetworkVspCommand that = (ImplementNetworkVspCommand) o;
 
-if (_dnsServers != null ? !_dnsServers.equals(that._dnsServers) : 
that._dnsServers != null) return false;
+if (_dhcpOption != null ? !_dhcpOption.equals(that._dhcpOption) : 
that._dhcpOption != null) return false;
--- End diff --

Please consider simplifying this ``if`` block with Guava's 
``Objects.equals``.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77754300
  
--- Diff: 
plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/element/ShutDownVspCommand.java
 ---
@@ -0,0 +1,73 @@
+//
+// 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 com.cloud.agent.api.element;
+
+import net.nuage.vsp.acs.client.api.model.VspDhcpDomainOption;
+import net.nuage.vsp.acs.client.api.model.VspNetwork;
+
+import com.cloud.agent.api.Command;
+
+public class ShutDownVspCommand extends Command {
+
+private final VspNetwork _network;
+private final VspDhcpDomainOption _dhcpOptions;
+
+public ShutDownVspCommand(VspNetwork network, VspDhcpDomainOption 
dhcpOptions) {
+super();
+this._network = network;
+this._dhcpOptions = dhcpOptions;
+}
+
+public VspNetwork getNetwork() {
+return _network;
+}
+
+public VspDhcpDomainOption getDhcpOptions() {
+return _dhcpOptions;
+}
+
+@Override
+public boolean executeInSequence() {
+return false;
+}
+
+@Override
+public boolean equals(Object o) {
+if (this == o) return true;
+if (!(o instanceof ShutDownVspCommand)) return false;
+if (!super.equals(o)) return false;
--- End diff --

Per our coding standards, please wrap these ``if`` blocks with curly braces.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77754323
  
--- Diff: 
plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/element/ShutDownVspCommand.java
 ---
@@ -0,0 +1,73 @@
+//
+// 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 com.cloud.agent.api.element;
+
+import net.nuage.vsp.acs.client.api.model.VspDhcpDomainOption;
+import net.nuage.vsp.acs.client.api.model.VspNetwork;
+
+import com.cloud.agent.api.Command;
+
+public class ShutDownVspCommand extends Command {
+
+private final VspNetwork _network;
+private final VspDhcpDomainOption _dhcpOptions;
+
+public ShutDownVspCommand(VspNetwork network, VspDhcpDomainOption 
dhcpOptions) {
+super();
+this._network = network;
+this._dhcpOptions = dhcpOptions;
+}
+
+public VspNetwork getNetwork() {
+return _network;
+}
+
+public VspDhcpDomainOption getDhcpOptions() {
+return _dhcpOptions;
+}
+
+@Override
+public boolean executeInSequence() {
+return false;
+}
+
+@Override
+public boolean equals(Object o) {
+if (this == o) return true;
+if (!(o instanceof ShutDownVspCommand)) return false;
+if (!super.equals(o)) return false;
+
+ShutDownVspCommand that = (ShutDownVspCommand) o;
+
+if (_dhcpOptions != null ? !_dhcpOptions.equals(that._dhcpOptions) 
: that._dhcpOptions != null)
+return false;
+if (_network != null ? !_network.equals(that._network) : 
that._network != null) return false;
--- End diff --

Please consider simplifying these two ``if`` blocks with Guava's 
``Objects.equal``.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r77754184
  
--- Diff: 
engine/orchestration/src/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
 ---
@@ -2645,6 +2669,18 @@ public DhcpServiceProvider 
getDhcpServiceProvider(final Network network) {
 }
 }
 
+@Override
+public DnsServiceProvider getDnsServiceProvider(final Network network) 
{
+final String dnsProvider = 
_ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(), Service.Dns);
+
+if (dnsProvider == null) {
+s_logger.debug("Network " + network + " doesn't support 
service " + Service.Dhcp.getName());
--- End diff --

Should this message be logged as ``WARN`` instead of ``DEBUG``?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[fmaximus]

Github user fmaximus commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r76993494
  
--- Diff: 
engine/orchestration/src/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
 ---
@@ -2645,6 +2669,18 @@ public DhcpServiceProvider 
getDhcpServiceProvider(final Network network) {
 }
 }
 
+@Override
+public DnsServiceProvider getDnsServiceProvider(final Network network) 
{
+final String DnsProvider = 
_ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(), Service.Dns);
--- End diff --

Variables should start lowecase


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[fmaximus]

Github user fmaximus commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r76991083
  
--- Diff: 
plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java
 ---
@@ -242,6 +249,40 @@ public void reserve(NicProfile nic, Network network, 
VirtualMachineProfile vm, D
 throw new IllegalStateException("The broadcast URI path " 
+ network.getBroadcastUri() + " is empty or in an incorrect format.");
 }
 
+HostVO nuageVspHost = 
getNuageVspHost(network.getPhysicalNetworkId());
+VspNetwork vspNetwork = 
_nuageVspEntityBuilder.buildVspNetwork(network, false);
+
+// Set flags for dhcp options
+boolean networkHasDns = networkHasDns(network);
+Boolean defaultHasDns = null;
+// Determine if dhcp options of the other nics in the network 
need to be updated
+if (VirtualMachine.Type.DomainRouter.equals(vm.getType()) && 
!State.Implementing.equals(network.getState())) {
+// Update dhcp options if a VR is added when we are not 
initiating the network
+s_logger.debug(String.format("DomainRouter is added to an 
existing network: %s in state: %s", network.getName(), network.getState()));
+List dhcpOptions = Lists.newArrayList();
+for (NicVO userNic 
:_nicDao.listByNetworkId(network.getId())){
+if 
(!VirtualMachine.Type.DomainRouter.equals(userNic.getVmType())) {
+// Dhcp options for Domain router are handled later
+VMInstanceVO userVm  = 
_vmInstanceDao.findById(userNic.getInstanceId());
--- End diff --

It is not necessary to fetch the vm here, as only the id is used.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[fmaximus]

Github user fmaximus commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1578#discussion_r76991270
  
--- Diff: 
plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java
 ---
@@ -403,4 +448,25 @@ private HostVO getNuageVspHost(long physicalNetworkId) 
{
 }
 return nuageVspHost;
 }
+
+private boolean networkHasDns(Network network) {
+
+if (network != null) {
+List dnsProvider = 
_ntwkOfferingSrvcDao.listProvidersForServiceForNetworkOffering(network.getNetworkOfferingId(),
 Network.Service.Dns);
+if (dnsProvider.contains("VirtualRouter") || 
dnsProvider.contains("VpcVirtualRouter")) {
+return true;
+}
+}
+return false;
+}
+
+
+private NetworkVO getDefaultNetwork (long vmId) {
+
+NicVO defaultNic = _nicDao.findDefaultNicForVM(vmId);
+if (defaultNic != null ) return 
_networkDao.findById(_nicDao.findById(defaultNic.getId()).getNetworkId());
--- End diff --

Not necessary to fetch the nic from DB twice.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1578: CLOUDSTACK-9401 : Support for Internal DNS in...

[nlivens]

GitHub user nlivens opened a pull request:

https://github.com/apache/cloudstack/pull/1578

CLOUDSTACK-9401 : Support for Internal DNS in Nuage VSP plugin

Testrun:-
Verify InternalDns on Isolated Network ... === TestName: 
test_01_Isolated_Network_with_zone | Status : SUCCESS ===
ok
Verify InternalDns on Isolated Network with ping by hostname ... === 
TestName: test_02_Isolated_Network | Status : SUCCESS ===
ok
Verify update NetworkDomain for InternalDns on Isolated Network ... === 
TestName: test_03_Update_Network_with_Domain | Status : SUCCESS ===
ok
Verify update NetworkDomain for InternalDns on Isolated Network with ping 
VM ... === TestName: test_04_Update_Network_with_Domain | Status : SUCCESS ===
ok
Verify InternalDns on VPC Network ... === TestName: 
test_05_VPC_Network_With_InternalDns | Status : SUCCESS ===
ok
Verify InternalDns on VPC Network by ping with hostname ... === TestName: 
test_06_VPC_Network_With_InternalDns | Status : SUCCESS ===
ok
--
Ran 6 tests in 5736.562s
OK
cloudstack$ pep8 --max-line-length=150 test_internal_dns.py
cloudstack$ pyflakes test_internal_dns.py
cloudstack$

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/nlivens/cloudstack nuage_vsp_internal_dns

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1578.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1578


commit c86ab936f03b9815debaa535065df10dc724149f
Author: Eric Waegeman 
Date:   2016-05-26T09:37:52Z

CLOUDSTACK-9401 : Support for Internal DNS in Nuage VSP plugin

commit 064646de7604ffb58a4ae7ecc3db103e4e067fbf
Author: rahul singal 
Date:   2016-06-02T09:30:25Z

CLOUDSTACK-9401 : Marvin tests for Internal DNS verification with NuageVsp




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---