You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/07/29 07:49:10 UTC
[GitHub] [cloudstack] ravening opened a new pull request #4228: Dont add host back after agent service restart
ravening opened a new pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228
## Description
<!--- Describe your changes in detail -->
Fixes #4218
If a host is removed from cloudstack, it will be
added back if we restart the agent service on
the host. It should not be added back if manualy
removed.
So when host is removed, guid is set to null.
When service is restarted, check if guid is null
for the host it send to mgt server. If guid is
null then dont add it back.
When we are adding host, temporarily set guid to
non null value so that when agent sends startuprouting command
it knows that the host is getting explicitly added.
Once host is added, set it back to null
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [X] Bug fix (non-breaking change which fixes an issue)
## Screenshots (if appropriate):
## How Has This Been Tested?
1. Add a host from ui. It will be successfully added
2. Now remove the host
3. ssh to host and restart cloudstack-agent service
4. The host is not added back
5. Try to add host from ui again. It will be added
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r480975079
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -410,6 +410,10 @@
public final static ConfigKey<Long> IOPS_MAX_WRITE_LENGTH = new ConfigKey<Long>(Long.class, "vm.disk.iops.maximum.write.length", "Advanced", "0",
"Maximum IOPS write burst duration (seconds). If '0' (zero) then does not check for maximum burst length.", true, ConfigKey.Scope.Global, null);
+ public static final ConfigKey<Boolean> ADD_HOST_ON_SERVICE_RESTART = new ConfigKey<Boolean>(Boolean.class, "add.host.on.service.restart", "Advanced", "true",
Review comment:
If this is KVM specific can we add kvm in the global setting name @ravening ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-668011597
> @ravening this will break the host-add feature for a lot of users who use the functionality to add a KVM host out-of-band without using the addHost API.
>
> Instead a middle-ground fix could be to remove a host's certificates when a KVM host is removed (all the /etc/cloudstack/agent/cloud.* files) which will stop KVM agent connections to be accepted by the management server for most deployments (i.e. CA plugin and auth strictness enabled).
@rhtyd this will work only if CA plugin and auth strictness is enabled but what if we have not configured both in some setup. In that case, the host will still be added
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-707032091
> ping @ravening any update? thanks.
@rhtyd apart from logs what else can I share? Manual testing involves
Add a host to cloudstack
Put it in maintenance mode
Remove it
Restart agent service on it
The host should not be added back
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening edited a comment on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening edited a comment on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-669102037
> @ravening @rhtyd how about hiding this behaviour behind a global setting `auto-add-hosts`? (default would have to be true for backwards compatibility)
@DaanHoogland is this what you are suggesting?
Lets say, I add a global setting `add.host.on.service.restart` with default value of true
When we delete the host, if the value is false then we set zone/cluster/pod value to null else retain as it is
Since default value is true, we dont change any value and host will be added back if service is restarted
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-668400923
Well @ravening fixing it otherwise would break some people's way of deploying/adding KVM host. Perhaps there's an easy way to reset the host (domain/IP) and zone/pod/cluster settings in the agent.properties (i.e. reset or remove them) when a host is removed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-696011060
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-669140042
@rhtyd @DaanHoogland please review again
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-694922835
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r482033071
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -410,6 +410,10 @@
public final static ConfigKey<Long> IOPS_MAX_WRITE_LENGTH = new ConfigKey<Long>(Long.class, "vm.disk.iops.maximum.write.length", "Advanced", "0",
"Maximum IOPS write burst duration (seconds). If '0' (zero) then does not check for maximum burst length.", true, ConfigKey.Scope.Global, null);
+ public static final ConfigKey<Boolean> ADD_HOST_ON_SERVICE_RESTART = new ConfigKey<Boolean>(Boolean.class, "add.host.on.service.restart", "Advanced", "true",
Review comment:
@rhtyd made the change
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-668417319
> Well @ravening fixing it otherwise would break some people's way of deploying/adding KVM host. Perhaps there's an easy way to reset the host (domain/IP) and zone/pod/cluster settings in the agent.properties (i.e. reset or remove them) when a host is removed?
I'm thinking of setting guid to null in agent.properties when host is removed and in connection handler we search for a host based on guid whenever we get a new connection request. Any other suggestions @rhtyd
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-694922355
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-669102037
> @ravening @rhtyd how about hiding this behaviour behind a global setting `auto-add-hosts`? (default would have to be true for backwards compatibility)
Lets say, I add a global setting `add.host.on.service.restart` with default value of true
When we delete the host, if the value is false then we set zone/cluster/pod value to null else retain as it is
Since default value is true, we dont change any value and host will be added back if service is restarted
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-696011060
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r481037844
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -410,6 +410,10 @@
public final static ConfigKey<Long> IOPS_MAX_WRITE_LENGTH = new ConfigKey<Long>(Long.class, "vm.disk.iops.maximum.write.length", "Advanced", "0",
"Maximum IOPS write burst duration (seconds). If '0' (zero) then does not check for maximum burst length.", true, ConfigKey.Scope.Global, null);
+ public static final ConfigKey<Boolean> ADD_HOST_ON_SERVICE_RESTART = new ConfigKey<Boolean>(Boolean.class, "add.host.on.service.restart", "Advanced", "true",
Review comment:
The agent only runs on KVM host, for the other two major hypervisors (VMware and XenServer) we don't run an agent on them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r468348299
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -410,6 +410,10 @@
public final static ConfigKey<Long> IOPS_MAX_WRITE_LENGTH = new ConfigKey<Long>(Long.class, "vm.disk.iops.maximum.write.length", "Advanced", "0",
"Maximum IOPS write burst duration (seconds). If '0' (zero) then does not check for maximum burst length.", true, ConfigKey.Scope.Global, null);
+ public static final ConfigKey<Boolean> AddHostOnServiceRestart = new ConfigKey<Boolean>(Boolean.class, "add.host.on.service.restart", "Advanced", "true",
Review comment:
Considering that this is defined as a `static final`, I would recommend keeping the code convention such as described on [Oracle Java Naming Conventions](https://www.oracle.com/java/technologies/javase/codeconventions-namingconventions.html).
In this case `AddHostOnServiceRestart` should be changed to something as `ADD_HOST_ON_SERVICE_RESTART`.
##########
File path: agent/src/main/java/com/cloud/agent/Agent.java
##########
@@ -419,6 +419,15 @@ protected void cancelTasks() {
}
}
+ protected void cleanupAgentZoneProperties() {
+ // Unset zone, cluster and pod values so that host is not added back
+ // when service is restarted. This will be set to proper values
+ // when host is added back
Review comment:
These commented lines could be a nice Javadoc for the method; what do you think of moving them to a Javadoc block?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r481277921
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -410,6 +410,10 @@
public final static ConfigKey<Long> IOPS_MAX_WRITE_LENGTH = new ConfigKey<Long>(Long.class, "vm.disk.iops.maximum.write.length", "Advanced", "0",
"Maximum IOPS write burst duration (seconds). If '0' (zero) then does not check for maximum burst length.", true, ConfigKey.Scope.Global, null);
+ public static final ConfigKey<Boolean> ADD_HOST_ON_SERVICE_RESTART = new ConfigKey<Boolean>(Boolean.class, "add.host.on.service.restart", "Advanced", "true",
Review comment:
Didnt know that :P will add KVM in the name
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r480987175
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -410,6 +410,10 @@
public final static ConfigKey<Long> IOPS_MAX_WRITE_LENGTH = new ConfigKey<Long>(Long.class, "vm.disk.iops.maximum.write.length", "Advanced", "0",
"Maximum IOPS write burst duration (seconds). If '0' (zero) then does not check for maximum burst length.", true, ConfigKey.Scope.Global, null);
+ public static final ConfigKey<Boolean> ADD_HOST_ON_SERVICE_RESTART = new ConfigKey<Boolean>(Boolean.class, "add.host.on.service.restart", "Advanced", "true",
Review comment:
@rhtyd i don't think this is KVM specific. The code path is same for all HV's I guess. Didn't test it on other Hv types.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland closed pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland closed pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-696087107
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-689871016
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-697884885
@GabrielBrascher @wido @rhtyd are we 🆗 with this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-669146670
@wido @GabrielBrascher I think this will impact you guys most. Can you review?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r462514784
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
Review comment:
I have no idea why it's there in first place. So I'm displaying message whatever we catch
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
throw e;
} catch (Exception e) {
String msg = " can't setup agent, due to " + e.toString() + " - " + e.getMessage();
s_logger.warn(msg);
} finally {
+ // set guid to null once host is added
Review comment:
Initialguid will always beer null since I'm fetching the value for a removed host. For non removed hosts it will be non null
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland merged pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r464399196
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
throw e;
} catch (Exception e) {
String msg = " can't setup agent, due to " + e.toString() + " - " + e.getMessage();
s_logger.warn(msg);
} finally {
+ // set guid to null once host is added
Review comment:
@DaanHoogland this code is executed only for the newly added hosts. When the host is removed previously, the guid is set to null. So the initial value of `guid` will always be `null` in this case.
Before adding the host, I set the guid to a temporary value and at the end I set it back to null which is the original value.
I hope to have cleared your confusion
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-666127252
@ravening this will break the host-add feature for a lot of users who use the functionality to add a KVM host out-of-band without using the addHost API.
Instead a middle-ground fix could be to remove a host's certificates when a KVM host is removed (all the /etc/cloudstack/agent/cloud.* files) which will stop KVM agent connections to be accepted by the management server for most deployments (i.e. CA plugin and auth strictness enabled).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-696551505
<b>Trillian test result (tid-2824)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 64517 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4228-t2824-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 82 look OK, 3 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_add_delete_kubernetes_supported_version | `Error` | 1816.25 | test_kubernetes_supported_versions.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 360.57 | test_vpc_redundant.py
test_hostha_kvm_host_fencing | `Error` | 5.55 | test_hostha_kvm.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] wido commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
wido commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-669894978
I have never run into this situation to be honest. But the change seems sane to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-707030955
ping @ravening any update? thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-669099849
@ravening @rhtyd how about hiding this behaviour behind a global setting `auto-add-hosts`? (default would have to be true for backwards compatibility)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-698157726
@rhtyd Sure... I will share the results by tomorrow
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r462482006
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
throw e;
} catch (Exception e) {
String msg = " can't setup agent, due to " + e.toString() + " - " + e.getMessage();
s_logger.warn(msg);
} finally {
+ // set guid to null once host is added
Review comment:
is this comment correct? initialGuid might have a value other than null.
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
Review comment:
this is not really saying it. what did we fail at for what reason with (and this part is there) what message.
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
throw e;
} catch (Exception e) {
String msg = " can't setup agent, due to " + e.toString() + " - " + e.getMessage();
s_logger.warn(msg);
} finally {
+ // set guid to null once host is added
Review comment:
exactly. it is not necessarily null. I think you'd better remove this comment, it is confusing.
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -348,11 +362,19 @@ private void setupAgentSecurity(final Connection sshConnection, final String age
_hostDao.saveDetails(connectedHost);
return resources;
} catch (DiscoveredWithErrorException e) {
+ s_logger.error("Exception: "+ e.getMessage());
Review comment:
ok, than let's at least make the text describing our course of action:
```suggestion
s_logger.error("DiscoveredWithErrorException caught and rethrowing, message; " + e.getMessage());
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-689890109
Packaging result: ✔centos7 ✖centos8 ✖debian. JID-1944
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-698157726
@rhtyd Sure... I will share the results by tomorrow
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-689870779
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-694949083
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2044
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-669059632
@ravening I think resetting the host, zone, cluster, etc to default (or remove) should be enough. Post agent restart the agent will not know where to connect.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-696551505
<b>Trillian test result (tid-2824)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 64517 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4228-t2824-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 82 look OK, 3 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_add_delete_kubernetes_supported_version | `Error` | 1816.25 | test_kubernetes_supported_versions.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | `Failure` | 360.57 | test_vpc_redundant.py
test_hostha_kvm_host_fencing | `Error` | 5.55 | test_hostha_kvm.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r468411269
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -410,6 +410,10 @@
public final static ConfigKey<Long> IOPS_MAX_WRITE_LENGTH = new ConfigKey<Long>(Long.class, "vm.disk.iops.maximum.write.length", "Advanced", "0",
"Maximum IOPS write burst duration (seconds). If '0' (zero) then does not check for maximum burst length.", true, ConfigKey.Scope.Global, null);
+ public static final ConfigKey<Boolean> AddHostOnServiceRestart = new ConfigKey<Boolean>(Boolean.class, "add.host.on.service.restart", "Advanced", "true",
Review comment:
@GabrielBrascher I made the necessary changes. Please review again
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#discussion_r465667554
##########
File path: server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java
##########
@@ -474,7 +477,8 @@ public DeleteHostAnswer deleteHost(HostVO host, boolean isForced, boolean isForc
_resourceMgr.deleteRoutingHost(host, isForced, isForceDeleteStorage);
try {
- ShutdownCommand cmd = new ShutdownCommand(ShutdownCommand.DeleteHost, null);
+ ShutdownCommand cmd = AddHostOnServiceRestart.value() ? new ShutdownCommand(ShutdownCommand.DeleteHost, null, false) :
+ new ShutdownCommand(ShutdownCommand.DeleteHost, "Cleaning up zone/pod/cluster details for host", true);
Review comment:
general jist looks good, i'd rather have the tristate operator in the parameter passing then the whole contructor invocation ? note the ?)
```suggestion
ShutdownCommand cmd = new ShutdownCommand(ShutdownCommand.DeleteHost, null, AddHostOnServiceRestart.value() ? false : true);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-698154589
@ravening @weizhouapache can you share your manual test results if any?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-696087359
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-696010577
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-698154589
@ravening @weizhouapache can you share your manual test results if any?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4228: Dont add host back after agent service restart
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4228:
URL: https://github.com/apache/cloudstack/pull/4228#issuecomment-696010577
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org