[GitHub] cloudstack pull request #1996: CLOUDSTACK-9099: SecretKey is returned from t...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1996#discussion_r111075302
  
--- Diff: 
api/src/org/apache/cloudstack/api/command/admin/user/GetUserKeysCmd.java ---
@@ -0,0 +1,76 @@
+// 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.user;
+
+
+import com.cloud.user.Account;
+import com.cloud.user.User;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.RegisterResponse;
+import org.apache.cloudstack.api.response.UserResponse;
+
+import java.util.List;
+import java.util.logging.Logger;
+
+@APICommand(name = "getUserKeys",
+description = "This command allows the user to query the 
seceret and API keys for the account",
+responseObject = RegisterResponse.class,
+requestHasSensitiveInfo = false,
+responseHasSensitiveInfo = true,
+authorized = {RoleType.User})
+
+public class GetUserKeysCmd extends BaseCmd{
+
+@Parameter(name= ApiConstants.ID, type = CommandType.UUID, entityType 
= UserResponse.class, required = true, description = "ID of the user whose keys 
are required")
+private Long id;
+
+public static final Logger s_logger = 
Logger.getLogger(RegisterCmd.class.getName());
+public static final String s_name = "getuserkeysresponse";
+
+public Long getID(){
+return id;
+}
+
+public String getCommandName(){
+return s_name;
+}
+
+public long getEntityOwnerId(){
+User user = _entityMgr.findById(User.class, getID());
+if(user != null){
+return user.getAccountId();
+}
+else return Account.ACCOUNT_ID_SYSTEM;
+}
+public void execute(){
+List keys = _accountService.getKeys(this);
+RegisterResponse response = new RegisterResponse();
+if(keys != null){
+response.setApiKey(keys.get(0));
+response.setSecretKey(keys.get(1));
+}
+
+response.setObjectName("listkeys");
--- End diff --

Should this be 'userKeys'?


---
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 #1996: CLOUDSTACK-9099: SecretKey is returned from t...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1996#discussion_r111075076
  
--- Diff: 
api/src/org/apache/cloudstack/api/command/admin/user/GetUserKeysCmd.java ---
@@ -0,0 +1,76 @@
+// 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.user;
+
+
+import com.cloud.user.Account;
+import com.cloud.user.User;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.RegisterResponse;
+import org.apache.cloudstack.api.response.UserResponse;
+
+import java.util.List;
+import java.util.logging.Logger;
+
+@APICommand(name = "getUserKeys",
+description = "This command allows the user to query the 
seceret and API keys for the account",
+responseObject = RegisterResponse.class,
+requestHasSensitiveInfo = false,
+responseHasSensitiveInfo = true,
+authorized = {RoleType.User})
--- End diff --

Can you add the 'since' parameter? 


---
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 #1996: CLOUDSTACK-9099: SecretKey is returned from t...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1996#discussion_r111078382
  
--- Diff: server/src/com/cloud/api/ApiDBUtils.java ---
@@ -559,6 +561,8 @@
 @Inject
 private VpcManager vpcMgr;
 @Inject
+private AccountManager accountManager;
--- End diff --

Why there is a need to inject AccountManager? If the config key is a static 
it can be accessed as AccountManager.


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

Re: MySQL 5.7 and SQL Mode

[Koushik Das]

Hi Wido,

Check initDataSource() in TransactionLegacy.java. The connection properties are 
read from db.properties.

-Koushik

On 11/04/17, 10:27 PM, "Wido den Hollander"  wrote:


> Op 11 april 2017 om 10:51 schreef Rohit Yadav :
> 
> 
> Hi Wido,
> 
> 
> You're right, MySQL 5.7 has by default strict(er) sql mode enabled. To 
make CloudStack work with MySQL 5.7, changing the sql mode makes MySQL 5.7 
behave like 5.6 with which the mgmt server/usage server should work.
> 

Yes, but a client can do this as well. It doesn't have to be server wide.

Do you know where the MySQL client is initiated by CloudStack? A few lines 
of code there should/might be enough.

Wido

> 
> Regards.
> 
> 
> From: Wido den Hollander 
> Sent: 10 April 2017 20:30:55
> To: dev@cloudstack.apache.org
> Subject: MySQL 5.7 and SQL Mode
> 
> Hi,
> 
> While testing with Ubuntu 16.04 and CloudStack 4.10 (from master) I've 
ran into this error on the management server:
> 
> com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Expression #1 
of SELECT list is not in GROUP BY clause and contains nonaggregated column 
'cloud.i.id' which is not functionally dependent on columns in GROUP BY clause; 
this is incompatible with sql_mode=only_full_group_by
> at sun.reflect.GeneratedConstructorAccessor50.newInstance(Unknown 
Source)
> at 
sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
> at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
> at com.mysql.jdbc.Util.handleNewInstance(Util.java:404)
> at com.mysql.jdbc.Util.getInstance(Util.java:387)
> at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:939)
> at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:3878)
> at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:3814)
> 
> I was able to fix this to add this to my my.cnf:
> 
> [mysqld]
> sql_mode = 
"STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION"
> 
> Should we maybe set the SQL Mode as a connection parameter when 
connecting to the DB? This prevents users from having to set this manually in 
their MySQL configuration.
> 
> Did somebody else run into this with MySQL 5.7?
> 
> Thank you,
> 
> Wido
> 
> rohit.ya...@shapeblue.com 
> www.shapeblue.com
> 53 Chandos Place, Covent Garden, London  WC2N 4HSUK
> @shapeblue
>   
>  
>





DISCLAIMER
==
This e-mail may contain privileged and confidential information which is the 
property of Accelerite, a Persistent Systems business. It is intended only for 
the use of the individual or entity to which it is addressed. If you are not 
the intended recipient, you are not authorized to read, retain, copy, print, 
distribute or use this message. If you have received this communication in 
error, please notify the sender and delete all copies of this message. 
Accelerite, a Persistent Systems business does not accept any liability for 
virus infected mails.

Re: growing #threads problem

[Koushik Das]

Check the MS logs to see if there are OOM exceptions due to threads not 
available. If that’s the case then you may have to adjust nproc limits in 
/etc/security/limits.conf.


On 11/04/17, 1:11 PM, "Daan Hoogland"  wrote:

My next step is to rebase my change to 4.9 and see if that works. But that 
is a stab in the dark I must admit.

On 11/04/17 09:30, "Daan Hoogland"  wrote:

Devs,

While I am creating a runnable tob e scheduled I ran into a problem. My 
scheduledJob nevers gets called and looking at the process the number of 
threads of type “DirectAgent-*” and “DirectAgentCronJob-*” keep growing. My job 
is scanning for templates in the primary storage and references to it in the 
database, so it seems to be unrelated. This is a 4.10 vmware based nested 
environment. Has anyone seen such a behaviour? I am still looking but I hope I 
won’t be reporting a blocker for 4.10 on this….

Any clue is welcome at this point,

daan.hoogl...@shapeblue.com 
www.shapeblue.com
53 Chandos Place, Covent Garden, London  WC2N 4HSUK
@shapeblue
  
 




daan.hoogl...@shapeblue.com 
www.shapeblue.com
53 Chandos Place, Covent Garden, London  WC2N 4HSUK
@shapeblue
  
 






DISCLAIMER
==
This e-mail may contain privileged and confidential information which is the 
property of Accelerite, a Persistent Systems business. It is intended only for 
the use of the individual or entity to which it is addressed. If you are not 
the intended recipient, you are not authorized to read, retain, copy, print, 
distribute or use this message. If you have received this communication in 
error, please notify the sender and delete all copies of this message. 
Accelerite, a Persistent Systems business does not accept any liability for 
virus infected mails.

[GitHub] cloudstack issue #1960: [4.11/Future] CLOUDSTACK-9782: Host HA and KVM HA pr...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1960
  
@abhinandanprateek Initially I also thought that this is about host HA. But 
after reading the FS I had doubts and asked about the definition of "host HA". 
If you refer to the discussion on dev@, it was mentioned that "host HA" would 
trigger VM HA so that they are started on other hosts. This is the same as 
existing VM HA and I don't think we should refer to it as host HA.

@rhtyd I had replied with some specific questions/comments on the 
discussion thread. I didn't see any responses on those.

Basically I would like to see a clear definition of "host HA". When a host 
is HA'd what all should happen? If the definition provided is nothing but VM HA 
then the question comes why a new framework when the existing framework is 
already there. 


---
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 issue #2032: CLOUDSTACK-9783: corrected the version number in met...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/2032
  
LGTM. Build is broken due to this.


---
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 issue #1960: [4.11/Future] CLOUDSTACK-9782: Host HA and KVM HA pr...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1960
  
There are open questions that I had asked in the dev@ list and haven't seen 
satisfactory answers to them. I am -1 on this feature till the need for a new 
VM HA framework is justified.


---
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 issue #2019: CLOUDSTACK-9851 travis CI build failure after merge ...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/2019
  
Code changes LGTM. @SudharmaJain Can you check on the travis failures?


---
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 issue #1953: CLOUDSTACK-9794: Unable to attach more than 14 devic...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1953
  
@karuturi This PR is not correct and is resulting in travis CI failures. It 
needs to be properly fixed.


---
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 issue #2019: CLOUDSTACK-9851 travis CI build failure after merge ...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/2019
  
@SudharmaJain Although the PR fixes the tests but it is not correct. I 
looked at PR #1953 and the changes there are incorrect.

getMaxDataVolumes() returns only the data volume count by excluding the 
root and cd-rom device. That PR is again subtracting them which results in 
incorrect limit.


---
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 issue #1853: CLOUDSTACK-9696: Fixed Virtual Router deployment fai...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1853
  
Code change LGTM.
@anshul1886 Please make sure Travis is green.


---
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 issue #1867: CLOUDSTACK-9706: Added snapshots cleanup in start an...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1867
  
Code changes LGTM
@anshul1886 Please check on travis failure


---
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 #1853: CLOUDSTACK-9696: Fixed Virtual Router deploym...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1853#discussion_r106589966
  
--- Diff: server/src/com/cloud/dc/dao/DedicatedResourceDaoImpl.java ---
@@ -312,6 +317,65 @@ public DedicatedResourceVO findByHostId(Long hostId) {
 }
 
 @Override
+public List listAvailableResources(Long 
accountId, Long... domains) {
--- End diff --

@anshul1886 Check out SearchBuilder class, it has methods like or(), op(), 
cp() which can be used to construct the query. Also the same field can be used 
in multiple places, search for reference to or(), op() etc. elsewhere in the 
code and you will find how to use them. One such place is QueryManagerImpl.java


---
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 #1853: CLOUDSTACK-9696: Fixed Virtual Router deploym...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1853#discussion_r106590720
  
--- Diff: server/src/com/cloud/deploy/DeploymentPlanningManagerImpl.java ---
@@ -584,105 +583,64 @@ private void 
checkForNonDedicatedResources(VirtualMachineProfile vmProfile, Data
 isExplicit = true;
 }
 
-List allPodsInDc = _podDao.listAllPods(dc.getId());
-List allDedicatedPods = _dedicatedDao.listAllPods();
-allPodsInDc.retainAll(allDedicatedPods);
-
-List allClustersInDc = 
_clusterDao.listAllCusters(dc.getId());
-List allDedicatedClusters = _dedicatedDao.listAllClusters();
-allClustersInDc.retainAll(allDedicatedClusters);
-
-List allHostsInDc = _hostDao.listAllHosts(dc.getId());
-List allDedicatedHosts = _dedicatedDao.listAllHosts();
-allHostsInDc.retainAll(allDedicatedHosts);
-
-//Only when the type is instance VM and not explicitly dedicated.
-if (vm.getType() == VirtualMachine.Type.User && !isExplicit) {
-//add explicitly dedicated resources in avoidList
-
-avoids.addPodList(allPodsInDc);
-avoids.addClusterList(allClustersInDc);
-avoids.addHostList(allHostsInDc);
-}
-
-//Handle the Virtual Router Case
-//No need to check the isExplicit. As both the cases are handled.
-if (vm.getType() == VirtualMachine.Type.DomainRouter) {
-long vmAccountId = vm.getAccountId();
-long vmDomainId = vm.getDomainId();
-
-//Lists all explicitly dedicated resources from vm account ID 
or domain ID.
-List allPodsFromDedicatedID = new ArrayList();
-List allClustersFromDedicatedID = new ArrayList();
-List allHostsFromDedicatedID = new ArrayList();
-
-//Whether the dedicated resources belong to Domain or not. If 
not, it may belongs to Account or no dedication.
-List domainGroupMappings = 
_affinityGroupDomainMapDao.listByDomain(vmDomainId);
-
-//For temporary storage and indexing.
-List tempStorage;
-
-if (domainGroupMappings == null || 
domainGroupMappings.isEmpty()) {
-//The dedicated resource belongs to VM Account ID.
-
-tempStorage = _dedicatedDao.searchDedicatedPods(null, 
vmDomainId, vmAccountId, null).first();
-
-for(DedicatedResourceVO vo : tempStorage) {
-allPodsFromDedicatedID.add(vo.getPodId());
-}
-
-tempStorage.clear();
-tempStorage = _dedicatedDao.searchDedicatedClusters(null, 
vmDomainId, vmAccountId, null).first();
+if ((vm.getType() == VirtualMachine.Type.User && !isExplicit) || 
vm.getType() == VirtualMachine.Type.DomainRouter) {
+List allPodsInDc = _podDao.listAllPods(dc.getId());
+List allDedicatedPods = _dedicatedDao.listAllPods();
+allPodsInDc.retainAll(allDedicatedPods);
 
-for(DedicatedResourceVO vo : tempStorage) {
-allClustersFromDedicatedID.add(vo.getClusterId());
-}
+List allClustersInDc = 
_clusterDao.listAllCusters(dc.getId());
+List allDedicatedClusters = 
_dedicatedDao.listAllClusters();
+allClustersInDc.retainAll(allDedicatedClusters);
 
-tempStorage.clear();
-tempStorage = _dedicatedDao.searchDedicatedHosts(null, 
vmDomainId, vmAccountId, null).first();
+List allHostsInDc = _hostDao.listAllHosts(dc.getId());
+List allDedicatedHosts = _dedicatedDao.listAllHosts();
+allHostsInDc.retainAll(allDedicatedHosts);
 
-for(DedicatedResourceVO vo : tempStorage) {
-allHostsFromDedicatedID.add(vo.getHostId());
-}
+//Only when the type is instance VM and not explicitly 
dedicated.
+if (vm.getType() == VirtualMachine.Type.User && !isExplicit) {
+//add explicitly dedicated resources in avoidList
 
-//Remove the dedicated ones from main list
-allPodsInDc.removeAll(allPodsFromDedicatedID);
-allClustersInDc.removeAll(allClustersFromDedicatedID);
-allHostsInDc.removeAll(allHostsFromDedicatedID);
+avoids.addPodList(allPodsInDc);
+avoids.addClusterList(allClustersInDc);
+avoids.addHostList(allHostsInDc);
 }
-else {
-//The dedicate

[GitHub] cloudstack pull request #1853: CLOUDSTACK-9696: Fixed Virtual Router deploym...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1853#discussion_r106591079
  
--- Diff: server/src/com/cloud/deploy/DeploymentPlanningManagerImpl.java ---
@@ -584,105 +583,64 @@ private void 
checkForNonDedicatedResources(VirtualMachineProfile vmProfile, Data
 isExplicit = true;
 }
 
-List allPodsInDc = _podDao.listAllPods(dc.getId());
-List allDedicatedPods = _dedicatedDao.listAllPods();
-allPodsInDc.retainAll(allDedicatedPods);
-
-List allClustersInDc = 
_clusterDao.listAllCusters(dc.getId());
-List allDedicatedClusters = _dedicatedDao.listAllClusters();
-allClustersInDc.retainAll(allDedicatedClusters);
-
-List allHostsInDc = _hostDao.listAllHosts(dc.getId());
-List allDedicatedHosts = _dedicatedDao.listAllHosts();
-allHostsInDc.retainAll(allDedicatedHosts);
-
-//Only when the type is instance VM and not explicitly dedicated.
-if (vm.getType() == VirtualMachine.Type.User && !isExplicit) {
-//add explicitly dedicated resources in avoidList
-
-avoids.addPodList(allPodsInDc);
-avoids.addClusterList(allClustersInDc);
-avoids.addHostList(allHostsInDc);
-}
-
-//Handle the Virtual Router Case
-//No need to check the isExplicit. As both the cases are handled.
-if (vm.getType() == VirtualMachine.Type.DomainRouter) {
-long vmAccountId = vm.getAccountId();
-long vmDomainId = vm.getDomainId();
-
-//Lists all explicitly dedicated resources from vm account ID 
or domain ID.
-List allPodsFromDedicatedID = new ArrayList();
-List allClustersFromDedicatedID = new ArrayList();
-List allHostsFromDedicatedID = new ArrayList();
-
-//Whether the dedicated resources belong to Domain or not. If 
not, it may belongs to Account or no dedication.
-List domainGroupMappings = 
_affinityGroupDomainMapDao.listByDomain(vmDomainId);
-
-//For temporary storage and indexing.
-List tempStorage;
-
-if (domainGroupMappings == null || 
domainGroupMappings.isEmpty()) {
-//The dedicated resource belongs to VM Account ID.
-
-tempStorage = _dedicatedDao.searchDedicatedPods(null, 
vmDomainId, vmAccountId, null).first();
-
-for(DedicatedResourceVO vo : tempStorage) {
-allPodsFromDedicatedID.add(vo.getPodId());
-}
-
-tempStorage.clear();
-tempStorage = _dedicatedDao.searchDedicatedClusters(null, 
vmDomainId, vmAccountId, null).first();
+if ((vm.getType() == VirtualMachine.Type.User && !isExplicit) || 
vm.getType() == VirtualMachine.Type.DomainRouter) {
--- End diff --

@anshul1886 Got 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 #1853: CLOUDSTACK-9696: Fixed Virtual Router deploym...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1853#discussion_r106138864
  
--- Diff: server/src/com/cloud/dc/dao/DedicatedResourceDaoImpl.java ---
@@ -312,6 +317,65 @@ public DedicatedResourceVO findByHostId(Long hostId) {
 }
 
 @Override
+public List listAvailableResources(Long 
accountId, Long... domains) {
--- End diff --

Please rewrite this using the standard query builder (SearchBuilder) that 
is available and used in the other methods in this 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 #1853: CLOUDSTACK-9696: Fixed Virtual Router deploym...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1853#discussion_r106142726
  
--- Diff: server/src/com/cloud/deploy/DeploymentPlanningManagerImpl.java ---
@@ -584,105 +583,64 @@ private void 
checkForNonDedicatedResources(VirtualMachineProfile vmProfile, Data
 isExplicit = true;
 }
 
-List allPodsInDc = _podDao.listAllPods(dc.getId());
-List allDedicatedPods = _dedicatedDao.listAllPods();
-allPodsInDc.retainAll(allDedicatedPods);
-
-List allClustersInDc = 
_clusterDao.listAllCusters(dc.getId());
-List allDedicatedClusters = _dedicatedDao.listAllClusters();
-allClustersInDc.retainAll(allDedicatedClusters);
-
-List allHostsInDc = _hostDao.listAllHosts(dc.getId());
-List allDedicatedHosts = _dedicatedDao.listAllHosts();
-allHostsInDc.retainAll(allDedicatedHosts);
-
-//Only when the type is instance VM and not explicitly dedicated.
-if (vm.getType() == VirtualMachine.Type.User && !isExplicit) {
-//add explicitly dedicated resources in avoidList
-
-avoids.addPodList(allPodsInDc);
-avoids.addClusterList(allClustersInDc);
-avoids.addHostList(allHostsInDc);
-}
-
-//Handle the Virtual Router Case
-//No need to check the isExplicit. As both the cases are handled.
-if (vm.getType() == VirtualMachine.Type.DomainRouter) {
-long vmAccountId = vm.getAccountId();
-long vmDomainId = vm.getDomainId();
-
-//Lists all explicitly dedicated resources from vm account ID 
or domain ID.
-List allPodsFromDedicatedID = new ArrayList();
-List allClustersFromDedicatedID = new ArrayList();
-List allHostsFromDedicatedID = new ArrayList();
-
-//Whether the dedicated resources belong to Domain or not. If 
not, it may belongs to Account or no dedication.
-List domainGroupMappings = 
_affinityGroupDomainMapDao.listByDomain(vmDomainId);
-
-//For temporary storage and indexing.
-List tempStorage;
-
-if (domainGroupMappings == null || 
domainGroupMappings.isEmpty()) {
-//The dedicated resource belongs to VM Account ID.
-
-tempStorage = _dedicatedDao.searchDedicatedPods(null, 
vmDomainId, vmAccountId, null).first();
-
-for(DedicatedResourceVO vo : tempStorage) {
-allPodsFromDedicatedID.add(vo.getPodId());
-}
-
-tempStorage.clear();
-tempStorage = _dedicatedDao.searchDedicatedClusters(null, 
vmDomainId, vmAccountId, null).first();
+if ((vm.getType() == VirtualMachine.Type.User && !isExplicit) || 
vm.getType() == VirtualMachine.Type.DomainRouter) {
--- End diff --

Any reason for this check? checkForNonDedicatedResources() is anyway called 
for user and router VMs.


---
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 #1853: CLOUDSTACK-9696: Fixed Virtual Router deploym...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1853#discussion_r106145072
  
--- Diff: server/src/com/cloud/deploy/DeploymentPlanningManagerImpl.java ---
@@ -584,105 +583,64 @@ private void 
checkForNonDedicatedResources(VirtualMachineProfile vmProfile, Data
 isExplicit = true;
 }
 
-List allPodsInDc = _podDao.listAllPods(dc.getId());
-List allDedicatedPods = _dedicatedDao.listAllPods();
-allPodsInDc.retainAll(allDedicatedPods);
-
-List allClustersInDc = 
_clusterDao.listAllCusters(dc.getId());
-List allDedicatedClusters = _dedicatedDao.listAllClusters();
-allClustersInDc.retainAll(allDedicatedClusters);
-
-List allHostsInDc = _hostDao.listAllHosts(dc.getId());
-List allDedicatedHosts = _dedicatedDao.listAllHosts();
-allHostsInDc.retainAll(allDedicatedHosts);
-
-//Only when the type is instance VM and not explicitly dedicated.
-if (vm.getType() == VirtualMachine.Type.User && !isExplicit) {
-//add explicitly dedicated resources in avoidList
-
-avoids.addPodList(allPodsInDc);
-avoids.addClusterList(allClustersInDc);
-avoids.addHostList(allHostsInDc);
-}
-
-//Handle the Virtual Router Case
-//No need to check the isExplicit. As both the cases are handled.
-if (vm.getType() == VirtualMachine.Type.DomainRouter) {
-long vmAccountId = vm.getAccountId();
-long vmDomainId = vm.getDomainId();
-
-//Lists all explicitly dedicated resources from vm account ID 
or domain ID.
-List allPodsFromDedicatedID = new ArrayList();
-List allClustersFromDedicatedID = new ArrayList();
-List allHostsFromDedicatedID = new ArrayList();
-
-//Whether the dedicated resources belong to Domain or not. If 
not, it may belongs to Account or no dedication.
-List domainGroupMappings = 
_affinityGroupDomainMapDao.listByDomain(vmDomainId);
-
-//For temporary storage and indexing.
-List tempStorage;
-
-if (domainGroupMappings == null || 
domainGroupMappings.isEmpty()) {
-//The dedicated resource belongs to VM Account ID.
-
-tempStorage = _dedicatedDao.searchDedicatedPods(null, 
vmDomainId, vmAccountId, null).first();
-
-for(DedicatedResourceVO vo : tempStorage) {
-allPodsFromDedicatedID.add(vo.getPodId());
-}
-
-tempStorage.clear();
-tempStorage = _dedicatedDao.searchDedicatedClusters(null, 
vmDomainId, vmAccountId, null).first();
+if ((vm.getType() == VirtualMachine.Type.User && !isExplicit) || 
vm.getType() == VirtualMachine.Type.DomainRouter) {
+List allPodsInDc = _podDao.listAllPods(dc.getId());
+List allDedicatedPods = _dedicatedDao.listAllPods();
+allPodsInDc.retainAll(allDedicatedPods);
 
-for(DedicatedResourceVO vo : tempStorage) {
-allClustersFromDedicatedID.add(vo.getClusterId());
-}
+List allClustersInDc = 
_clusterDao.listAllCusters(dc.getId());
+List allDedicatedClusters = 
_dedicatedDao.listAllClusters();
+allClustersInDc.retainAll(allDedicatedClusters);
 
-tempStorage.clear();
-tempStorage = _dedicatedDao.searchDedicatedHosts(null, 
vmDomainId, vmAccountId, null).first();
+List allHostsInDc = _hostDao.listAllHosts(dc.getId());
+List allDedicatedHosts = _dedicatedDao.listAllHosts();
+allHostsInDc.retainAll(allDedicatedHosts);
 
-for(DedicatedResourceVO vo : tempStorage) {
-allHostsFromDedicatedID.add(vo.getHostId());
-}
+//Only when the type is instance VM and not explicitly 
dedicated.
+if (vm.getType() == VirtualMachine.Type.User && !isExplicit) {
+//add explicitly dedicated resources in avoidList
 
-//Remove the dedicated ones from main list
-allPodsInDc.removeAll(allPodsFromDedicatedID);
-allClustersInDc.removeAll(allClustersFromDedicatedID);
-allHostsInDc.removeAll(allHostsFromDedicatedID);
+avoids.addPodList(allPodsInDc);
+avoids.addClusterList(allClustersInDc);
+avoids.addHostList(allHostsInDc);
 }
-else {
-//The dedicate

[GitHub] cloudstack pull request #1867: CLOUDSTACK-9706: Added snapshots cleanup in s...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1867#discussion_r106133214
  
--- Diff: 
engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java
 ---
@@ -191,7 +191,8 @@ public void 
processEvent(ObjectInDataStoreStateMachine.Event event) {
 s_logger.debug("Failed to update state:" + e.toString());
 throw new CloudRuntimeException("Failed to update state: " + 
e.toString());
 } finally {
-if (event == 
ObjectInDataStoreStateMachine.Event.OperationFailed) {
+DataObjectInStore obj = objectInStoreMgr.findObject(this, 
this.getDataStore());
+if (event == 
ObjectInDataStoreStateMachine.Event.OperationFailed && 
!obj.getState().equals(ObjectInDataStoreStateMachine.State.Destroying)) {
--- End diff --

Is there a possibility for 'obj' to be null? Can you put some comment as to 
why there is a need to check 'obj' state not in 'Destroying'?


---
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 #1867: CLOUDSTACK-9706: Added snapshots cleanup in s...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1867#discussion_r103894706
  
--- Diff: server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java ---
@@ -1181,6 +1181,17 @@ public boolean configure(String name, Map<String, 
Object> params) throws Configu
 
 @Override
 public boolean start() {
+//destroy snapshots in destroying state
+List dsnapshots = 
_snapshotDao.listAllByStatus(Snapshot.State.Destroying);
--- End diff --

Simply use snapshots instead of 'dsnapshots'.


---
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 #1867: CLOUDSTACK-9706: Added snapshots cleanup in s...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1867#discussion_r106134695
  
--- Diff: 
engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java
 ---
@@ -194,18 +194,22 @@ protected boolean deleteSnapshotChain(SnapshotInfo 
snapshot) {
 }
 }
 if (!deleted) {
-boolean r = snapshotSvr.deleteSnapshot(snapshot);
-if (r) {
-// delete snapshot in cache if there is
-List cacheSnaps = 
snapshotDataFactory.listSnapshotOnCache(snapshot.getId());
-for (SnapshotInfo cacheSnap : cacheSnaps) {
-s_logger.debug("Delete snapshot " + 
snapshot.getId() + " from image cache store: " + 
cacheSnap.getDataStore().getName());
-cacheSnap.delete();
+try {
+boolean r = snapshotSvr.deleteSnapshot(snapshot);
+if (r) {
+// delete snapshot in cache if there is
+List cacheSnaps = 
snapshotDataFactory.listSnapshotOnCache(snapshot.getId());
+for (SnapshotInfo cacheSnap : cacheSnaps) {
+s_logger.debug("Delete snapshot " + 
snapshot.getId() + " from image cache store: " + 
cacheSnap.getDataStore().getName());
+cacheSnap.delete();
+}
 }
-}
-if (!resultIsSet) {
-result = r;
-resultIsSet = true;
+if (!resultIsSet) {
+result = r;
+resultIsSet = true;
+}
+} catch (Exception e){
--- End diff --

Can you put a comment here as well as to why there is catch all so that the 
intent is clear? Also there are some minor formatting issues, please fix them.


---
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 #1867: CLOUDSTACK-9706: Added snapshots cleanup in s...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1867#discussion_r106137098
  
--- Diff: server/src/com/cloud/storage/StorageManagerImpl.java ---
@@ -1078,6 +1082,16 @@ public void cleanupStorage(boolean recurring) {
 }
 }
 
+//destroy snapshots in destroying state in 
snapshot_store_ref
+List  ssSnapshots = 
_snapshotStoreDao.listByState(ObjectInDataStoreStateMachine.State.Destroying);
+for(SnapshotDataStoreVO ssSnapshotVO : ssSnapshots){
+try {
+
_snapshotService.deleteSnapshot(snapshotFactory.getSnapshot(ssSnapshotVO.getSnapshotId(),
 DataStoreRole.Image));
--- End diff --

deleteSnapshot() is going to send an agent command to cleanup the snapshot 
from secondary, if there is any failure will cleanup be retried or the DB entry 
is simply updated?


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

Re: [Proposal] - StorageHA

[Koushik Das]

Hi Jeromy,

Thanks for the proposal. It will good if you can create a FS in cwiki for the 
same. I saw your comment about force stopping VMs affected by primary storage 
outage. If this can be done without any issues then it can be leveraged to 
improve the current behavior of Cloudstack as well. Currently in case of 
primary storage outage all hosts attached to it are rebooted based on some 
timeout (for XS and KVM). Refer to the heartbeat scripts 
(scripts/vm/hypervisor/xenserver/xenheartbeat.sh and 
scripts/vm/hypervisor/kvm/kvmheartbeat.sh).

"3.  You need to be very sure of failures before shutting hosts down.  Also a 
host is likely to be connected to multiple storage pools, so you wouldn't want 
to shut down a host due to one pool becoming unavailable."

JG:  The script wouldn’t shut down any hosts at all.  Just force stop the 
affected VMs on that specific host and then start them on a host that is not 
having the issue with storage.

Thanks,
Koushik


On 15/03/17, 12:34 AM, "Rafael Weingärtner"  wrote:

Jeromy I already experienced a similar problem. A host had a problem of
connectivity with a storage, ACS did not know (I think it does check this
right now) and it was kind of odd. Because some VMs could not be started,
because of their last host id was the host with the connectivity problem.

I believe it would be a great thing to improve these checks and to treat
this type of problem in ACS.

My two cents:
Change the name of what you are proposing. It is not high availability per
se; it is more like a storage health check/heartbeat thing. It may give
people very high expectations (I am not trying to undermine the work that
is required). For instance, the first time I read your email, I thought
about real HA, with some sort of block replication and redundancy in the
storage system. That means, I was expecting if a whole storage goes down,
ACS would have to have a way of solving this problem without losing data
and without interrupting VMs. What you want to do is a little different, if
a host has some connectivity problem, we simply add it to the avoid list,
and then we migrate/move the VMs that were running there to a healthy host.

About the idea of being for KVM only; even though we might not be able to
run scripts on some hypervisors (Dom0 not being exposed), we could use
their API and check if the storage is responding. For instance, in XenServr
you could call “xe sr-scan uuid=” (or maybe other command
that uses storage), if there is any communication problem, it will report
an error. The same type of feature will probably be available on all other
hypervisors.

On Tue, Mar 14, 2017 at 1:46 PM, Jeromy Grimmett 
wrote:

> If all networking is lost, then obviously there is a bigger problem
> there.  The monitor is designed to do a true read/write test to the 
storage
> and report back a pass/fail.  Through this discussion, there was a ping
> suggestion, which I think we will include.
>
> The way this came about was that one of our hosts had a problem with a
> single primary storage, but all other hosts were 100% good across that
> storage and all others.  That troubled host was having problems with just
> the single storage device, but according to CloudStack, all was well and
> everything was good.  The way I am looking at this, is that what we are
> attempting to do is a much better and far more accurate test of storage
> availability compared to how Cloudstack currently does it based our
> experience.
>
> Make more sense?
>
> j
>
> Jeromy Grimmett
> P: 603.766.3625
> jer...@cloudbrix.com
> www.cloudbrix.com
>
>
> -Original Message-
> From: Simon Weller [mailto:swel...@ena.com]
> Sent: Tuesday, March 14, 2017 10:01 AM
> To: dev@cloudstack.apache.org
> Cc: Alex Bonilla 
> Subject: Re: [Proposal] - StorageHA
>
> So a few questions come to mind here.
>
>
> So if all networking is lost, how are you going to being able reliably
> fence the VM on the hosts?
>
> Are you assuming you still have out of band IPMI connectivity?
>
> If you're running bonded interfaces to different switches, what scenario
> would occur where the host loses network connectivity?
>
>
> - Si
>
> 
> From: Tutkowski, Mike 
> Sent: Tuesday, March 14, 2017 8:25 AM
> To: dev@cloudstack.apache.org
> Cc: Alex Bonilla
> Subject: Re: [Proposal] - StorageHA
>
> Thanks for your clarification. I see now. You were referring to a
> networking problem where one host could not see the storage (but the
> storage was still up and running).
>
> On 3/13/17, 10:31 

Re: :[VOTE] Apache Cloudstack 4.10.0.0

[Koushik Das]

Looks like there is already a PR for the same issue 
https://github.com/apache/cloudstack/pull/1982 from Kishan.

-Koushik

On 03/03/17, 1:58 PM, "Rohit Yadav"  wrote:

-1 (binding)


All, I've found an upgrade blocker. Pre 4.6 users are required to seed 4.6 
systemvmtemplate to proceed with the upgrade otherwise upgrade fails, and from 
4.9 upgrade to 4.10 does no check/enforcement that 4.10 based systemvmtemplate 
has been seeded/registered, nor the minimum required systemvmtemplate version 
is changed from 4.6.0 to 4.10.0.


After we have merged the strongswan/java8 PR, I had updated the upgrade 
docs on how to upgrade the systemvmtemplate here:


http://docs.cloudstack.apache.org/projects/cloudstack-release-notes/en/4.10/upgrade/upgrade-4.9.html


Using the above, I've tried to fix these issues here, please review and 
merge for RC2:

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


With above fix, the aim is 
that users only seed the 4.10 systemvmtemplate before upgrade and post-upgrade 
the upgrade paths fix the entries, global setting etc.


Regards.


From: Tutkowski, Mike 
Sent: 02 March 2017 22:39:08
To: dev@cloudstack.apache.org
Subject: Re: :[VOTE] Apache Cloudstack 4.10.0.0

I rolled back to my master branch at 
da66b06e7d562393da2e4b52206943f8bad49d10 and it works.

It appears something that went into after that commit has broken this. It 
looks like this SHA is about two weeks old and that 43 commits have gone into 
master since it.

On 3/2/17, 7:06 AM, "Tutkowski, Mike"  wrote:

According to where the code fails, though, it appears to be a 
networking problem. If I set a breakpoint before the failure and change a 
variable to say that security groups are not being used, then the VM starts.

I think this is a recently introduced problem because I have another 
branch based off of a slightly older version of master and it works fine here.

> On Mar 2, 2017, at 6:51 AM, Pierre-Luc Dion  
wrote:
>
> Hi Mike,
> Try vm with at least 512MB for memory.
>
>> On Mar 1, 2017 15:01, "Tutkowski, Mike"  
wrote:
>>
>> I see the following exception when trying to deploy a user VM in a 
Basic
>> Zone with two XenServer 6.5 hosts in one cluster. My system VMs have 
all
>> deployed properly. The user template gets downloaded fine. I can see 
the
>> user VM begin to start on a XenServer host, then it goes away. We 
then
>> automatically try on the other host. I can see the VM begin to start 
there
>> for a moment, then it goes away.
>>
>> I am just deploying the user VM’s template and root disk to NFS (same
>> place where the template and root disks of my system VMs are).
>>
>> I am using the built-in XenServer CentOS 5.6 (64 bit) template with 1
>> vCPU, 500 MHz, and 256 MB memory.
>>
>> WARN  [c.c.a.r.v.VirtualRoutingResource] (DirectAgent-7:ctx-35aded78)
>> (logid:aab9c320) Expected 1 answers while executing VmDataCommand but
>> received 2
>> WARN  [c.c.v.VirtualMachinePowerStateSyncImpl] 
(DirectAgentCronJob-14:ctx-27fb1ac3)
>> (logid:2c342f23) VM state was updated but update time is null?! vm 
id: 6
>> INFO  [o.a.c.f.j.i.AsyncJobManagerImpl] 
(AsyncJobMgr-Heartbeat-1:ctx-2c7d2dce)
>> (logid:a56a9a8c) Begin cleanup expired async-jobs
>> INFO  [o.a.c.f.j.i.AsyncJobManagerImpl] 
(AsyncJobMgr-Heartbeat-1:ctx-2c7d2dce)
>> (logid:a56a9a8c) End cleanup expired async-jobs
>> INFO  [c.c.u.AccountManagerImpl] (AccountChecker-1:ctx-383a632c)
>> (logid:541e9ba5) Found 0 removed accounts to cleanup
>> INFO  [c.c.u.AccountManagerImpl] (AccountChecker-1:ctx-383a632c)
>> (logid:541e9ba5) Found 0 disabled accounts to cleanup
>> INFO  [c.c.u.AccountManagerImpl] (AccountChecker-1:ctx-383a632c)
>> (logid:541e9ba5) Found 0 inactive domains to cleanup
>> INFO  [c.c.u.AccountManagerImpl] (AccountChecker-1:ctx-383a632c)
>> (logid:541e9ba5) Found 0 disabled projects to cleanup
>> WARN  [c.c.h.x.r.CitrixResourceBase] (DirectAgent-16:ctx-7c901443)
>> (logid:aab9c320) callHostPlugin failed for cmd: 
default_network_rules with
>> args secIps: 0:, vmName: i-2-6-VM, vmID: 6, vmIP: 10.117.40.53, 
vmMAC:
>> 06:b2:f4:00:00:22,  due to There was a failure communicating with the
>> plugin.
>> WARN  [c.c.h.x.r.w.x.CitrixStartCommandWrapper]
>> (DirectAgent-16:ctx-7c901443) (logid:aab9c320) Catch Exception: class
>> 

[GitHub] cloudstack issue #1865: CLOUDSTACK-9705: Unauthenticated API allows Admin pa...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1865
  
Code changes LGTM


---
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 #1865: CLOUDSTACK-9705: Unauthenticated API allows A...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1865#discussion_r103875741
  
--- Diff: server/src/com/cloud/api/ApiServer.java ---
@@ -430,8 +433,27 @@ public void handle(final HttpRequest request, final 
HttpResponse response, final
 if (!(responseType.equals(HttpUtils.RESPONSE_TYPE_JSON) || 
responseType.equals(HttpUtils.RESPONSE_TYPE_XML))) {
 responseType = HttpUtils.RESPONSE_TYPE_XML;
 }
-
 try {
+//verify that parameter is legit for passing via admin port
--- End diff --

Check if it makes sense to move this as a separate helper method. There are 
also other places in code that reads the annotation on the API commands and 
parameters. Check if some of them can be reused.


---
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 issue #1865: CLOUDSTACK-9705: Unauthenticated API allows Admin pa...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1865
  
@anshul1886 @karuturi Should this be treated as a security issue and fixed 
on priority?


---
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 issue #1980: CLOUDSTACK-9805: Display VR list in network details

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1980
  
Is it only showing VR or other appliances providing service in that 
network? If it is the latter then the tab name is appropriate otherwise it is 
better to name it something like "Virtual Routers" to avoid confusion.
Verify that it is working as expected for RVR, VPC, basic zone network and 
other possible scenarios.


---
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 issue #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1278
  
I think disabled resource related checks are only applicable for regular 
users, root/system users can go ahead and deploy VMs in disabled resources as 
well.


---
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 issue #1960: [4.11/Future] CLOUDSTACK-9782: Host HA and KVM HA pr...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1960
  
@abhinandanprateek If you refer to the discussion on dev@ 
https://goo.gl/cU8RuX, @rhtyd proposed it as a generic HA framework for any 
resources (and not limited to VM). Now if it just a replacement of the existing 
VM-HA framework then we need to discuss the benefits it provides in more detail 
compared to the existing one. I have already made some specific comments on the 
justifications provided in favour of a new framework.


---
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 issue #1960: [4.11/Future] CLOUDSTACK-9782: Host HA and KVM HA pr...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1960
  
I have already raised some questions on dev@ on the need for a new HA 
framework when the existing HA framework can do all the things mentioned. The 
new framework only supports VM HA. If we see some concrete implementation of 
network/storage or any other type of resource HA which doesn't currently exist 
using the new framework then it would be much more easier to see the value add. 
I think more discussion is needed on this.


---
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 #:

[koushik-das]

Github user koushik-das commented on the pull request:


https://github.com/apache/cloudstack/commit/fa85151be962824cc88776a0264e1ac6ef90560c#commitcomment-21082422
  
In server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java:
In server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java on line 295:
Thats correct, already submitted #1975 


---
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 issue #1975: Fix build failure on master

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1975
  
ping @karuturi


---
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 #1975: Fix build failure on master

[koushik-das]

GitHub user koushik-das opened a pull request:

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

Fix build failure on master



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

$ git pull https://github.com/Accelerite/cloudstack build-failure

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

https://github.com/apache/cloudstack/pull/1975.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 #1975


commit f843fcb969780e1368a060e26fe229f08a4ec92c
Author: Koushik Das <kous...@apache.org>
Date:   2017-02-28T05:52:30Z

Fix build failure on master




---
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 issue #1886: CLOUDSTACK-9728: Fixed traffic sentinel HTTP 414 err...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1886
  
Code changes LGTM


---
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 issue #1825: CLOUDSTACK-9660: NPE while destroying volumes during...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1825
  
tag:mergeready


---
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 issue #1968: CLOUDSTACK-9666 Added basic configuration validation...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1968
  
Code changes LGTM


---
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 issue #1967: CLOUDSTACK-9638 Problems caused when inputting doubl...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1967
  
Code change LGTM


---
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 issue #1964: Bug-Id: CLOUDSTACK-9800 Enabled netscaler inline mod...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1964
  
Code changes LGTM


---
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 issue #1825: CLOUDSTACK-9660: NPE while destroying volumes during...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1825
  
@karuturi This can be merged


---
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 #1941: CLOUDSTACK-8663: Fixed various issues to allo...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1941#discussion_r102698293
  
--- Diff: 
plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java
 ---
@@ -1415,6 +1417,10 @@ public VM createWorkingVM(final Connection conn, 
final String vmName, final Stri
 vbdr.userdevice = "autodetect";
 vbdr.mode = Types.VbdMode.RW;
 vbdr.type = Types.VbdType.DISK;
+Long deviceId = volumeTO.getDeviceId();
+if (deviceId != null && (!isDeviceUsed(conn, vm, deviceId) || 
deviceId > 3)) {
--- End diff --

I remember seeing these changes in another PR, check if that is merged or 
not


---
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 #1771: CLOUDSTACK-9611: Dedicating a Guest VLAN rang...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1771#discussion_r102445777
  
--- Diff: server/src/com/cloud/network/NetworkServiceImpl.java ---
@@ -3085,9 +3085,10 @@ public GuestVlan 
dedicateGuestVlanRange(DedicateGuestVlanRangeCmd cmd) {
 // Verify account is valid
 Account vlanOwner = null;
 if (projectId != null) {
-if (accountName != null) {
-throw new InvalidParameterValueException("accountName and 
projectId are mutually exclusive");
-}
+//accountName and projectId are mutually exclusive
--- End diff --

@nitin-maharana @ustcweizhou The scope field in UI should be fine. But 
since there is a breaking change (accountName is made optional from required), 
there may be scenarios which will break. As long as it is documented should be 
ok.


---
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 issue #1951: CLOUDSTACK-9792: Add upgrade path for 4.9.3.0

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1951
  
I already see schema-4920to41000.sql and the corresponding cleanup file in 
master. So how will this fit in the overall scheme? Is there any plans for 
4.9.3?


---
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 issue #1773: CLOUDSTACK-9607: Preventing template deletion when t...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1773
  
I think changing the default API behaviour for fixing a bug should be ok as 
long as documented properly. Also in this case the deletion criteria is made 
more strict, so there is no unwanted data loss.


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

Re: [DISCUSS][FS] Host HA for CloudStack

[Koushik Das]

See inline.

Thanks,
Koushik

On 21/02/17, 11:47 AM, "Rohit Yadav" <rohit.ya...@shapeblue.com> wrote:

Hi Koushik,


Thanks for sharing your comments and questions.


1. Yes, the FS is divided into two parts - a general HA framework which makes 
no assumption about the type of resource and HA provider implementation that 
works on a type of resource/hypervisor/storage etc.

[Koushik] Hmm the heading is misleading then. I would like to see the details 
of the generic HA framework that you are proposing for any resource type. What 
all resource types can/need to be HA’ed? Also I would like to see a clear 
definition of “storage HA”, ”network HA” or “any resource HA” etc. before going 
ahead with this generic framework. If this new framework ends up doing only 
doing Host/VM HA then there is no point doing all this.

Specifically, with this feature we want to solve the problem of HA-ing a host 
reliably and use out-of-band management subsystem (i.e. ipmi based 
status/reboot/power-off to investigate/recover/fence the host) in the HA 
provider implementation. Yes, a host HA should trigger VM HA, i.e. for the host 
being fenced move HA VMs to other hosts. This also reliably solves the issue of 
disk corruption when same HA VMs get started on multiple hosts.

[Koushik] If host HA implies doing HA on all VMs running in a host, I am not 
clear as to why host HA is needed separately when there is already VM HA 
available.

2. The old VM HA implementation makes a lot of assumptions about the type 
of resource (i.e. VM) it is HA-ing, it is tied to VM HA which is why HA for 
host could not be added in a straight forward way without regressions we could 
not test. With this new HA framework, it does not make any assumption around 
type of the resource and separates policy from mechanism, we also want to add 
deterministic tests (using marvin tests and a simulator based ha provider 
implementation) to demonstrate the generic HA functionality. In future with 
this framework, HA for various resources such as VM, storage, network can be 
added. As a first step we want to get the framework in, and support for Host as 
a resource type. We also want to reduce assumptions, or dependency as both VM 
HA and Host HA are related (sequence etc). The HAProvider interface would be 
something every hypervisor can implement.

[Koushik] Again please justify why host HA is needed when VM HA is already 
there? If the question is about ease of writing automated tests, I have already 
written simulator based tests for the existing VM HA. Please refer 
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Writing+tests+leveraging+the+simulator+enhancements
 for the test details.

3. While an existing (VM) HA framework exists, it was safer to write new 
code and demonstrate it works for any general HA resource than refactor and 
implement this in the old framework which could introduce serious regressions 
leading to production issues. For the most part, we've avoided to alter 
anything in the old HA framework while making sure that old (VM) HA works well 
with the new HA framework. The JIRA issue for the feature is in the FS.

[Koushik] As mentioned in a previous comment, please define what all resources 
need to be HA’d and why is it needed? For e.g. there is RVR which provides HA 
for the network services provided by VR. Also for other network plugins there 
may be native ways for achieving HA and may not need anything from CS 
perspective. I wanted to make sure that all these points are accounted for 
before we proceed with a generic framework.


4. Any HA operation can be blocking in nature, one of the things included 
is a background polling manager that polls for changes, and a task/activity 
executor as out-of-band operations can take time. Therefore, all the 
health/activity/fencing/recovery operations have some timeout, limits and 
specific queues. The existing framework does not provide any abstraction to 
queue, restrict operation timeout, and tie them against a FSM. The existing 
framework also is hard to test, specifically to validate using integration 
test. We also wanted to avoid adding any regressions to existing/old VM HA. 
Lastly, the primary use of IPMI/out-of-band management in performing host-ha is 
not for investigation but for recovery (try a reboot), and fencing (power off).

[Koushik] A lot of points you have raised here is not correct. There is already 
polling of all the hosts to find out VM state changes, queues, time-outs in 
place to send commands to hypervisors etc. Have you evaluated the option of 
using IPMI in the existing KVM HA plugins?

 

Hope this answers your questions, please feel free add more comments and 
questions. Thanks.


Regards.


____
    From: Koushik Das <koushik@accelerite.com>
Sent: 20 February 2017 11:45
To: dev@cloudstack.apache.org
   

Re: [DISCUSS][FS] Host HA for CloudStack

[Koushik Das]

Rohit,

Thanks for the effort you have put in writing the FS. I have some questions 
based on my initial reading of the FS.

1. “Host HA” – In the FS you are talking about a generic HA framework but it is 
not clear what is meaning of “host HA”. Is it something like all or some VMs 
running on a host will be started on another host(s) in case of a failure or is 
it something else? How is it different from the existing “VM HA” that is 
already there?
2. You have mentioned that “Cloudstack lacks a way to reliably fence host”. 
Cloudstack considers VM as a 1st class object and so provides fencing for VM 
instead of host. There are hypervisor specific plugins that implement mechanism 
to fence a VM. I am not sure if it makes sense to expose host fencing as end 
user doesn’t care about it. Now the VM fencing implementation can use something 
like “host fencing” internally.
3. There is an existing HA framework which provides plugins for doing 
investigation if a VM is alive or not, host is alive or not, fencing of VM in 
case it is not alive. It will be good to understand the limitations of the 
existing framework and how the new framework helps in solving these problems. 
We also need to understand if the limitation is in the framework or some 
specific plugin implementation that is causing issues. Reference to JIRA issues 
would help.
4. You have mentioned about ipmi to investigate host failure. I would like to 
understand why same can’t be used in the existing framework.

Thanks,
Koushik

On 16/02/17, 4:48 PM, "Rohit Yadav"  wrote:

All,


I would like to start discussion on a new feature - Host HA for CloudStack.

CloudStack lacks a way to reliably fence a host, the idea of the host-ha 
feature is to provide a general purpose HA framework and HA provider 
implementation specific for hypervisor that can use additional mechanism such 
as OOBM (ipmi based power management) to reliably investigate, recover and 
fence a host. This feature can handle scenarios associated with server crash 
issues and reliable fencing of hosts and HA of VM. The first version will have 
HA provider implementation for KVM (and for simulator to test the framework 
implementation, and write marvin tests that can validate the feature on Travis 
and others).


Please have a look at the FS here:

https://cwiki.apache.org/confluence/display/CLOUDSTACK/Host+HA


Looking forward to your comments and questions.


Regards.

rohit.ya...@shapeblue.com 
www.shapeblue.com
53 Chandos Place, Covent Garden, London  WC2N 4HSUK
@shapeblue
  
 






DISCLAIMER
==
This e-mail may contain privileged and confidential information which is the 
property of Accelerite, a Persistent Systems business. It is intended only for 
the use of the individual or entity to which it is addressed. If you are not 
the intended recipient, you are not authorized to read, retain, copy, print, 
distribute or use this message. If you have received this communication in 
error, please notify the sender and delete all copies of this message. 
Accelerite, a Persistent Systems business does not accept any liability for 
virus infected mails.

[GitHub] cloudstack issue #1838: CLOUDSTACK-9682: Block VM migration to a storage whi...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1838
  
Code changes LGTM
@karuturi This can be merged


---
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 #1771: CLOUDSTACK-9611: Dedicating a Guest VLAN rang...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1771#discussion_r101486770
  
--- Diff: server/src/com/cloud/network/NetworkServiceImpl.java ---
@@ -3085,9 +3085,10 @@ public GuestVlan 
dedicateGuestVlanRange(DedicateGuestVlanRangeCmd cmd) {
 // Verify account is valid
 Account vlanOwner = null;
 if (projectId != null) {
-if (accountName != null) {
-throw new InvalidParameterValueException("accountName and 
projectId are mutually exclusive");
-}
+//accountName and projectId are mutually exclusive
--- End diff --

@nitin-maharana So as I understand in order to avoid making a breaking API 
change you are simply ignoring account and giving priority to project if both 
are specified.


---
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 issue #1829: CLOUDSTACK-9363: Fix HVM VM restart bug in XenServer

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1829
  
@borisstoyanov Sorry don't have a windows template


---
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 issue #1829: CLOUDSTACK-9363: Fix HVM VM restart bug in XenServer

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1829
  
@borisstoyanov try using a Windows template


---
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 issue #1834: CLOUDSTACK-9679:Allow master user to manage subordin...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1834
  
Code changes LGTM


---
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 issue #1758: CLOUDSTACK-9588: Add Load Balancer functionality in ...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1758
  
@karuturi This is ready for merge. UI change only, 2 LGTMs, UI screenshot


---
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 #1771: CLOUDSTACK-9611: Dedicating a Guest VLAN rang...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1771#discussion_r101451791
  
--- Diff: server/src/com/cloud/network/NetworkServiceImpl.java ---
@@ -3085,9 +3085,10 @@ public GuestVlan 
dedicateGuestVlanRange(DedicateGuestVlanRangeCmd cmd) {
 // Verify account is valid
 Account vlanOwner = null;
 if (projectId != null) {
-if (accountName != null) {
-throw new InvalidParameterValueException("accountName and 
projectId are mutually exclusive");
-}
+//accountName and projectId are mutually exclusive
--- End diff --

@nitin-maharana If account and project are mutually exclusive why have you 
removed the validation?


---
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 issue #1786: CLOUDSTACK-9618: Load Balancer configuration page do...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1786
  
LGTM based on code review
@karuturi Since this is a network plugin related fix and requires 
Netscaler, I don't think there is a need to run the BVTs.


---
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 issue #1792: CLOUDSTACK-9623: Deploying virtual machine fails due...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1792
  
Code changes LGTM
@nitin-maharana Can you check on the travis failure?


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

Re: Attaching more than 14 data volumes to an instance

[Koushik Das]

The hardcoded value of 15 needs to be fixed, it can be replaced with 
getMaxDataVolumesSupported() just above it. Please file a bug and if possible 
raise a PR as well.

-Koushik

On 15/02/17, 11:21 PM, "Voloshanenko Igor"  wrote:

On VM we try to emulate real hardware )))
So any device honor specification
In this case PCI :)

To be honest we can increase limits by adding multifunctional devices or
migrate to virtio-iscsi-blk

But as for me - 14 disks more than enough now


About 3 for cdrom. I will check . I think CDROM emulated as IDE device, not
via virtio-blk

For 0 - root volume , interesting , in this case we can easily add 1 more
DATA disk :)

ср, 15 февр. 2017 г. в 19:24, Rafael Weingärtner <
rafaelweingart...@gmail.com>:

> I thought that on a VM we would not be bound by PCI limitations.
> Interesting explanations, thanks.
>
>
> On Wed, Feb 15, 2017 at 12:19 PM, Voloshanenko Igor <
> igor.voloshane...@gmail.com> wrote:
>
> > I think explanation very easy.
> > PCI itself can handle up to 32 devices.
> >
> > If you run lspci inside empty (fresh created) VM - you will see, that 8
> > slots already occupied
> > [root@test-virtio-blk ~]# lspci
> > 00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev
> > 02)
> > 00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton
> II]
> > 00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE 
[Natoma/Triton
> > II]
> > 00:01.2 USB controller: Intel Corporation 82371SB PIIX3 USB
> [Natoma/Triton
> > II] (rev 01)
> > 00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)
> > 00:02.0 VGA compatible controller: Cirrus Logic GD 5446
> > 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
> > 00:04.0 SCSI storage controller: Red Hat, Inc Virtio block device
> >
> > [root@test-virtio-blk ~]# lspci | wc -l
> > 8
> >
> > So, 7 system devices + 1 ROOT disk
> >
> > in current implementation, we used virtio-blk, which can handle only 1
> > device per instance.
> >
> > So, we have 32-8 == 24 free slots...
> >
> > As Cloudstack support more than 1 eth cards - 8 of them reserved for
> > network cards and 16 available for virtio-blk
> >
> > So, practical limit equal 16 devices (for DATA disks)
> >
> > Why 2 devices (0 + 3) excluded - interesting question... I will try to
> > research and post explanation
> >
> >
> >
> >
> >
> >
> >
> >
> > 2017-02-15 18:27 GMT+02:00 Rafael Weingärtner <
> rafaelweingart...@gmail.com
> > >:
> >
> > > I hate to say this, but probably no one knows why.
> > > I looked at the history and this method has always being like this.
> > >
> > > The device ID 3 seems to be something reserved, probably for Xen tools
> > (big
> > > guess here)?
> > >
> > > Also, regarding the limit; I could speculate two explanations for the
> > > limit. A developer did not get the full specs and decided to do
> whatever
> > > he/she wanted. Or, maybe, at the time of coding (long, long time ago)
> > there
> > > was a hypervisor that limited (maybe still limits) the number of
> devices
> > > that could be plugged to a VM and the first developers decided to 
level
> > > everything by that spec.
> > >
> > > It may be worth checking with KVM, XenServer, Hyper-V, and VMware if
> they
> > > have such limitation on disks that can be attached to a VM. If they do
> > not
> > > have, we could remove that, or at least externalize the limit in a
> > > parameter.
> > >
> > > On Wed, Feb 15, 2017 at 5:54 AM, Friðvin Logi Oddbjörnsson <
> > > frid...@greenqloud.com> wrote:
> > >
> > > > CloudStack is currently limiting the number of data volumes, that 
can
> > be
> > > > attached to an instance, to 14.
> > > > More specifically, this limitation relates to the device ids that
> > > > CloudStack considers valid for data volumes.
> > > > In method VolumeApiServiceImpl.getDeviceId(long, Long), only device
> > ids
> > > 1,
> > > > 2, and 4-15 are considered valid.
> > > > What I would like to know is: is there a reason for this limitation?
> > (of
> > > > not going higher than device id 15)
> > > >
> > > > Note that the current number of attached data volumes is already
> being
> > > > checked against the maximum number of data volumes per instance, as
> > > > specified by the relevant hypervisor’s capabilities.
> > > > E.g. if the relevant hypervisor’s capabilities specify that it only
> > > > supports 6 data volumes per instance, CloudStack rejects attaching a
> > > > seventh data volume.
> > > >
> > > >
> > > > 

[GitHub] cloudstack issue #1889: CLOUDSTACK-9718: Revamp the dropdown showing lists o...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1889
  
LGTM based on code review


---
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 issue #1885: CLOUDSTACK-9724: Fixed missing additional public ip ...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1885
  
Code changes LGTM


---
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 #1885: CLOUDSTACK-9724: Fixed missing additional pub...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1885#discussion_r100767709
  
--- Diff: server/src/com/cloud/network/IpAddressManagerImpl.java ---
@@ -460,6 +460,12 @@ boolean checkIfIpAssocRequired(Network network, 
boolean postApplyRules, List 0) {
+if (network.getVpcId() != null) {
--- End diff --

@jayapalu Please improve the code comment


---
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 #1935: CLOUDSTACK-9764: Delete domain failure due to...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r100478202
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean 
cleanup) {
 
 @Override
 public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
-// mark domain as inactive
-s_logger.debug("Marking domain id=" + domain.getId() + " as " + 
Domain.State.Inactive + " before actually deleting it");
-domain.setState(Domain.State.Inactive);
-_domainDao.update(domain.getId(), domain);
-boolean rollBackState = false;
-boolean hasDedicatedResources = false;
+GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+if (lock == null) {
+s_logger.debug("Couldn't get the global lock");
+return false;
+}
+
+if (!lock.lock(30)) {
+s_logger.debug("Couldn't lock the db");
+return false;
+}
 
 try {
-long ownerId = domain.getAccountId();
-if ((cleanup != null) && cleanup.booleanValue()) {
-if (!cleanupDomain(domain.getId(), ownerId)) {
-rollBackState = true;
-CloudRuntimeException e =
-new CloudRuntimeException("Failed to clean up 
domain resources and sub domains, delete failed on domain " + domain.getName() 
+ " (id: " +
-domain.getId() + ").");
-e.addProxyObject(domain.getUuid(), "domainId");
-throw e;
-}
-} else {
-//don't delete the domain if there are accounts set for 
cleanup, or non-removed networks exist, or domain has dedicated resources
-List networkIds = 
_networkDomainDao.listNetworkIdsByDomain(domain.getId());
-List accountsForCleanup = 
_accountDao.findCleanupsForRemovedAccounts(domain.getId());
-List dedicatedResources = 
_dedicatedDao.listByDomainId(domain.getId());
-if (dedicatedResources != null && 
!dedicatedResources.isEmpty()) {
-s_logger.error("There are dedicated resources for the 
domain " + domain.getId());
-hasDedicatedResources = true;
-}
-if (accountsForCleanup.isEmpty() && networkIds.isEmpty() 
&& !hasDedicatedResources) {
-_messageBus.publish(_name, 
MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
-if (!_domainDao.remove(domain.getId())) {
+// mark domain as inactive
+s_logger.debug("Marking domain id=" + domain.getId() + " as " 
+ Domain.State.Inactive + " before actually deleting it");
+domain.setState(Domain.State.Inactive);
+_domainDao.update(domain.getId(), domain);
+boolean rollBackState = false;
+boolean hasDedicatedResources = false;
+
+try {
+long ownerId = domain.getAccountId();
+if ((cleanup != null) && cleanup.booleanValue()) {
+if (!cleanupDomain(domain.getId(), ownerId)) {
 rollBackState = true;
 CloudRuntimeException e =
-new CloudRuntimeException("Delete failed on 
domain " + domain.getName() + " (id: " + domain.getId() +
-"); Please make sure all users and sub 
domains have been removed from the domain before deleting");
+new CloudRuntimeException("Failed to clean up 
domain resources and sub domains, delete failed on domain " + domain.getName() 
+ " (id: " +
+domain.getId() + ").");
 e.addProxyObject(domain.getUuid(), "domainId");
 throw e;
 }
-_messageBus.publish(_name, 
MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
 } else {
-rollBackState = true;
-String msg = null;
-if (!accountsForCleanup.isEmpty()) {
-msg = accountsForCleanup.size() + " accounts to 
cleanup";
-} else if (!networkIds.isEmpty()) {
-msg = networkIds

[GitHub] cloudstack pull request #1935: CLOUDSTACK-9764: Delete domain failure due to...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1935#discussion_r100477949
  
--- Diff: server/src/com/cloud/user/DomainManagerImpl.java ---
@@ -273,79 +274,97 @@ public boolean deleteDomain(long domainId, Boolean 
cleanup) {
 
 @Override
 public boolean deleteDomain(DomainVO domain, Boolean cleanup) {
-// mark domain as inactive
-s_logger.debug("Marking domain id=" + domain.getId() + " as " + 
Domain.State.Inactive + " before actually deleting it");
-domain.setState(Domain.State.Inactive);
-_domainDao.update(domain.getId(), domain);
-boolean rollBackState = false;
-boolean hasDedicatedResources = false;
+GlobalLock lock = GlobalLock.getInternLock("AccountCleanup");
+if (lock == null) {
+s_logger.debug("Couldn't get the global lock");
+return false;
+}
+
+if (!lock.lock(30)) {
+s_logger.debug("Couldn't lock the db");
+return false;
+}
 
 try {
-long ownerId = domain.getAccountId();
-if ((cleanup != null) && cleanup.booleanValue()) {
-if (!cleanupDomain(domain.getId(), ownerId)) {
-rollBackState = true;
-CloudRuntimeException e =
-new CloudRuntimeException("Failed to clean up 
domain resources and sub domains, delete failed on domain " + domain.getName() 
+ " (id: " +
-domain.getId() + ").");
-e.addProxyObject(domain.getUuid(), "domainId");
-throw e;
-}
-} else {
-//don't delete the domain if there are accounts set for 
cleanup, or non-removed networks exist, or domain has dedicated resources
-List networkIds = 
_networkDomainDao.listNetworkIdsByDomain(domain.getId());
-List accountsForCleanup = 
_accountDao.findCleanupsForRemovedAccounts(domain.getId());
-List dedicatedResources = 
_dedicatedDao.listByDomainId(domain.getId());
-if (dedicatedResources != null && 
!dedicatedResources.isEmpty()) {
-s_logger.error("There are dedicated resources for the 
domain " + domain.getId());
-hasDedicatedResources = true;
-}
-if (accountsForCleanup.isEmpty() && networkIds.isEmpty() 
&& !hasDedicatedResources) {
-_messageBus.publish(_name, 
MESSAGE_PRE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
-if (!_domainDao.remove(domain.getId())) {
+// mark domain as inactive
+s_logger.debug("Marking domain id=" + domain.getId() + " as " 
+ Domain.State.Inactive + " before actually deleting it");
+domain.setState(Domain.State.Inactive);
+_domainDao.update(domain.getId(), domain);
+boolean rollBackState = false;
+boolean hasDedicatedResources = false;
+
+try {
+long ownerId = domain.getAccountId();
+if ((cleanup != null) && cleanup.booleanValue()) {
+if (!cleanupDomain(domain.getId(), ownerId)) {
 rollBackState = true;
 CloudRuntimeException e =
-new CloudRuntimeException("Delete failed on 
domain " + domain.getName() + " (id: " + domain.getId() +
-"); Please make sure all users and sub 
domains have been removed from the domain before deleting");
+new CloudRuntimeException("Failed to clean up 
domain resources and sub domains, delete failed on domain " + domain.getName() 
+ " (id: " +
+domain.getId() + ").");
 e.addProxyObject(domain.getUuid(), "domainId");
 throw e;
 }
-_messageBus.publish(_name, 
MESSAGE_REMOVE_DOMAIN_EVENT, PublishScope.LOCAL, domain);
 } else {
-rollBackState = true;
-String msg = null;
-if (!accountsForCleanup.isEmpty()) {
-msg = accountsForCleanup.size() + " accounts to 
cleanup";
-} else if (!networkIds.isEmpty()) {
-msg = networkIds

[GitHub] cloudstack issue #1936: [CLOUDSTACK-9773] API: don't break API output with n...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1936
  
Code changes LGTM


---
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 #1906: CLOUDSTACK-9743 - ODL plugin responds to dele...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1906#discussion_r100281158
  
--- Diff: 
plugins/network-elements/opendaylight/src/main/java/org/apache/cloudstack/network/opendaylight/OpendaylightElement.java
 ---
@@ -156,7 +153,7 @@ public HostVO createHostVOForDirectConnectAgent(HostVO 
host, StartupCommand[] st
 
 @Override
 public DeleteHostAnswer deleteHost(HostVO host, boolean isForced, 
boolean isForceDeleteStorage) throws UnableDeleteHostException {
-return new DeleteHostAnswer(true);
+return null;
--- End diff --

@syed I am not sure if this is used or maintained by anyone. But the right 
fix may be to use some checks to return a valid answer or null. How is this 
handled in some of the other network elements?


---
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 issue #1930: CLOUDSTACK-9687: if the allocated amount is 0 the ca...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1930
  
Code changes LGTM


---
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 issue #1933: CLOUDSTACK-9569: add router.aggregation.command.each...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1933
  
@abhinandanprateek Looks like there is another PR #1856 related to this.


---
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 #1727: CLOUDSTACK-9539: Support changing Service off...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1727#discussion_r100267498
  
--- Diff: setup/db/db/schema-490to4910.sql ---
@@ -71,3 +71,17 @@ INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` 
(uuid,hypervisor_type, hypervis
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(),'VMware', '6.0', 'sles11_64Guest', 187, utc_timestamp(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(),'VMware', '6.0', 'sles11Guest', 188, utc_timestamp(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(),'VMware', '6.0', 'sles12_64Guest', 244, utc_timestamp(), 0);
+
+-- Add service_offering_id column to vm_snapshots table
--- End diff --

@nvazquez This should be either in schema-4920to41000.sql or 
schema-4920to4930.sql if there is a plan for 4.9.3. 490to4910 is not the 
correct one to put these schema changes.


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

Re: re-introduction

[Koushik Das]

Welcome back Daan!

On 01/02/17, 1:56 PM, "Daan Hoogland"  wrote:

Hello,


My name is Daan Hoogland. I've been mostly out of the community since May 
last year. I am now back through the generous sponsorship of my new employer 
and will be working (mostly) as developer on cloudstack.

For those who remember me and are curious, I've been learning some scala 
and some rust in the meanwhile and have been working on financial middleware in 
between.


I expect to have good times back in here :)

daan.hoogl...@shapeblue.com 
www.shapeblue.com
53 Chandos Place, Covent Garden, Utrecht Utrecht 3531 VENetherlands
@shapeblue
  
 






DISCLAIMER
==
This e-mail may contain privileged and confidential information which is the 
property of Accelerite, a Persistent Systems business. It is intended only for 
the use of the individual or entity to which it is addressed. If you are not 
the intended recipient, you are not authorized to read, retain, copy, print, 
distribute or use this message. If you have received this communication in 
error, please notify the sender and delete all copies of this message. 
Accelerite, a Persistent Systems business does not accept any liability for 
virus infected mails.

Re: Delete Volumes

[Koushik Das]

Looks like a bug to me. Please file one in jira and if possible submit a PR to 
fix it.

Thanks,
Koushik

On 13/01/17, 5:53 PM, "Wiesener, Erik"  wrote:

Hello CloudStack development team,

It seems not very logical that the action : “delete Volume” is a 
synchronous job. This may cause timeout problems. Is there any explanation for 
this?

Thank you in advance
Yours sinclerey

Erik Wiesener, independent Developer





DISCLAIMER
==
This e-mail may contain privileged and confidential information which is the 
property of Accelerite, a Persistent Systems business. It is intended only for 
the use of the individual or entity to which it is addressed. If you are not 
the intended recipient, you are not authorized to read, retain, copy, print, 
distribute or use this message. If you have received this communication in 
error, please notify the sender and delete all copies of this message. 
Accelerite, a Persistent Systems business does not accept any liability for 
virus infected mails.

[GitHub] cloudstack issue #1819: CLOUDSTACK-9653 The system capacity was not getting ...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1819
  
Code changes LGTM


---
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 #1889: CLOUDSTACK-9718: Revamp the dropdown showing ...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1889#discussion_r95973044
  
--- Diff: 
api/src/org/apache/cloudstack/api/command/admin/host/ListHostsCmd.java ---
@@ -206,7 +206,7 @@ public void execute() {
 } else {
 Pair<List, Integer> result;
 Ternary<Pair<List, Integer>, List, Map<Host, Boolean>> hostsForMigration =
-_mgr.listHostsForMigrationOfVM(getVirtualMachineId(), 
this.getStartIndex(), this.getPageSizeVal());
+_mgr.listHostsForMigrationOfVM(getVirtualMachineId(), 
this.getStartIndex(), this.getPageSizeVal(), null);
--- End diff --

Why keyword is not used here? All API commands derived from BaseListCmd 
should have keyword.


---
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 #1889: CLOUDSTACK-9718: Revamp the dropdown showing ...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1889#discussion_r95973392
  
--- Diff: ui/scripts/ui-custom/migrate.js ---
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
--- End diff --

For UI changes please provide UI screenshots


---
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 issue #1848: CLOUDSTACK-9693 Cluster View - Status symbol does no...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1848
  
@rashmidixit Please provide screenshots for UI changes (better if you also 
provide screenshot prior to the fix). Also for a cluster enabled/disabled and 
managed/unmanaged are different properties.


---
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 issue #1852: CLOUDSTACK-9695: VM snapshot is disabled if the VM I...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1852
  
@rashmidixit Code LGTM. Please provide screenshots of the changes.


---
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 issue #1899: CLOUDSTACK-9650: Allow starting VMs regardless of cp...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1899
  
@rhtyd If you had checked #1812 you wouldn't have asked these questions :)
This config was introduced in #1812 and the scope was incorrectly put as 
zone, the config is meant to be a global one.


---
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 issue #1892: CLOUDSTACK-9731: Hardcoded label appears on the Add ...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1892
  
@sureshanaparti please post a screen shot of the UI change


---
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 issue #1899: CLOUDSTACK-9650: Allow starting VMs regardless of cp...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1899
  
@rhtyd Tests are present in #1812


---
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 issue #1899: CLOUDSTACK-9650: Allow starting VMs regardless of cp...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1899
  
Merging this


---
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 #1727: CLOUDSTACK-9539: Support changing Service off...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1727#discussion_r95514039
  
--- Diff: server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java ---
@@ -350,6 +368,24 @@ public VMSnapshot allocVMSnapshot(Long vmId, String 
vsDisplayName, String vsDesc
 return null;
 }
 
+/**
+ * Add entries on vm_snapshot_details if service offering is dynamic. 
This will allow setting details when revert to vm snapshot
+ * @param vmId vm id
+ * @param serviceOfferingId service offering id
+ * @param vmSnapshotId vm snapshot id
+ */
+private void addSupportForCustomServiceOffering(long vmId, long 
serviceOfferingId, long vmSnapshotId) {
+ServiceOfferingVO serviceOfferingVO = 
_serviceOfferingDao.findById(serviceOfferingId);
+if (serviceOfferingVO.isDynamic()) {
+List vmDetails = 
_userVmDetailsDao.listDetails(vmId);
+for (UserVmDetailVO detail : vmDetails) {
+if(detail.getName().equalsIgnoreCase("cpuNumber") || 
detail.getName().equalsIgnoreCase("cpuSpeed") || 
detail.getName().equalsIgnoreCase("memory")) {
+_vmSnapshotDetailsDao.addDetail(vmSnapshotId, 
detail.getName(), detail.getValue(), true);
--- End diff --

Use saveDetails() instead of multiple addDetail() so that it is transaction 
consistent


---
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 #1727: CLOUDSTACK-9539: Support changing Service off...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1727#discussion_r95515374
  
--- Diff: server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java ---
@@ -338,10 +354,12 @@ public VMSnapshot allocVMSnapshot(Long vmId, String 
vsDisplayName, String vsDesc
 VMSnapshotVO vmSnapshotVo =
 new VMSnapshotVO(userVmVo.getAccountId(), 
userVmVo.getDomainId(), vmId, vsDescription, vmSnapshotName, vsDisplayName, 
userVmVo.getServiceOfferingId(),
 vmSnapshotType, null);
+
 VMSnapshot vmSnapshot = _vmSnapshotDao.persist(vmSnapshotVo);
 if (vmSnapshot == null) {
 throw new CloudRuntimeException("Failed to create snapshot 
for vm: " + vmId);
 }
+addSupportForCustomServiceOffering(userVmVo.getId(), 
userVmVo.getServiceOfferingId(), vmSnapshotVo.getId());
--- End diff --

snapshotDao.persist() and addSupportForCustomServiceOffering() should be 
wrapped in a transaction, check Transaction.execute()


---
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 #1727: CLOUDSTACK-9539: Support changing Service off...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1727#discussion_r95517419
  
--- Diff: server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java ---
@@ -707,16 +802,41 @@ private UserVm orchestrateRevertToVMSnapshot(Long 
vmSnapshotId) throws Insuffici
 throw new InvalidParameterValueException("There is other 
active vm snapshot tasks on the instance, please try again later");
 }
 
+revertUserVmDetailsFromVmSnapshot(userVm, vmSnapshotVo);
+
 try {
 VMSnapshotStrategy strategy = 
findVMSnapshotStrategy(vmSnapshotVo);
 strategy.revertVMSnapshot(vmSnapshotVo);
+updateUserVmServiceOffering(userVm, vmSnapshotVo);
--- End diff --

Service offering and details update should be done in a transaction once 
the snapshot revert completes successfully provided there is no agent call 
happening as part of _userVmManager.upgradeVirtualMachine()


---
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 #1727: CLOUDSTACK-9539: Support changing Service off...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1727#discussion_r95517522
  
--- Diff: server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java ---
@@ -643,6 +679,65 @@ else if (jobResult instanceof Throwable)
 }
 }
 
+/**
+ * If snapshot was taken with a different service offering than actual 
used in vm, should change it back to it
+ * @param userVm vm to change service offering (if necessary)
+ * @param vmSnapshotVo vm snapshot
+ */
+private void updateUserVmServiceOffering(UserVm userVm, VMSnapshotVO 
vmSnapshotVo) {
+if (vmSnapshotVo.getServiceOfferingId() != 
userVm.getServiceOfferingId()) {
+changeUserVmServiceOffering(userVm, vmSnapshotVo);
+}
+}
+
+/**
+ * Get user vm details as a map
+ * @param userVm user vm
+ * @return map
+ */
+private Map<String, String> getVmMapDetails(UserVm userVm) {
--- End diff --

Is this used anywhere?


---
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 #1727: CLOUDSTACK-9539: Support changing Service off...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1727#discussion_r95514745
  
--- Diff: server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java ---
@@ -707,16 +802,41 @@ private UserVm orchestrateRevertToVMSnapshot(Long 
vmSnapshotId) throws Insuffici
 throw new InvalidParameterValueException("There is other 
active vm snapshot tasks on the instance, please try again later");
 }
 
+revertUserVmDetailsFromVmSnapshot(userVm, vmSnapshotVo);
+
 try {
 VMSnapshotStrategy strategy = 
findVMSnapshotStrategy(vmSnapshotVo);
 strategy.revertVMSnapshot(vmSnapshotVo);
+updateUserVmServiceOffering(userVm, vmSnapshotVo);
 return userVm;
 } catch (Exception e) {
 s_logger.debug("Failed to revert vmsnapshot: " + vmSnapshotId, 
e);
 throw new CloudRuntimeException(e.getMessage());
 }
 }
 
+/**
+ * Update or add user vm details from vm snapshot for vms with custom 
service offerings
+ * @param userVm user vm
+ * @param vmSnapshotVo vm snapshot
+ */
+private void revertUserVmDetailsFromVmSnapshot(UserVmVO userVm, 
VMSnapshotVO vmSnapshotVo) {
+ServiceOfferingVO serviceOfferingVO = 
_serviceOfferingDao.findById(vmSnapshotVo.getServiceOfferingId());
+if (serviceOfferingVO.isDynamic()) {
+List vmSnapshotDetails = 
_vmSnapshotDetailsDao.listDetails(vmSnapshotVo.getId());
+for (VMSnapshotDetailsVO detail : vmSnapshotDetails) {
--- End diff --

Check if saveDetails() can be used here or all the individual calls to 
update details need to be wrapped in transaction


---
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 #1727: CLOUDSTACK-9539: Support changing Service off...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1727#discussion_r95517926
  
--- Diff: setup/db/db/schema-490to4910.sql ---
@@ -71,3 +71,17 @@ INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` 
(uuid,hypervisor_type, hypervis
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(),'VMware', '6.0', 'sles11_64Guest', 187, utc_timestamp(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(),'VMware', '6.0', 'sles11Guest', 188, utc_timestamp(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(),'VMware', '6.0', 'sles12_64Guest', 244, utc_timestamp(), 0);
+
+-- Add service_offering_id column to vm_snapshots table
--- End diff --

This should be in the appropriate upgrade path now that 4.9.2 is released


---
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 #1727: CLOUDSTACK-9539: Support changing Service off...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1727#discussion_r95516498
  
--- Diff: server/src/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java ---
@@ -707,16 +802,41 @@ private UserVm orchestrateRevertToVMSnapshot(Long 
vmSnapshotId) throws Insuffici
 throw new InvalidParameterValueException("There is other 
active vm snapshot tasks on the instance, please try again later");
 }
 
+revertUserVmDetailsFromVmSnapshot(userVm, vmSnapshotVo);
--- End diff --

Are the details needed in the actual revert snapshot operation? If not then 
it is better to do these DB updates after the operation is completed 
successfully.


---
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 #1727: CLOUDSTACK-9539: Support changing Service off...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1727#discussion_r95516609
  
--- Diff: engine/schema/src/com/cloud/vm/UserVmDetailVO.java ---
@@ -80,4 +80,8 @@ public boolean isDisplay() {
 return display;
 }
 
+public void setValue(String value) {
--- End diff --

Is this used anywhere?


---
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 issue #1812: CLOUDSTACK-9650: Allow starting VMs regardless of cp...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1812
  
@rhtyd Sure will let you know if trillian tests are needed. In this case XS 
test results were already there from @cloudmonger. I used the git-pr script but 
not sure why the merge commit didn't have the messages, will check that out.


---
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 issue #1899: CLOUDSTACK-9650: Allow starting VMs regardless of cp...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1899
  
@ustcweizhou please review


---
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 #1899: CLOUDSTACK-9650: Allow starting VMs regardles...

[koushik-das]

GitHub user koushik-das opened a pull request:

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

CLOUDSTACK-9650: Allow starting VMs regardless of cpu/memory cluster.…

…disablethreshold setting

Fixed the scope of configuration flag 'cluster.threshold.enabled' 
introduced as part of PR#1812 to global

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

$ git pull https://github.com/Accelerite/cloudstack CLOUDSTACK-9650

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

https://github.com/apache/cloudstack/pull/1899.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 #1899


commit 44a59652489f4129fd09d4c8de0bb6f0247d8fd7
Author: Koushik Das <kous...@apache.org>
Date:   2017-01-10T10:38:59Z

CLOUDSTACK-9650: Allow starting VMs regardless of cpu/memory 
cluster.disablethreshold setting
Fixed the scope of configuration flag 'cluster.threshold.enabled' 
introduced as part of PR#1812 to global




---
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 issue #1812: CLOUDSTACK-9650: Allow starting VMs regardless of cp...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1812
  
Looks like this is ready for merge. None of the test failures are related 
to this PR.


---
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 issue #1829: CLOUDSTACK-9363: Fix HVM VM restart bug in XenServer

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1829
  
@pdube The description mentions about a way to workaround the problem, 
thats the reason for not calling it a blocker. If you think the issue is 
important for the current 4.9 release, please reply on the voting thread that 
is currently going on.


---
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 issue #1882: CLOUDSTACK-8737: Removed the missed out-of-band VR r...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1882
  
Code changes LGTM


---
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 #1861: CLOUDSTACK-9698 Make hardcorded wait timeout ...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1861#discussion_r94206618
  
--- Diff: 
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
 ---
@@ -230,6 +230,7 @@
 import com.cloud.hypervisor.guru.VMwareGuru;
 import com.cloud.hypervisor.vmware.manager.VmwareHostService;
 import com.cloud.hypervisor.vmware.manager.VmwareManager;
+import com.cloud.hypervisor.vmware.manager.VmwareManagerImpl;
--- End diff --

Better to define the config parameter in VmwareManager itself.


---
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 #1861: CLOUDSTACK-9698 Make hardcorded wait timeout ...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1861#discussion_r94206556
  
--- Diff: 
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java
 ---
@@ -123,12 +125,14 @@
 import com.cloud.utils.ssh.SshHelper;
 import com.cloud.vm.DomainRouterVO;
 
-public class VmwareManagerImpl extends ManagerBase implements 
VmwareManager, VmwareStorageMount, Listener, VmwareDatacenterService {
+public class VmwareManagerImpl extends ManagerBase implements 
VmwareManager, VmwareStorageMount, Listener, VmwareDatacenterService, 
Configurable {
 private static final Logger s_logger = 
Logger.getLogger(VmwareManagerImpl.class);
 
 private static final int STARTUP_DELAY = 6; // 60 
seconds
 private static final long DEFAULT_HOST_SCAN_INTERVAL = 60; // 
every 10 minutes
 
+public static final ConfigKey s_vmwareNicHotplugWaitTimeout = 
new ConfigKey("Advanced", Long.class, "vmware.nic.hotplug.wait.timeout", 
"2",
--- End diff --

Earlier the hardcoded wait was 15000 ms, now you have made the default as 
2 ms. Is this intended?


---
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 issue #1844: CLOUDSTACK-9668 : disksizeallocated of PrimaryStorag...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1844
  
LGTM. Unit tests?


---
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 issue #1850: CLOUDSTACK-9694: Unable to limit the Public IPs in V...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1850
  
Code changes LGTM. Can you add some unit/marvin tests for the scenario that 
you have mentioned?


---
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 issue #1841: CLOUDSTACK-9684 Invalid zone id error while listing ...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1841
  
Code changes LGTM


---
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 issue #1881: CLOUDSTACK-9721: Remove deprecated/unused global con...

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1881
  
Code changes LGTM


---
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 #1856: CLOUDSTACK-9569: propagate global configurati...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1856#discussion_r93603576
  
--- Diff: core/src/com/cloud/agent/api/SetHostParamsCommand.java ---
@@ -0,0 +1,43 @@
+//
+// 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;
+
+import java.util.Map;
+
+public class SetHostParamsCommand extends Command {
+
+Map<String, String> params;
--- End diff --

Map is not serializable, also HashMap that is actually used may have issues 
with serialization. Please check.


---
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 issue #1711: XenServer 7 Support

[koushik-das]

Github user koushik-das commented on the issue:

https://github.com/apache/cloudstack/pull/1711
  
Code changes are good. In the last test run I am seeing 
test_01_primary_storage_iscsi is failing. Do we know the reason for that?


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