You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wido <gi...@git.apache.org> on 2016/05/13 09:41:12 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
GitHub user wido opened a pull request:
https://github.com/apache/cloudstack/pull/1545
CLOUDSTACK-8715: Add channel to Instances for Qemu Guest Agent
This commit adds a additional VirtIO channel with the name
'org.qemu.guest_agent.0' to all Instances.
With the Qemu Guest Agent the Hypervisor gains more control over the Instance if
these tools are present inside the Instance, for example:
* Power control
* Flushing filesystems
* Fetching Network information
In the future this should allow safer snapshots on KVM since we can instruct the
Instance to flush the filesystems prior to snapshotting the disk.
More information: http://wiki.qemu.org/Features/QAPI/GuestAgent
Keep in mind that on Ubuntu AppArmor still needs to be disabled since the default
AppArmor profile doesn't allow libvirt to write into /var/lib/libvirt/qemu
This commit does not add any communication methods through API-calls, it merely
adds the channel to the Instances and installs the Guest Agent in the SSVMs.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wido/cloudstack qemu-guest-agent
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1545.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 #1545
----
commit 58a591ce417da5cb3e3dcd3cf182369cc65be028
Author: Wido den Hollander <wi...@widodh.nl>
Date: 2016-05-13T09:04:49Z
CLOUDSTACK-8715: Add channel to Instances for Qemu Guest Agent
This commit adds a additional VirtIO channel with the name
'org.qemu.guest_agent.0' to all Instances.
With the Qemu Guest Agent the Hypervisor gains more control over the Instance if
these tools are present inside the Instance, for example:
* Power control
* Flushing filesystems
* Fetching Network information
In the future this should allow safer snapshots on KVM since we can instruct the
Instance to flush the filesystems prior to snapshotting the disk.
More information: http://wiki.qemu.org/Features/QAPI/GuestAgent
Keep in mind that on Ubuntu AppArmor still needs to be disabled since the default
AppArmor profile doesn't allow libvirt to write into /var/lib/libvirt/qemu
This commit does not add any communication methods through API-calls, it merely
adds the channel to the Instances and installs the Guest Agent in the SSVMs.
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido the Trillian env failed to run the tests for your latest PR due to following error:
2016-11-21 09:59:54,000 INFO [cloud.agent.AgentShell] (main:null) (logid:) Agent started
2016-11-21 09:59:54,005 INFO [cloud.agent.AgentShell] (main:null) (logid:) Implementation Version is 4.10.0.0-SNAPSHOT
2016-11-21 09:59:54,007 INFO [cloud.agent.AgentShell] (main:null) (logid:) agent.properties found at /etc/cloudstack/agent/agent.properties
2016-11-21 09:59:54,013 INFO [cloud.agent.AgentShell] (main:null) (logid:) Defaulting to using properties file for storage
2016-11-21 09:59:54,016 INFO [cloud.agent.AgentShell] (main:null) (logid:) Defaulting to the constant time backoff algorithm
2016-11-21 09:59:54,035 INFO [cloud.utils.LogUtils] (main:null) (logid:) log4j configuration found at /etc/cloudstack/agent/log4j-cloud.xml
2016-11-21 09:59:54,050 INFO [cloud.agent.AgentShell] (main:null) (logid:) Using default Java settings for IPv6 preference for agent connection
2016-11-21 09:59:54,211 INFO [cloud.agent.Agent] (main:null) (logid:) id is
2016-11-21 09:59:54,251 ERROR [cloud.agent.AgentShell] (main:null) (logid:) Unable to start agent:
java.lang.NullPointerException
at java.io.File.<init>(File.java:277)
at com.cloud.hypervisor.kvm.resource.LibvirtComputingResource.configure(LibvirtComputingResource.java:786)
at com.cloud.agent.Agent.<init>(Agent.java:165)
at com.cloud.agent.AgentShell.launchAgent(AgentShell.java:397)
at com.cloud.agent.AgentShell.launchAgentFromClassInfo(AgentShell.java:367)
at com.cloud.agent.AgentShell.launchAgent(AgentShell.java:351)
at com.cloud.agent.AgentShell.start(AgentShell.java:456)
at com.cloud.agent.AgentShell.main(AgentShell.java:491)
Please modify the code to still work where an explicit path may not be defined, and that with your change KVM agent would run on CentOS6/7. Thanks.
/cc @jburwell
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-219104928
Ya, I like this. I think this is a nice advancement. If we are going to try to get this into 4.9, I will need some help getting this moving forward cause we are pretty tight on time. @wido are you running this in production and what can you provide to show it is working as expected? I need to review the details for releasing when including system VM changes. I know we wanted to try to get StrongSwan into this release (assuming we can get it there), so that would require a new system VM template 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan test
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido @rhtyd given the flakiness of patchviasocket, I think we should consider including this fix in all supported branches (4.8, 4.9, and master). The issues I have seen move this bug from a Major to a Critical -- especially in large environments where the law of large numbers greatly increases the probability of occurrence and its impact. My primary hesitation is the need for a new system VM template. However, I think LTS will eventually require a new system VM template to address outstanding site 2 site VPN issues. Also, I don't see the patchviasocket script being impacted by this patch. How does this patch fix/replace patchviasocket?
/cc @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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd Yes, we could not add the port to SSVMs right now and thus skip the Agent inside the SSVM.
That way only Instances will get the Socket and we can use it.
We can add it to the SSVM later, that would be a very simple PR.
Would that be a easier route to get this in?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r89533059
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java ---
@@ -175,6 +180,26 @@ public boolean parseDomainXML(String domXML) {
interfaces.add(def);
}
+ NodeList ports = devices.getElementsByTagName("channel");
+ for (int i = 0; i < ports.getLength(); i++) {
+ Element channel = (Element)ports.item(i);
+
+ String type = channel.getAttribute("type");
+ String path = getAttrValue("source", "path", channel);
+ String name = getAttrValue("target", "name", channel);
+ String state = getAttrValue("target", "state", channel);
+
+ ChannelDef def = null;
+ if (!StringUtils.isNotBlank(state)) {
--- End diff --
This is fine, though you can use guava's Strings.isNullOrEmpty, which is what I've been using in most of my 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Thanks for the tests @bvbharatk ! Those tests seem network related to the VR and I've seen those tests fail on other parts as well.
This code only touches KVM and the Xen tests don't include that. So the failures do not seem related to this code.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74250773
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
+
+ public ChannelDef(String name, ChannelType type) {
+ this.name = name;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, String path) {
+ this.name = name;
+ this.path = path;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state) {
+ this.name = name;
+ this.state = state;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state, String path) {
+ this.name = name;
+ this.path = path;
+ this.state = state;
+ this.type = type;
+ }
+
+ public ChannelType getChannelType() {
+ return type;
+ }
+
+ public ChannelState getChannelState() {
+ return state;
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public String getPath() {
+ return path;
}
@Override
public String toString() {
StringBuilder virtioSerialBuilder = new StringBuilder();
- if (_path == null) {
- _path = "/var/lib/libvirt/qemu";
+ virtioSerialBuilder.append("<channel type='" + type.toString() + "'>\n");
--- End diff --
Please use ``System.lineseparator()`` instead of ``\n`` for proper platform independence.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan test
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r88378255
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---
@@ -252,6 +253,8 @@
protected String _rngPath = "/dev/random";
protected int _rngRatePeriod = 1000;
protected int _rngRateBytes = 2048;
+ protected File _qemuSocketsPath;
+ private String _qemuGuestAgentSocketName = "org.qemu.guest_agent.0";
--- End diff --
`_qemuGuestAgentSocketName` appears to be a constant value. If so, please consider declaring it as a 'private static final`.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan test
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido thanks, @PaulAngus found an issue with the kvm patchviasocket script and I've sent a fix #1533
I was not sure what was causing the failure and saw this PR which looked promising. Let's get it in master and for master we can rebuild a new systemvm template (for kvm only?). /cc @jburwell @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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r88378673
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
+
+ public ChannelDef(String name, ChannelType type) {
+ this.name = name;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, String path) {
+ this.name = name;
+ this.path = path;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state) {
+ this.name = name;
+ this.state = state;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state, String path) {
+ this.name = name;
+ this.path = path;
+ this.state = state;
+ this.type = type;
+ }
+
+ public ChannelType getChannelType() {
+ return type;
+ }
+
+ public ChannelState getChannelState() {
+ return state;
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public String getPath() {
+ return path;
}
@Override
public String toString() {
StringBuilder virtioSerialBuilder = new StringBuilder();
- if (_path == null) {
- _path = "/var/lib/libvirt/qemu";
+ virtioSerialBuilder.append("<channel type='" + type.toString() + "'>\n");
--- End diff --
@wido have you had a chance to consider this 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@jburwell a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
We have multiple LGTM. Are we good to merge 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r63849902
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java ---
@@ -171,6 +173,25 @@ public boolean parseDomainXML(String domXML) {
interfaces.add(def);
}
+ NodeList ports = devices.getElementsByTagName("channel");
+ for (int i = 0; i < ports.getLength(); i++) {
+ Element channel = (Element)ports.item(i);
+
+ String type = channel.getAttribute("type");
+ String path = getAttrValue("source", "path", channel);
+ String name = getAttrValue("target", "name", channel);
+ String state = getAttrValue("target", "state", channel);
+
+ ChannelDef def = null;
+ if (state == null || state.length() == 0) {
--- End diff --
we can use Strings.isNullOrEmpty here
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido I would like to get this PR merged for 4.10.0.0. Could you please rebase to resolve conflicts and pick up the latest Marvin enhancements/fixes? Once this rebase is done, I will kick off KVM regression tests in order to merge 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74251739
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
+
+ public ChannelDef(String name, ChannelType type) {
+ this.name = name;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, String path) {
+ this.name = name;
+ this.path = path;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state) {
+ this.name = name;
+ this.state = state;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state, String path) {
+ this.name = name;
+ this.path = path;
+ this.state = state;
+ this.type = type;
+ }
+
+ public ChannelType getChannelType() {
+ return type;
+ }
+
+ public ChannelState getChannelState() {
+ return state;
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public String getPath() {
+ return path;
}
@Override
public String toString() {
StringBuilder virtioSerialBuilder = new StringBuilder();
- if (_path == null) {
- _path = "/var/lib/libvirt/qemu";
+ virtioSerialBuilder.append("<channel type='" + type.toString() + "'>\n");
+ if (path == null) {
+ virtioSerialBuilder.append("<source mode='bind'/>\n");
+ } else {
+ virtioSerialBuilder.append("<source mode='bind' path='" + path + "'/>\n");
}
- virtioSerialBuilder.append("<channel type='unix'>\n");
- virtioSerialBuilder.append("<source mode='bind' path='" + _path + "/" + _name + ".agent'/>\n");
- virtioSerialBuilder.append("<target type='virtio' name='" + _name + ".vport'/>\n");
virtioSerialBuilder.append("<address type='virtio-serial'/>\n");
+ if (state == null) {
+ virtioSerialBuilder.append("<target type='virtio' name='" + name + "'/>\n");
+ } else {
+ virtioSerialBuilder.append("<target type='virtio' name='" + name + "' state='" + state.toString() + "'/>\n");
+ }
--- End diff --
See previously recommended refactoring for source mode as this ``if`` condition appears to follow the same pattern.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r88378111
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---
@@ -252,6 +253,8 @@
protected String _rngPath = "/dev/random";
protected int _rngRatePeriod = 1000;
protected int _rngRateBytes = 2048;
+ protected File _qemuSocketsPath;
--- End diff --
Why is this attribute declared `protected` rather than `private`?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r63889814
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---
@@ -1978,11 +1978,16 @@ So if getMinSpeed() returns null we fall back to getSpeed().
final SerialDef serial = new SerialDef("pty", null, (short)0);
devices.addDevice(serial);
+ /* Add a VirtIO channel for SystemVMs for communication and provisioning */
if (vmTO.getType() != VirtualMachine.Type.User) {
- final VirtioSerialDef vserial = new VirtioSerialDef(vmTO.getName(), null);
- devices.addDevice(vserial);
+ devices.addDevice(new ChannelDef(vmTO.getName() + ".vport", ChannelDef.ChannelType.UNIX,
+ "/var/lib/libvirt/qemu/" + vmTO.getName() + ".agent"));
--- End diff --
This has always been hardcoded, it's just that I'm moving this part.
This is the SSVM socket which is already there. The socket patch code wants to find the .agent there. Not much I can do 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Sorry backend is `down`, please try again after sometime.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-221221816
I understand @swill. I'll leave this up to you.
The PR has two LGTMs, so we can merge when we want. Same goes for #1531
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r63848326
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String _type;
+
+ ChannelType(String type) {
+ _type = type;
+ }
+
+ @Override
+ public String toString() {
+ return _type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String _type;
+
+ ChannelState(String type) {
+ _type = type;
+ }
+
+ @Override
+ public String toString() {
+ return _type;
+ }
+ }
+
+ private String _name;
private String _path;
+ private ChannelType _type;
+ private ChannelState _state;
+
+ public ChannelDef(String name, ChannelType type) {
+ _name = name;
+ _type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, String path) {
+ _name = name;
+ _path = path;
+ _type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state) {
+ _name = name;
+ _state = state;
+ _type = type;
+ }
- public VirtioSerialDef(String name, String path) {
+ public ChannelDef(String name, ChannelType type, ChannelState state, String path) {
_name = name;
_path = path;
+ _state = state;
+ _type = type;
--- End diff --
please remove the '_'s
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
LGTM, @wido looks like this will require a new systemvm 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-237
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
<b>Trillian test result (tid-421)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29503 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr1545-t421-kvm-centos7.zip
Test completed. 48 look ok, 1 have error(s)
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_create_template | `Error` | 55.62 | test_templates.py
test_01_vpc_site2site_vpn | Success | 166.49 | test_vpc_vpn.py
test_01_vpc_remote_access_vpn | Success | 66.20 | test_vpc_vpn.py
test_01_redundant_vpc_site2site_vpn | Success | 321.26 | test_vpc_vpn.py
test_02_VPC_default_routes | Success | 311.06 | test_vpc_router_nics.py
test_01_VPC_nics_after_destroy | Success | 552.87 | test_vpc_router_nics.py
test_05_rvpc_multi_tiers | Success | 534.88 | test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics | Success | 1427.78 | test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Success | 564.46 | test_vpc_redundant.py
test_02_redundant_VPC_default_routes | Success | 823.05 | test_vpc_redundant.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Success | 1359.34 | test_vpc_redundant.py
test_09_delete_detached_volume | Success | 15.67 | test_volumes.py
test_08_resize_volume | Success | 15.46 | test_volumes.py
test_07_resize_fail | Success | 20.53 | test_volumes.py
test_06_download_detached_volume | Success | 15.38 | test_volumes.py
test_05_detach_volume | Success | 100.28 | test_volumes.py
test_04_delete_attached_volume | Success | 10.27 | test_volumes.py
test_03_download_attached_volume | Success | 15.36 | test_volumes.py
test_02_attach_volume | Success | 43.75 | test_volumes.py
test_01_create_volume | Success | 621.70 | test_volumes.py
test_deploy_vm_multiple | Success | 299.13 | test_vm_life_cycle.py
test_deploy_vm | Success | 0.04 | test_vm_life_cycle.py
test_advZoneVirtualRouter | Success | 0.03 | test_vm_life_cycle.py
test_10_attachAndDetach_iso | Success | 27.20 | test_vm_life_cycle.py
test_09_expunge_vm | Success | 125.26 | test_vm_life_cycle.py
test_08_migrate_vm | Success | 36.04 | test_vm_life_cycle.py
test_07_restore_vm | Success | 0.15 | test_vm_life_cycle.py
test_06_destroy_vm | Success | 130.98 | test_vm_life_cycle.py
test_03_reboot_vm | Success | 131.14 | test_vm_life_cycle.py
test_02_start_vm | Success | 10.20 | test_vm_life_cycle.py
test_01_stop_vm | Success | 40.37 | test_vm_life_cycle.py
test_CreateTemplateWithDuplicateName | Success | 176.45 | test_templates.py
test_08_list_system_templates | Success | 0.03 | test_templates.py
test_07_list_public_templates | Success | 0.05 | test_templates.py
test_05_template_permissions | Success | 0.13 | test_templates.py
test_04_extract_template | Success | 5.16 | test_templates.py
test_03_delete_template | Success | 5.13 | test_templates.py
test_02_edit_template | Success | 90.17 | test_templates.py
test_10_destroy_cpvm | Success | 166.77 | test_ssvm.py
test_09_destroy_ssvm | Success | 198.98 | test_ssvm.py
test_08_reboot_cpvm | Success | 136.74 | test_ssvm.py
test_07_reboot_ssvm | Success | 139.10 | test_ssvm.py
test_06_stop_cpvm | Success | 167.17 | test_ssvm.py
test_05_stop_ssvm | Success | 168.95 | test_ssvm.py
test_04_cpvm_internals | Success | 1.24 | test_ssvm.py
test_03_ssvm_internals | Success | 3.92 | test_ssvm.py
test_02_list_cpvm_vm | Success | 0.14 | test_ssvm.py
test_01_list_sec_storage_vm | Success | 0.17 | test_ssvm.py
test_01_snapshot_root_disk | Success | 21.61 | test_snapshots.py
test_04_change_offering_small | Success | 215.40 | test_service_offerings.py
test_03_delete_service_offering | Success | 0.05 | test_service_offerings.py
test_02_edit_service_offering | Success | 0.10 | test_service_offerings.py
test_01_create_service_offering | Success | 0.16 | test_service_offerings.py
test_02_sys_template_ready | Success | 0.14 | test_secondary_storage.py
test_01_sys_vm_start | Success | 0.20 | test_secondary_storage.py
test_09_reboot_router | Success | 55.49 | test_routers.py
test_08_start_router | Success | 35.39 | test_routers.py
test_07_stop_router | Success | 15.23 | test_routers.py
test_06_router_advanced | Success | 0.06 | test_routers.py
test_05_router_basic | Success | 0.06 | test_routers.py
test_04_restart_network_wo_cleanup | Success | 5.75 | test_routers.py
test_03_restart_network_cleanup | Success | 75.67 | test_routers.py
test_02_router_internal_adv | Success | 1.05 | test_routers.py
test_01_router_internal_basic | Success | 0.58 | test_routers.py
test_router_dns_guestipquery | Success | 78.25 | test_router_dns.py
test_router_dns_externalipquery | Success | 0.08 | test_router_dns.py
test_router_dhcphosts | Success | 287.06 | test_router_dhcphosts.py
test_01_updatevolumedetail | Success | 0.12 | test_resource_detail.py
test_01_reset_vm_on_reboot | Success | 186.54 | test_reset_vm_on_reboot.py
test_createRegion | Success | 0.06 | test_regions.py
test_create_pvlan_network | Success | 5.29 | test_pvlan.py
test_dedicatePublicIpRange | Success | 0.61 | test_public_ip_range.py
test_04_rvpc_privategw_static_routes | Success | 1087.09 | test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup | Success | 609.67 | test_privategw_acl.py
test_02_vpc_privategw_static_routes | Success | 508.03 | test_privategw_acl.py
test_01_vpc_privategw_acl | Success | 108.70 | test_privategw_acl.py
test_01_primary_storage_nfs | Success | 36.09 | test_primary_storage.py
test_createPortablePublicIPRange | Success | 10.39 | test_portable_publicip.py
test_createPortablePublicIPAcquire | Success | 15.50 | test_portable_publicip.py
test_isolate_network_password_server | Success | 90.10 | test_password_server.py
test_UpdateStorageOverProvisioningFactor | Success | 0.17 | test_over_provisioning.py
test_oobm_zchange_password | Success | 21.02 | test_outofbandmanagement.py
test_oobm_multiple_mgmt_server_ownership | Success | 16.52 | test_outofbandmanagement.py
test_oobm_issue_power_status | Success | 11.01 | test_outofbandmanagement.py
test_oobm_issue_power_soft | Success | 15.53 | test_outofbandmanagement.py
test_oobm_issue_power_reset | Success | 15.55 | test_outofbandmanagement.py
test_oobm_issue_power_on | Success | 15.52 | test_outofbandmanagement.py
test_oobm_issue_power_off | Success | 15.52 | test_outofbandmanagement.py
test_oobm_issue_power_cycle | Success | 15.51 | test_outofbandmanagement.py
test_oobm_enabledisable_across_clusterzones | Success | 62.76 | test_outofbandmanagement.py
test_oobm_enable_feature_valid | Success | 5.28 | test_outofbandmanagement.py
test_oobm_enable_feature_invalid | Success | 0.18 | test_outofbandmanagement.py
test_oobm_disable_feature_valid | Success | 0.28 | test_outofbandmanagement.py
test_oobm_disable_feature_invalid | Success | 0.24 | test_outofbandmanagement.py
test_oobm_configure_invalid_driver | Success | 0.14 | test_outofbandmanagement.py
test_oobm_configure_default_driver | Success | 0.18 | test_outofbandmanagement.py
test_oobm_background_powerstate_sync | Success | 23.87 | test_outofbandmanagement.py
test_extendPhysicalNetworkVlan | Success | 15.35 | test_non_contigiousvlan.py
test_01_nic | Success | 637.54 | test_nic.py
test_releaseIP | Success | 349.88 | test_network.py
test_reboot_router | Success | 470.65 | test_network.py
test_public_ip_user_account | Success | 10.32 | test_network.py
test_public_ip_admin_account | Success | 40.29 | test_network.py
test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Success | 67.18 | test_network.py
test_network_rules_acquired_public_ip_2_nat_rule | Success | 61.99 | test_network.py
test_network_rules_acquired_public_ip_1_static_nat_rule | Success | 124.50 | test_network.py
test_delete_account | Success | 279.31 | test_network.py
test_02_port_fwd_on_non_src_nat | Success | 56.17 | test_network.py
test_01_port_fwd_on_src_nat | Success | 112.02 | test_network.py
test_nic_secondaryip_add_remove | Success | 254.25 | test_multipleips_per_nic.py
login_test_saml_user | Success | 26.37 | test_login.py
test_assign_and_removal_lb | Success | 134.19 | test_loadbalance.py
test_02_create_lb_rule_non_nat | Success | 187.98 | test_loadbalance.py
test_01_create_lb_rule_src_nat | Success | 240.15 | test_loadbalance.py
test_03_list_snapshots | Success | 0.11 | test_list_ids_parameter.py
test_02_list_templates | Success | 0.05 | test_list_ids_parameter.py
test_01_list_volumes | Success | 0.04 | test_list_ids_parameter.py
test_07_list_default_iso | Success | 0.07 | test_iso.py
test_05_iso_permissions | Success | 0.07 | test_iso.py
test_04_extract_Iso | Success | 5.59 | test_iso.py
test_03_delete_iso | Success | 95.21 | test_iso.py
test_02_edit_iso | Success | 0.06 | test_iso.py
test_01_create_iso | Success | 22.49 | test_iso.py
test_04_rvpc_internallb_haproxy_stats_on_all_interfaces | Success | 309.76 | test_internal_lb.py
test_03_vpc_internallb_haproxy_stats_on_all_interfaces | Success | 309.27 | test_internal_lb.py
test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80 | Success | 679.36 | test_internal_lb.py
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | Success | 502.22 | test_internal_lb.py
test_dedicateGuestVlanRange | Success | 10.35 | test_guest_vlan_range.py
test_UpdateConfigParamWithScope | Success | 0.16 | test_global_settings.py
test_rolepermission_lifecycle_update | Success | 7.57 | test_dynamicroles.py
test_rolepermission_lifecycle_list | Success | 7.54 | test_dynamicroles.py
test_rolepermission_lifecycle_delete | Success | 6.98 | test_dynamicroles.py
test_rolepermission_lifecycle_create | Success | 7.08 | test_dynamicroles.py
test_rolepermission_lifecycle_concurrent_updates | Success | 7.01 | test_dynamicroles.py
test_role_lifecycle_update_role_inuse | Success | 6.88 | test_dynamicroles.py
test_role_lifecycle_update | Success | 12.69 | test_dynamicroles.py
test_role_lifecycle_list | Success | 6.99 | test_dynamicroles.py
test_role_lifecycle_delete | Success | 12.14 | test_dynamicroles.py
test_role_lifecycle_create | Success | 7.28 | test_dynamicroles.py
test_role_inuse_deletion | Success | 6.81 | test_dynamicroles.py
test_role_account_acls_multiple_mgmt_servers | Success | 9.48 | test_dynamicroles.py
test_role_account_acls | Success | 9.53 | test_dynamicroles.py
test_default_role_deletion | Success | 7.23 | test_dynamicroles.py
test_04_create_fat_type_disk_offering | Success | 0.11 | test_disk_offerings.py
test_03_delete_disk_offering | Success | 0.04 | test_disk_offerings.py
test_02_edit_disk_offering | Success | 0.05 | test_disk_offerings.py
test_02_create_sparse_type_disk_offering | Success | 0.07 | test_disk_offerings.py
test_01_create_disk_offering | Success | 0.11 | test_disk_offerings.py
test_deployvm_userdispersing | Success | 20.75 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userconcentrated | Success | 20.79 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_firstfit | Success | 50.68 | test_deploy_vms_with_varied_deploymentplanners.py
test_deployvm_userdata_post | Success | 5.45 | test_deploy_vm_with_userdata.py
test_deployvm_userdata | Success | 46.14 | test_deploy_vm_with_userdata.py
test_02_deploy_vm_root_resize | Success | 6.93 | test_deploy_vm_root_resize.py
test_01_deploy_vm_root_resize | Success | 7.20 | test_deploy_vm_root_resize.py
test_00_deploy_vm_root_resize | Success | 279.09 | test_deploy_vm_root_resize.py
test_deploy_vm_from_iso | Success | 218.50 | test_deploy_vm_iso.py
test_DeployVmAntiAffinityGroup | Success | 96.33 | test_affinity_groups.py
test_03_delete_vm_snapshots | Skipped | 0.00 | test_vm_snapshots.py
test_02_revert_vm_snapshots | Skipped | 0.00 | test_vm_snapshots.py
test_01_test_vm_volume_snapshot | Skipped | 0.00 | test_vm_snapshots.py
test_01_create_vm_snapshots | Skipped | 0.00 | test_vm_snapshots.py
test_06_copy_template | Skipped | 0.00 | test_templates.py
test_static_role_account_acls | Skipped | 0.03 | test_staticroles.py
test_11_ss_nfs_version_on_ssvm | Skipped | 0.02 | test_ssvm.py
test_01_scale_vm | Skipped | 0.00 | test_scale_vm.py
test_01_primary_storage_iscsi | Skipped | 0.05 | test_primary_storage.py
test_nested_virtualization_vmware | Skipped | 0.00 | test_nested_virtualization.py
test_06_copy_iso | Skipped | 0.00 | test_iso.py
test_deploy_vgpu_enabled_vm | Skipped | 0.04 | test_deploy_vgpu_enabled_vm.py
test_3d_gpu_support | Skipped | 0.04 | test_deploy_vgpu_enabled_vm.py
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Yes @rhtyd, I added a check for NULL 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-219034798
@swill and @rhtyd could you take a look at this one?
Once we fork libvirt-java (which I want to do when we switch to new repos) we can implement the actual commands in the KVM Agent.
Just having the channels there also allows admins to send commands to Instances using virsh manually.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan package
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r89449483
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java ---
@@ -175,6 +180,26 @@ public boolean parseDomainXML(String domXML) {
interfaces.add(def);
}
+ NodeList ports = devices.getElementsByTagName("channel");
+ for (int i = 0; i < ports.getLength(); i++) {
+ Element channel = (Element)ports.item(i);
+
+ String type = channel.getAttribute("type");
+ String path = getAttrValue("source", "path", channel);
+ String name = getAttrValue("target", "name", channel);
+ String state = getAttrValue("target", "state", channel);
+
+ ChannelDef def = null;
+ if (!StringUtils.isNotBlank(state)) {
--- End diff --
com.cloud.utils.StringUtils doesn't have that method. We would need to use the Apache Commons 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220599638
LGTM, do we retest @swill?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220278587
In general, a good idea and would be useful for flushing out disk IO and doing clean shutdown of VRs and vms. There are some outstanding issues, but the feature is nice.
I've not tested this, I suppose @wido would have tested this for Debian/Ubuntu based kvm hosts, we'll also need testing efforts on centos6 and 7 based kvm hosts.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74250269
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
+
+ public ChannelDef(String name, ChannelType type) {
+ this.name = name;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, String path) {
--- End diff --
Why not delegate to ``this(String name, ChannelType type, ChannelState state, String path)``?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r63848237
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String _type;
+
+ ChannelType(String type) {
+ _type = type;
+ }
+
+ @Override
+ public String toString() {
+ return _type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String _type;
+
+ ChannelState(String type) {
+ _type = type;
+ }
+
+ @Override
+ public String toString() {
+ return _type;
+ }
+ }
+
+ private String _name;
private String _path;
--- End diff --
this one is legacy but the others should be named without the '_'
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido I can help run tests if you can rebase/fix-conflicts on this PR, thanks.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd Do you want to backport this to 4.9 then? I've changed the base to 4.9 now, take a look about what you think.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@jburwell @rhtyd I fixed most of the comments.
The NPE should be fixed and the StringUtils comments as well. I didn't refactor the other stuff @jburwell mentioned as my code is in-line with how the existing classes are written in the same files.
A new PR could do a refactor of those whole files, but it wouldn't change anything for a end-result. The functionality is still that we have a Qemu Guest Agent inside the VMs.
But this PR is open since May. Could we please merge this one in?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220579267
@swill I have pushed the new version of the commits
@rhtyd @DaanHoogland Thanks for your review and feedback. Should be good now I think.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd I didn't realize that we needed a system VM template for this one. It is too late in the release process for 4.9.1.0 and 4.10.0.0 to cut new system VM template.
For LTS, yes, we can cut a new system VM template. Any objection to holding this patch over to 4.9.2.0 and 4.10.1.0? I will make it top priority to merge once the release branches are unfrozen.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74250104
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
--- End diff --
Do you design this class for inheritance? If not, declare it ``final``?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-17
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r63848121
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String _type;
--- End diff --
name convention: please use type and refer to this.type
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
LGTM, the tests are all good as one (one failure was due to cleanup in the create template test case and was env related). Based on this I'm 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-218998413
This PR follows up on #985, but this one does not have a libvirt-java dependency.
It only adds the channel in the XML for all Instances and it installs the Qemu Guest Agent inside the SSVM.
This allows is in the future to use the available channel when libvirt-java has the proper support.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220621430
@DaanHoogland yes, I will retest now. Thanks guys...
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74249185
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java ---
@@ -171,6 +174,25 @@ public boolean parseDomainXML(String domXML) {
interfaces.add(def);
}
+ NodeList ports = devices.getElementsByTagName("channel");
+ for (int i = 0; i < ports.getLength(); i++) {
+ Element channel = (Element)ports.item(i);
+
+ String type = channel.getAttribute("type");
+ String path = getAttrValue("source", "path", channel);
+ String name = getAttrValue("target", "name", channel);
+ String state = getAttrValue("target", "state", channel);
+
+ ChannelDef def = null;
+ if (Strings.isNullOrEmpty(state)) {
+ def = new ChannelDef(name, ChannelDef.ChannelType.valueOf(type.toUpperCase()), path);
--- End diff --
What if ``type`` is ``null``? While it likely shouldn't happen, malformed XML could cause an NPE. Consider adding a ``checkState(StringUtils.isNoneBlank(type), "<explanation of unexpected condition>")`` on line 185.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r88377937
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java ---
@@ -175,6 +180,26 @@ public boolean parseDomainXML(String domXML) {
interfaces.add(def);
}
+ NodeList ports = devices.getElementsByTagName("channel");
+ for (int i = 0; i < ports.getLength(); i++) {
+ Element channel = (Element)ports.item(i);
+
+ String type = channel.getAttribute("type");
+ String path = getAttrValue("source", "path", channel);
+ String name = getAttrValue("target", "name", channel);
+ String state = getAttrValue("target", "state", channel);
+
+ ChannelDef def = null;
+ if (!StringUtils.isNotBlank(state)) {
--- End diff --
Minor nit: Performing a logical not operation on the result `StringUtils.isNotBlank` seems a bit awkward. Please consider refactoring to `if (StringUtlis.isBlank(state))`.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r63850026
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String _type;
--- End diff --
also, we can consider dropping the `_` as variable prefix
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Thanks @wido can you also squash 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd Yes, fixed. Forgot to commit that change locally. Pushed a new version.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
This one is still open, are we ready for a merge yet?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan test
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-221166274
I missed that this one was going to need a new systemvmtemplate. Given that I am already over a week late on this release and I have not done any testing or work with changing system vm templates, I am a bit hesitant to go down that road. I am also light on time this week because I have some customer work the next couple days and the CCCNA conference is just around the corner and I am very involved with that as well. Hmmm, not really sure what to do here. I was hoping to merge everything outstanding and freeze in the morning, but this would throw a wrench into that as I really don't know what I am doing to release a new system vm 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74249341
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java ---
@@ -171,6 +174,25 @@ public boolean parseDomainXML(String domXML) {
interfaces.add(def);
}
+ NodeList ports = devices.getElementsByTagName("channel");
+ for (int i = 0; i < ports.getLength(); i++) {
+ Element channel = (Element)ports.item(i);
+
+ String type = channel.getAttribute("type");
+ String path = getAttrValue("source", "path", channel);
+ String name = getAttrValue("target", "name", channel);
+ String state = getAttrValue("target", "state", channel);
+
+ ChannelDef def = null;
+ if (Strings.isNullOrEmpty(state)) {
+ def = new ChannelDef(name, ChannelDef.ChannelType.valueOf(type.toUpperCase()), path);
+ } else {
+ def = new ChannelDef(name, ChannelDef.ChannelType.valueOf(type.toUpperCase()), ChannelDef.ChannelState.valueOf(state.toUpperCase()), path);
--- End diff --
What if ``state`` is ``null``? While it likely shouldn't happen, malformed XML could cause an NPE which will not be helpful to operators or developers trying to debug the issue. Consider adding a ``checkState(StringUtils.isNoneBlank(state), "<explanation of unexpected condition>")`` on line 185.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Packaging result: \u2716centos6 \u2716centos7 \u2716debian. JID-239
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74250378
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
--- End diff --
These attributes are not modified by the class. Why not declare them as ``final``?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido please fix the build failures:
[INFO] Starting audit...
/jenkins/workspace/acs-pr-centos7-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.10.0.0-SNAPSHOT/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java:31:8: error: Unused import - com.cloud.utils.StringUtils.
Audit done.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220365886
Sorry, I need to queue this one up...
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd: Yes, it does. It installs the Qemu Guest Agent into the SSVM.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r63850379
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---
@@ -1978,11 +1978,16 @@ So if getMinSpeed() returns null we fall back to getSpeed().
final SerialDef serial = new SerialDef("pty", null, (short)0);
devices.addDevice(serial);
+ /* Add a VirtIO channel for SystemVMs for communication and provisioning */
if (vmTO.getType() != VirtualMachine.Type.User) {
- final VirtioSerialDef vserial = new VirtioSerialDef(vmTO.getName(), null);
- devices.addDevice(vserial);
+ devices.addDevice(new ChannelDef(vmTO.getName() + ".vport", ChannelDef.ChannelType.UNIX,
+ "/var/lib/libvirt/qemu/" + vmTO.getName() + ".agent"));
}
+ /* Add a VirtIO channel for the Qemu Guest Agent tools */
+ devices.addDevice(new ChannelDef("org.qemu.guest_agent.0", ChannelDef.ChannelType.UNIX,
+ "/var/lib/libvirt/qemu/" + vmTO.getName() + ".org.qemu.guest_agent.0"));
--- End diff --
We're hardcoding channel type `unix` here, is that the default or should we allow users to choose between unix, serial and others? Also, about the guest agent name, while `/var/lib/libvirt/qemu` is the default path on most distros we should make it configurable or make it detect the path based on the env.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Done @rhtyd
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r88378451
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---
@@ -776,6 +779,13 @@ public boolean configure(final String name, final Map<String, Object> params) th
_localStoragePath = "/var/lib/libvirt/images/";
}
+ /* Directory to use for Qemu sockets like for the Qemu Guest Agent */
+ _qemuSocketsPath = new File("/var/lib/libvirt/qemu");
+ String _qemuSocketsPathVar = (String)params.get("qemu.sockets.path");
+ if (!StringUtils.isNotBlank(_qemuSocketsPathVar)) {
--- End diff --
Minor not: A logical not perform on `StringUtils.isNotBlank` is a awkward. Please consider refactoring to `if (StringUtils.isBlank(_qemuSocketsPathVar)`.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74247733
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---
@@ -768,6 +770,12 @@ public boolean configure(final String name, final Map<String, Object> params) th
_localStoragePath = "/var/lib/libvirt/images/";
}
+ /* Directory to use for Qemu sockets like for the Qemu Guest Agent */
+ _qemuSocketsPath = (String)params.get("qemu.sockets.path");
+ if (Strings.isNullOrEmpty(_qemuSocketsPath)) {
--- End diff --
One issue we have found with ``Strings.isNullOrEmpty`` is that it does do a ``trim`` when checking for an empty string. Therefore, a string containing one or more strings is considered non-empty. Therefore, consider using ``StringUtils.isNoneBlank`` if a string containing only whitespace would be a a problem.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido alright, I'll initiate a final test round -- if that passes we can consider merging. /cc @jburwell
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan test
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido based on that feedback, sounds like a good items to get into 4.8.2.0 and 4.9.1.0. Does changing the base branch for this PR work?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-129
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r63848658
--- Diff: plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParserTest.java ---
@@ -144,7 +152,7 @@ public void testDomainXMLParser() {
"</console>" +
"<channel type='unix'>" +
"<source mode='bind' path='/var/lib/libvirt/qemu/s-2970-VM.agent'/>" +
- "<target type='virtio' name='s-2970-VM.vport'/>" +
+ "<target type='virtio' name='s-2970-VM.vport' state='disconnected'/>" +
--- End diff --
to bad the kvm restructure didn't come through. we'd have this in a separate file ;)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r88378691
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
+
+ public ChannelDef(String name, ChannelType type) {
+ this.name = name;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, String path) {
+ this.name = name;
+ this.path = path;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state) {
+ this.name = name;
+ this.state = state;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state, String path) {
+ this.name = name;
+ this.path = path;
+ this.state = state;
+ this.type = type;
+ }
+
+ public ChannelType getChannelType() {
+ return type;
+ }
+
+ public ChannelState getChannelState() {
+ return state;
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public String getPath() {
+ return path;
}
@Override
public String toString() {
StringBuilder virtioSerialBuilder = new StringBuilder();
- if (_path == null) {
- _path = "/var/lib/libvirt/qemu";
+ virtioSerialBuilder.append("<channel type='" + type.toString() + "'>\n");
+ if (path == null) {
+ virtioSerialBuilder.append("<source mode='bind'/>\n");
+ } else {
+ virtioSerialBuilder.append("<source mode='bind' path='" + path + "'/>\n");
}
- virtioSerialBuilder.append("<channel type='unix'>\n");
- virtioSerialBuilder.append("<source mode='bind' path='" + _path + "/" + _name + ".agent'/>\n");
- virtioSerialBuilder.append("<target type='virtio' name='" + _name + ".vport'/>\n");
virtioSerialBuilder.append("<address type='virtio-serial'/>\n");
+ if (state == null) {
+ virtioSerialBuilder.append("<target type='virtio' name='" + name + "'/>\n");
+ } else {
+ virtioSerialBuilder.append("<target type='virtio' name='" + name + "' state='" + state.toString() + "'/>\n");
+ }
--- End diff --
@wido have you had a chance to review this 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan test
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan package
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@jburwell I doubt it will work if I change the branch. Haven't tried that. But it's a new feature, so should this be backported at all?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido thanks
@jburwell
With 4.9/lts we've reproduced many issues around patchviasocket timing out. If this is a better and more robust approach, should we have this fix in 4.9/lts as well? This may require a new systemvmtemplate though. Given our 4.6 based systemvm template is quite old (at least 8-9 months) and there have been security/openssl related updates, does it make sense to generate/publish a new systemvmtemplte for 4.9.1/lts? @wido is there a way we can avoid installation/generation of a new systemvm template package?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan package
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@jburwell Ok, thanks. Why against 4.8? I'm probably missing a point here, but master is where new features go, right?
If I change the base to 4.8 the amount of commits which come in are huge.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-221158751
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 0
Errors: 0
Duration: 4h 25m 18s
```
**Associated Uploads**
**`/tmp/MarvinLogs/DeployDataCenter__May_22_2016_06_23_20_0Q3H4Q:`**
* [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/DeployDataCenter__May_22_2016_06_23_20_0Q3H4Q/dc_entries.obj)
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/DeployDataCenter__May_22_2016_06_23_20_0Q3H4Q/failed_plus_exceptions.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/DeployDataCenter__May_22_2016_06_23_20_0Q3H4Q/runinfo.txt)
**`/tmp/MarvinLogs/test_network_DSGUPF:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_network_DSGUPF/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_network_DSGUPF/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_network_DSGUPF/runinfo.txt)
**`/tmp/MarvinLogs/test_vpc_routers_V97C2K:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_vpc_routers_V97C2K/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_vpc_routers_V97C2K/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_vpc_routers_V97C2K/runinfo.txt)
Uploads will be available until `2016-07-24 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-219152627
@swill Yes, running this in production on our cloud
I just started an Instance and the Agent (backport to 4.8, clean merge) produced this XML:
```
<domain type='kvm'>
<name>i-2-25-VM</name>
<uuid>bd93ca81-d976-41be-823c-6fc0ea013111</uuid>
<description>Ubuntu 12.04 (64-bit)</description>
<cpu mode='host-passthrough'></cpu><sysinfo type='smbios'>
<system>
<entry name='manufacturer'>Apache Software Foundation</entry>
<entry name='product'>CloudStack KVM Hypervisor</entry>
<entry name='uuid'>bd93ca81-d976-41be-823c-6fc0ea013111</entry>
</system>
</sysinfo>
<os>
<type arch='x86_64' machine='pc'>hvm</type>
<boot dev='cdrom'/>
<boot dev='hd'/>
<smbios mode='sysinfo'/>
</os>
<on_reboot>restart</on_reboot>
<on_poweroff>destroy</on_poweroff>
<on_crash>destroy</on_crash>
<memory>4194304</memory>
<devices>
<memballoon model='none'/>
</devices>
<vcpu>2</vcpu>
<features>
<pae/>
<apic/>
<acpi/>
</features>
<cputune>
<shares>1000</shares>
</cputune>
<clock offset='utc'>
<timer name='kvmclock' >
</timer>
</clock>
<devices>
<emulator>/usr/libexec/qemu-kvm</emulator>
<interface type='bridge'>
<source bridge='cloudbr0'/>
<mac address='06:73:12:00:00:67'/>
<model type='virtio'/>
<bandwidth>
<inbound average='128000' peak='128000'/>
<outbound average='128000' peak='128000'/>
</bandwidth>
</interface>
<console type='pty'>
<target port='0'/>
</console>
<disk device='disk' type='network'>
<driver name='qemu' type='raw' cache='writeback' />
<source protocol='rbd' name='rbd-c01-p01/d70d7e6e-753d-4a0e-ad5f-ec8ac531e32b'>
<host name='monitor.ceph.XXXX.XXXX.XXXXX.nl' port='6789'/>
</source>
<auth username='cloudstack'>
<secret type='ceph' uuid='5e34fd37-1763-3092-9f51-280a31bdb36f'/>
</auth>
<target dev='vda' bus='virtio'/>
<serial>d70d7e6e753d4a0ead5f</serial></disk>
<disk device='disk' type='network'>
<driver name='qemu' type='raw' cache='writeback' />
<source protocol='rbd' name='rbd-c01-p01/3fcb9970-cf63-4251-b1b9-2d916f6ab1c6'>
<host name='monitor.ceph.XXXXX.XXXXXX.XXXXXX.nl' port='6789'/>
</source>
<auth username='cloudstack'>
<secret type='ceph' uuid='5e34fd37-1763-3092-9f51-280a31bdb36f'/>
</auth>
<target dev='vdb' bus='virtio'/>
<serial>3fcb9970cf634251b1b9</serial></disk>
<disk device='cdrom' type='file'>
<driver name='qemu' type='raw' cache='none' />
<source file=''/>
<target dev='hdc' bus='ide'/>
</disk>
<serial type='pty'>
<target port='0'/>
</serial>
<graphics type='vnc' autoport='yes' listen='10.5.0.106' passwd='XXXXXXXX'/>
<channel type='unix'>
<source mode='bind' path='/var/lib/libvirt/qemu/i-2-25-VM.org.qemu.guest_agent.0'/>
<address type='virtio-serial'/>
<target type='virtio' name='org.qemu.guest_agent.0'/>
</channel>
<input type='tablet' bus='usb'/>
</devices>
</domain>
```
There you see this channel added:
```
<channel type='unix'>
<source mode='bind' path='/var/lib/libvirt/qemu/i-2-25-VM.org.qemu.guest_agent.0'/>
<address type='virtio-serial'/>
<target type='virtio' name='org.qemu.guest_agent.0'/>
</channel>
```
In the Ubuntu 14.04 the Guest Agent is running:
<pre>
root@vhosting-relay:~# ps aux|grep qemu
root 2789 0.0 0.0 17468 700 ? Ss 22:19 0:00 /usr/sbin/qemu-ga --daemonize -m virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent.0
root 2890 0.0 0.0 10460 908 pts/6 S+ 22:20 0:00 grep qemu
root@vhosting-relay:~#
</pre>
From the HV I can send commands through libvirt:
<pre>
[root@n06 ~]# virsh qemu-agent-command i-2-25-VM '{"execute":"guest-ping"}'
{"return":{}}
[root@n06 ~]#
</pre>
I also re-started a Console Proxy which has a additional channel, that works as well:
```
<channel type='unix'>
<source mode='bind' path='/var/lib/libvirt/qemu/v-215-VM.agent'/>
<address type='virtio-serial'/>
<target type='virtio' name='v-215-VM.vport'/>
</channel>
<channel type='unix'>
<source mode='bind' path='/var/lib/libvirt/qemu/v-215-VM.org.qemu.guest_agent.0'/>
<address type='virtio-serial'/>
<target type='virtio' name='org.qemu.guest_agent.0'/>
</channel>
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220512458
### CI RESULTS
```
Tests Run: 78
Skipped: 0
Failed: 0
Errors: 4
Duration: 7h 47m 49s
```
**Summary of the problem(s):**
```
ERROR: test suite for <class 'integration.smoke.test_internal_lb.TestInternalLb'>
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
self.setUp()
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
self.setupContext(ancestor)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
try_run(context, names)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
return func()
File "/data/git/cs1/cloudstack/test/integration/smoke/test_internal_lb.py", line 296, in setUpClass
cls.template.download(cls.apiclient)
File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_YJVJO9/results.txt
```
```
ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestRVPCSite2SiteVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
self.setUp()
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
self.setupContext(ancestor)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
try_run(context, names)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
return func()
File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 835, in setUpClass
cls.template.download(cls.apiclient)
File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_YJVJO9/results.txt
```
```
ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestVpcRemoteAccessVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
self.setUp()
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
self.setupContext(ancestor)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
try_run(context, names)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
return func()
File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 293, in setUpClass
cls.template.download(cls.apiclient)
File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_YJVJO9/results.txt
```
```
ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestVpcSite2SiteVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
self.setUp()
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
self.setupContext(ancestor)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
try_run(context, names)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
return func()
File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 472, in setUpClass
cls.template.download(cls.apiclient)
File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_YJVJO9/results.txt
```
**Associated Uploads**
**`/tmp/MarvinLogs/DeployDataCenter__May_19_2016_19_37_29_UK7LQG:`**
* [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/DeployDataCenter__May_19_2016_19_37_29_UK7LQG/dc_entries.obj)
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/DeployDataCenter__May_19_2016_19_37_29_UK7LQG/failed_plus_exceptions.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/DeployDataCenter__May_19_2016_19_37_29_UK7LQG/runinfo.txt)
**`/tmp/MarvinLogs/test_network_YJVJO9:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_network_YJVJO9/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_network_YJVJO9/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_network_YJVJO9/runinfo.txt)
**`/tmp/MarvinLogs/test_vpc_routers_NDLJQN:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_vpc_routers_NDLJQN/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_vpc_routers_NDLJQN/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1545/tmp/MarvinLogs/test_vpc_routers_NDLJQN/runinfo.txt)
Uploads will be available until `2016-07-20 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@jburwell I worked through almost all your comments and fixed what could be done.
I think this one is good to go into 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r63847694
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---
@@ -1978,11 +1978,16 @@ So if getMinSpeed() returns null we fall back to getSpeed().
final SerialDef serial = new SerialDef("pty", null, (short)0);
devices.addDevice(serial);
+ /* Add a VirtIO channel for SystemVMs for communication and provisioning */
if (vmTO.getType() != VirtualMachine.Type.User) {
- final VirtioSerialDef vserial = new VirtioSerialDef(vmTO.getName(), null);
- devices.addDevice(vserial);
+ devices.addDevice(new ChannelDef(vmTO.getName() + ".vport", ChannelDef.ChannelType.UNIX,
+ "/var/lib/libvirt/qemu/" + vmTO.getName() + ".agent"));
--- End diff --
these names and paths should not be hardcoded here.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74248311
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---
@@ -1978,11 +1986,16 @@ So if getMinSpeed() returns null we fall back to getSpeed().
final SerialDef serial = new SerialDef("pty", null, (short)0);
devices.addDevice(serial);
+ /* Add a VirtIO channel for SystemVMs for communication and provisioning */
if (vmTO.getType() != VirtualMachine.Type.User) {
- final VirtioSerialDef vserial = new VirtioSerialDef(vmTO.getName(), null);
- devices.addDevice(vserial);
+ devices.addDevice(new ChannelDef(vmTO.getName() + ".vport", ChannelDef.ChannelType.UNIX,
+ _qemuSocketsPath + "/" + vmTO.getName() + ".agent"));
}
+ /* Add a VirtIO channel for the Qemu Guest Agent tools */
+ devices.addDevice(new ChannelDef("org.qemu.guest_agent.0", ChannelDef.ChannelType.UNIX,
--- End diff --
The ``org.qemu.guest_agent.0`` value is used in multiple places and appears to be a magic value. Please consider extracting to a constant and/or a parameter passed into the method if this value could change based on context.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan test
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74248622
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---
@@ -244,6 +245,7 @@
protected long _diskActivityCheckFileSizeMin = 10485760; // 10MB
protected int _diskActivityCheckTimeoutSeconds = 120; // 120s
protected long _diskActivityInactiveThresholdMilliseconds = 30000; // 30s
+ protected String _qemuSocketsPath;
--- End diff --
Why not represent this value as a ``File`` instead of a ``String``? The ``File`` class provides semantics for path construction, as well as, providing stronger type information. Also, why is it declared ``protected`` and not ``private``?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@jburwell a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests against packages at http://packages.shapeblue.com/cloudstack/pr/1545
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido I misunderstood. I thought was fixing one or more bugs. Given that it is an enhancement, you are correct that it should only target master. As soon as we iron the smoke test issues in #1692, we will enqueue this PR for testing on KVM and get it merged for 4.10.0.0.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220512533
These failures are not related to this PR, but if you have ideas why they are happening I would be interested. I will wait till @wido has a chance to work on this PR to run the CI again. Thx...
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74250260
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
+
+ public ChannelDef(String name, ChannelType type) {
+ this.name = name;
+ this.type = type;
--- End diff --
Why not delegate to ``this(String name, ChannelType type, ChannelState state, String path)``?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-219168754
ok, thank you sir. \U0001f44d
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido I will re-review the code later today. Could you change the base branch to 4.8 (these [instructions](https://github.com/blog/2224-change-the-base-branch-of-a-pull-request) explain how to change the base branch without opening a new PR)?
We also need a test LGTM. I am going to kick blueorangutan to package and test it. Are there any additional component tests that should be run?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido can you change the base back to master for now, @jburwell to use this we'll need to build/publish new systemvmtemplate. Is that feasible for the LTS release? If so, we'll need to make changes in the upgrade paths. This is why I'm not in favour of getting this on 4.8 branch, we can discuss whether it's solid enough to include in 4.9 for lts release. In case we accept this in 4.9, then we've to build a new systemvm template in which case I would also want to include the strongswan PR (so we dont' have to rebuild another systemvmtemplate within next month/weeks for 4.10/4.11+)?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220275302
apart from some code style and convention things 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r63849851
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java ---
@@ -1978,11 +1978,16 @@ So if getMinSpeed() returns null we fall back to getSpeed().
final SerialDef serial = new SerialDef("pty", null, (short)0);
devices.addDevice(serial);
+ /* Add a VirtIO channel for SystemVMs for communication and provisioning */
if (vmTO.getType() != VirtualMachine.Type.User) {
- final VirtioSerialDef vserial = new VirtioSerialDef(vmTO.getName(), null);
- devices.addDevice(vserial);
+ devices.addDevice(new ChannelDef(vmTO.getName() + ".vport", ChannelDef.ChannelType.UNIX,
+ "/var/lib/libvirt/qemu/" + vmTO.getName() + ".agent"));
--- End diff --
agree, @wido do you mind moving them something configurable via global settings or agent.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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r88378575
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
+
+ public ChannelDef(String name, ChannelType type) {
+ this.name = name;
+ this.type = type;
--- End diff --
@wido have you had a chance to consider this 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74248818
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java ---
@@ -171,6 +174,25 @@ public boolean parseDomainXML(String domXML) {
interfaces.add(def);
}
+ NodeList ports = devices.getElementsByTagName("channel");
+ for (int i = 0; i < ports.getLength(); i++) {
+ Element channel = (Element)ports.item(i);
+
+ String type = channel.getAttribute("type");
+ String path = getAttrValue("source", "path", channel);
+ String name = getAttrValue("target", "name", channel);
+ String state = getAttrValue("target", "state", channel);
+
+ ChannelDef def = null;
+ if (Strings.isNullOrEmpty(state)) {
--- End diff --
Please see previous comment about whitespace sensitivity in ``String.isNullorEmpty``.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220263263
@rhtyd and @DaanHoogland Could you maybe take a quick look at this one? We could finally get the first part of this in.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220281463
@swill, did this pass the (not so C)I?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-219168529
@swill I added a small fix for inside the SSVM. During some additional manual testing I found that the SSVM is picky in the order in the XML definition.
My second commit makes sure it doesn't use the first VirtIO port, but uses the proper port.
I tested that fix in production as well, works just fine.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the issue:
https://github.com/apache/cloudstack/pull/1545
### ACS CI BVT Run
**Sumarry:**
Build Number 196
Hypervisor xenserver
NetworkType Advanced
Passed=71
Failed=2
Skipped=3
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* test_vpc_vpn.py
* ContextSuite context=TestRVPCSite2SiteVpn>:setup Failing since 21 runs
* ContextSuite context=TestVpcRemoteAccessVpn>:setup Failing since 21 runs
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-221158910
This is coming back clean. Can I get a second LGTM? Maybe @rhtyd?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@wido can you confirm if you've fixed the NPE issue I've shared in the comment above?
java.lang.NullPointerException
at java.io.File.(File.java:277)
at com.cloud.hypervisor.kvm.resource.LibvirtComputingResource.configure(LibvirtComputingResource.java:786)
at com.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220342553
Thanks for the feedback, I will work on that tomorrow.
@rhtyd I tested this on both Ubuntu and CentOS systems.
The main issue with Ubuntu is AppArmor where libvirt isn't allowed to write in /var/lib/qemu.
I'll make that directory configurable, but keep in mind that on all platforms I know that is the directory where the sockets are stored.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-240
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74250473
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
--- End diff --
Does the ``path`` attribute represent a file path? If so, why not represent it as a ``File``?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74251286
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
+
+ public ChannelDef(String name, ChannelType type) {
+ this.name = name;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, String path) {
+ this.name = name;
+ this.path = path;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state) {
+ this.name = name;
+ this.state = state;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state, String path) {
+ this.name = name;
+ this.path = path;
+ this.state = state;
+ this.type = type;
+ }
+
+ public ChannelType getChannelType() {
+ return type;
+ }
+
+ public ChannelState getChannelState() {
+ return state;
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public String getPath() {
+ return path;
}
@Override
public String toString() {
StringBuilder virtioSerialBuilder = new StringBuilder();
- if (_path == null) {
- _path = "/var/lib/libvirt/qemu";
+ virtioSerialBuilder.append("<channel type='" + type.toString() + "'>\n");
+ if (path == null) {
+ virtioSerialBuilder.append("<source mode='bind'/>\n");
+ } else {
+ virtioSerialBuilder.append("<source mode='bind' path='" + path + "'/>\n");
--- End diff --
``<source mode=`bind`" is duplicated between both if conditions. Furthermore, the else appears to be a superset of the default case. Therefore, please consider refactoring as follows:
```
virtioSerialBuilder.append("<source mode='bind'");
if (path != null) {
virtioSerialBuilder.append("path='");
virtioSerialBuilder.append(path);
virtioSerialBuilder.append("'");
}
virtioSerialBuilder.append("/>");
virtioSerialBuilder.append(System.lineSeparator());
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
I think we can change base to 4.9 as the amount of conflicts will be minimal. At this point, it would add a lot of overhead to support 4.8, 4.9, master (4.10) simultaneous. As per our agreed guideline, we have been maintaining only two trees at a given times -- the master and the latest/last release branch (n, n+1, i.e. 4.9 and 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Thanks @wido
@blueorangutan package
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan package
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1545
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan test
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r88378602
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
+
+ public ChannelDef(String name, ChannelType type) {
+ this.name = name;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, String path) {
+ this.name = name;
+ this.path = path;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state) {
--- End diff --
@wido have you had a chance to review this commit?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74249850
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java ---
@@ -234,6 +256,10 @@ public Integer getVncPort() {
return diskDefs;
}
+ public List<ChannelDef> getChannels() {
+ return channels;
--- End diff --
Please make a defensive copy of the list before returning to avoid side-effects on this class by potential mutations of the list by the caller. Ideally, return it as a ``Collections.unmodifiableList(channels)``.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@blueorangutan package
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@jburwell and @rhtyd: Done, conflicts have been resolved!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-18
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1545: CLOUDSTACK-8715: Add channel to Instances for...
Posted by jburwell <gi...@git.apache.org>.
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1545#discussion_r74250288
--- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java ---
@@ -1209,25 +1209,95 @@ public String toString() {
}
}
- public static class VirtioSerialDef {
- private final String _name;
- private String _path;
+ public static class ChannelDef {
+ enum ChannelType {
+ UNIX("unix"), SERIAL("serial");
+ String type;
- public VirtioSerialDef(String name, String path) {
- _name = name;
- _path = path;
+ ChannelType(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return this.type;
+ }
+ }
+
+ enum ChannelState {
+ DISCONNECTED("disconnected"), CONNECTED("connected");
+ String type;
+
+ ChannelState(String type) {
+ this.type = type;
+ }
+
+ @Override
+ public String toString() {
+ return type;
+ }
+ }
+
+ private String name;
+ private String path;
+ private ChannelType type;
+ private ChannelState state;
+
+ public ChannelDef(String name, ChannelType type) {
+ this.name = name;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, String path) {
+ this.name = name;
+ this.path = path;
+ this.type = type;
+ }
+
+ public ChannelDef(String name, ChannelType type, ChannelState state) {
--- End diff --
Why not delegate to ``this(String name, ChannelType type, ChannelState state, String path)``?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1545
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-219
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-220440726
@swill I will work on the changes tomorrow morning (my time, CET). Shouldn't be very hard to do.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8715: Add channel to Instances...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1545#issuecomment-221165022
LGTM, to use this feature. KVM users will need to use new systemvmtemplates, can we get #1531 merged as well? /cc @swill
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1545: CLOUDSTACK-8715: Add channel to Instances for Qemu G...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the issue:
https://github.com/apache/cloudstack/pull/1545
@jburwell The old systemVM will also work, but that assumes it's serial port is *always* the first port attached.
The XML we generate will make sure that the Qemu Guest Agent port is the second one, so the old SSVM template will still work. I run this patch in production with 4.9 without a modified SSVM, works fine.
The changes in the SSVM are just there to make it more robust against such 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---