You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Dave Cahill <dc...@midokura.jp> on 2012/10/29 10:20:39 UTC
Behavior with regard to private NICs (KVM)
Hi all,
>From LibvirtComputingResource.java, we can see that the CloudStack agent
fails to start if there is no "private" NIC name set.
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
698 getPifs();
699 if (_pifs.get("private") == null) {
700 s_logger.debug("Failed to get private nic name");
701 throw new ConfigurationException("Failed to get private
nic name");
702 }
In other words, the agent fails to start if the entry for "private" in the
HashMap returned by getPifs() is null.
A recent commit [1] seems to have changed the behavior of getPifs() so that
this happens more often. The new code has two behaviors which seem strange
to me:
1. The value of the "private" entry is effectively set to the interface of
the *last* bridge in the list returned by "brctl show".
2. A single bridge can only be used to set either the "public" or the
"private" value of the HashMap.
This means that the getPifs() HashMap entry for "private" is null (causing
the agent to fail to start) in two situations which seem normal:
1. A bridge with no interfaces defined exists on the system.
For example, libvirt creates a bridge called virbr0 by default, which has
no interfaces. When getPifs() sees this entry, it counts it as null. If
this entry is the last one in the list, "private" is set to null.
2. The private and public bridge are the same.
>From my reading of the previous code, if _guestBridgeName and
_publicBridgeName were set to the same value (only one bridge defined), the
entries in _pifs for "public" and "private" would be the same, and the
agent would start correctly. However, the new code loops through the
bridges, setting them as the value of *either* public or private (or
neither), so if there is only one bridge available, the "private" entry in
pifs will be null.
I'm just coming up to speed with the code base, so there may easily be a
good reason for this behavior change. Does anyone have an explanation?
Thanks,
Dave Cahill.
[1]
https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=915babd9
Re: Behavior with regard to private NICs (KVM)
Posted by Marcus Sorensen <sh...@gmail.com>.
No problem. My philosophy is that it's more important to be responsive
and eager to fix your mistakes than it is to be absolutely perfect.
I'll never be the latter, so I might as well strive for the former :-)
I'm just glad I caught it in the flood of my mailbox.
The fix is commit 843e14085807cd4e54246093f73b923e757f722d in master.
On Mon, Oct 29, 2012 at 8:27 PM, Dave Cahill <dc...@midokura.jp> wrote:
> Hi Marcus,
>
> Thanks for the quick reply.
>
> Your suggested code change solves both issues from my point of view, and
> returns the "choosing of public and private NICs" behavior to what it was
> prior to the commit I mentioned in my mail.
>
> I submitted the change to reviewboard and added yourself and Edison Su
> (reviewer from the previous commit) as reviewers:
> https://reviews.apache.org/r/7773/
>
> Any questions, just let me know.
>
> Thanks again for the quick response,
> Dave.
>
> On Mon, Oct 29, 2012 at 11:31 PM, Marcus Sorensen <sh...@gmail.com>wrote:
>
>> Ok, with #1 I was hung up on your 'bridge with no interfaces' example,
>> which isn't the issue. As an aside, n my CentOS 6.3 and Ubuntu 12.04
>> test sytems, the default libvirt virbr0 does have an auto-configured
>> interface, 'vnet0'. And if there are no interfaces on a bridge, then
>> getPifs should treat it as null, since it's goal is to get interfaces.
>> So I was failing to see how the new code was different in that regard,
>> however it just creates a side effect, and is not your point.
>>
>> Looking at getPifs(), if we change:
>>
>> if(_publicBridgeName != null &&
>> bridge.equals(_publicBridgeName)){
>> _pifs.put("public", pif);
>> } else if (_guestBridgeName != null) {
>> _pifs.put("private", pif);
>> }
>>
>> to
>>
>> if(_publicBridgeName != null &&
>> bridge.equals(_publicBridgeName)){
>> _pifs.put("public", pif);
>> }
>> if (_guestBridgeName != null &&
>> bridge.equals(_guestBridgeName)) {
>> _pifs.put("private", pif);
>> }
>>
>> does that solve the issue?
>>
>> On Mon, Oct 29, 2012 at 7:30 AM, Marcus Sorensen <sh...@gmail.com>
>> wrote:
>> > I'll take a look and address the concerns later today. #2 I can see
>> > immediately as its an if-else, but I don't see how the change affected #1
>> > from the previous behavior, just looking on my phone.
>> >
>> > On Oct 29, 2012 3:21 AM, "Dave Cahill" <dc...@midokura.jp> wrote:
>> >>
>> >> Hi all,
>> >>
>> >> From LibvirtComputingResource.java, we can see that the CloudStack agent
>> >> fails to start if there is no "private" NIC name set.
>> >>
>> >>
>> >>
>> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
>> >>
>> >> 698 getPifs();
>> >> 699 if (_pifs.get("private") == null) {
>> >> 700 s_logger.debug("Failed to get private nic name");
>> >> 701 throw new ConfigurationException("Failed to get private
>> >> nic name");
>> >> 702 }
>> >>
>> >> In other words, the agent fails to start if the entry for "private" in
>> the
>> >> HashMap returned by getPifs() is null.
>> >>
>> >> A recent commit [1] seems to have changed the behavior of getPifs() so
>> >> that
>> >> this happens more often. The new code has two behaviors which seem
>> strange
>> >> to me:
>> >>
>> >> 1. The value of the "private" entry is effectively set to the interface
>> of
>> >> the *last* bridge in the list returned by "brctl show".
>> >> 2. A single bridge can only be used to set either the "public" or the
>> >> "private" value of the HashMap.
>> >>
>> >> This means that the getPifs() HashMap entry for "private" is null
>> (causing
>> >> the agent to fail to start) in two situations which seem normal:
>> >>
>> >> 1. A bridge with no interfaces defined exists on the system.
>> >> For example, libvirt creates a bridge called virbr0 by default, which
>> has
>> >> no interfaces. When getPifs() sees this entry, it counts it as null. If
>> >> this entry is the last one in the list, "private" is set to null.
>> >>
>> >> 2. The private and public bridge are the same.
>> >> From my reading of the previous code, if _guestBridgeName and
>> >> _publicBridgeName were set to the same value (only one bridge defined),
>> >> the
>> >> entries in _pifs for "public" and "private" would be the same, and the
>> >> agent would start correctly. However, the new code loops through the
>> >> bridges, setting them as the value of *either* public or private (or
>> >> neither), so if there is only one bridge available, the "private" entry
>> in
>> >> pifs will be null.
>> >>
>> >> I'm just coming up to speed with the code base, so there may easily be a
>> >> good reason for this behavior change. Does anyone have an explanation?
>> >>
>> >> Thanks,
>> >> Dave Cahill.
>> >>
>> >>
>> >> [1]
>> >>
>> >>
>> https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=915babd9
>>
>
>
>
> --
> Thanks,
> Dave.
Re: Behavior with regard to private NICs (KVM)
Posted by Dave Cahill <dc...@midokura.jp>.
Hi Marcus,
Thanks for the quick reply.
Your suggested code change solves both issues from my point of view, and
returns the "choosing of public and private NICs" behavior to what it was
prior to the commit I mentioned in my mail.
I submitted the change to reviewboard and added yourself and Edison Su
(reviewer from the previous commit) as reviewers:
https://reviews.apache.org/r/7773/
Any questions, just let me know.
Thanks again for the quick response,
Dave.
On Mon, Oct 29, 2012 at 11:31 PM, Marcus Sorensen <sh...@gmail.com>wrote:
> Ok, with #1 I was hung up on your 'bridge with no interfaces' example,
> which isn't the issue. As an aside, n my CentOS 6.3 and Ubuntu 12.04
> test sytems, the default libvirt virbr0 does have an auto-configured
> interface, 'vnet0'. And if there are no interfaces on a bridge, then
> getPifs should treat it as null, since it's goal is to get interfaces.
> So I was failing to see how the new code was different in that regard,
> however it just creates a side effect, and is not your point.
>
> Looking at getPifs(), if we change:
>
> if(_publicBridgeName != null &&
> bridge.equals(_publicBridgeName)){
> _pifs.put("public", pif);
> } else if (_guestBridgeName != null) {
> _pifs.put("private", pif);
> }
>
> to
>
> if(_publicBridgeName != null &&
> bridge.equals(_publicBridgeName)){
> _pifs.put("public", pif);
> }
> if (_guestBridgeName != null &&
> bridge.equals(_guestBridgeName)) {
> _pifs.put("private", pif);
> }
>
> does that solve the issue?
>
> On Mon, Oct 29, 2012 at 7:30 AM, Marcus Sorensen <sh...@gmail.com>
> wrote:
> > I'll take a look and address the concerns later today. #2 I can see
> > immediately as its an if-else, but I don't see how the change affected #1
> > from the previous behavior, just looking on my phone.
> >
> > On Oct 29, 2012 3:21 AM, "Dave Cahill" <dc...@midokura.jp> wrote:
> >>
> >> Hi all,
> >>
> >> From LibvirtComputingResource.java, we can see that the CloudStack agent
> >> fails to start if there is no "private" NIC name set.
> >>
> >>
> >>
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
> >>
> >> 698 getPifs();
> >> 699 if (_pifs.get("private") == null) {
> >> 700 s_logger.debug("Failed to get private nic name");
> >> 701 throw new ConfigurationException("Failed to get private
> >> nic name");
> >> 702 }
> >>
> >> In other words, the agent fails to start if the entry for "private" in
> the
> >> HashMap returned by getPifs() is null.
> >>
> >> A recent commit [1] seems to have changed the behavior of getPifs() so
> >> that
> >> this happens more often. The new code has two behaviors which seem
> strange
> >> to me:
> >>
> >> 1. The value of the "private" entry is effectively set to the interface
> of
> >> the *last* bridge in the list returned by "brctl show".
> >> 2. A single bridge can only be used to set either the "public" or the
> >> "private" value of the HashMap.
> >>
> >> This means that the getPifs() HashMap entry for "private" is null
> (causing
> >> the agent to fail to start) in two situations which seem normal:
> >>
> >> 1. A bridge with no interfaces defined exists on the system.
> >> For example, libvirt creates a bridge called virbr0 by default, which
> has
> >> no interfaces. When getPifs() sees this entry, it counts it as null. If
> >> this entry is the last one in the list, "private" is set to null.
> >>
> >> 2. The private and public bridge are the same.
> >> From my reading of the previous code, if _guestBridgeName and
> >> _publicBridgeName were set to the same value (only one bridge defined),
> >> the
> >> entries in _pifs for "public" and "private" would be the same, and the
> >> agent would start correctly. However, the new code loops through the
> >> bridges, setting them as the value of *either* public or private (or
> >> neither), so if there is only one bridge available, the "private" entry
> in
> >> pifs will be null.
> >>
> >> I'm just coming up to speed with the code base, so there may easily be a
> >> good reason for this behavior change. Does anyone have an explanation?
> >>
> >> Thanks,
> >> Dave Cahill.
> >>
> >>
> >> [1]
> >>
> >>
> https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=915babd9
>
--
Thanks,
Dave.
Re: Behavior with regard to private NICs (KVM)
Posted by Marcus Sorensen <sh...@gmail.com>.
Ok, with #1 I was hung up on your 'bridge with no interfaces' example,
which isn't the issue. As an aside, n my CentOS 6.3 and Ubuntu 12.04
test sytems, the default libvirt virbr0 does have an auto-configured
interface, 'vnet0'. And if there are no interfaces on a bridge, then
getPifs should treat it as null, since it's goal is to get interfaces.
So I was failing to see how the new code was different in that regard,
however it just creates a side effect, and is not your point.
Looking at getPifs(), if we change:
if(_publicBridgeName != null && bridge.equals(_publicBridgeName)){
_pifs.put("public", pif);
} else if (_guestBridgeName != null) {
_pifs.put("private", pif);
}
to
if(_publicBridgeName != null && bridge.equals(_publicBridgeName)){
_pifs.put("public", pif);
}
if (_guestBridgeName != null && bridge.equals(_guestBridgeName)) {
_pifs.put("private", pif);
}
does that solve the issue?
On Mon, Oct 29, 2012 at 7:30 AM, Marcus Sorensen <sh...@gmail.com> wrote:
> I'll take a look and address the concerns later today. #2 I can see
> immediately as its an if-else, but I don't see how the change affected #1
> from the previous behavior, just looking on my phone.
>
> On Oct 29, 2012 3:21 AM, "Dave Cahill" <dc...@midokura.jp> wrote:
>>
>> Hi all,
>>
>> From LibvirtComputingResource.java, we can see that the CloudStack agent
>> fails to start if there is no "private" NIC name set.
>>
>>
>> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
>>
>> 698 getPifs();
>> 699 if (_pifs.get("private") == null) {
>> 700 s_logger.debug("Failed to get private nic name");
>> 701 throw new ConfigurationException("Failed to get private
>> nic name");
>> 702 }
>>
>> In other words, the agent fails to start if the entry for "private" in the
>> HashMap returned by getPifs() is null.
>>
>> A recent commit [1] seems to have changed the behavior of getPifs() so
>> that
>> this happens more often. The new code has two behaviors which seem strange
>> to me:
>>
>> 1. The value of the "private" entry is effectively set to the interface of
>> the *last* bridge in the list returned by "brctl show".
>> 2. A single bridge can only be used to set either the "public" or the
>> "private" value of the HashMap.
>>
>> This means that the getPifs() HashMap entry for "private" is null (causing
>> the agent to fail to start) in two situations which seem normal:
>>
>> 1. A bridge with no interfaces defined exists on the system.
>> For example, libvirt creates a bridge called virbr0 by default, which has
>> no interfaces. When getPifs() sees this entry, it counts it as null. If
>> this entry is the last one in the list, "private" is set to null.
>>
>> 2. The private and public bridge are the same.
>> From my reading of the previous code, if _guestBridgeName and
>> _publicBridgeName were set to the same value (only one bridge defined),
>> the
>> entries in _pifs for "public" and "private" would be the same, and the
>> agent would start correctly. However, the new code loops through the
>> bridges, setting them as the value of *either* public or private (or
>> neither), so if there is only one bridge available, the "private" entry in
>> pifs will be null.
>>
>> I'm just coming up to speed with the code base, so there may easily be a
>> good reason for this behavior change. Does anyone have an explanation?
>>
>> Thanks,
>> Dave Cahill.
>>
>>
>> [1]
>>
>> https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=915babd9
Re: Behavior with regard to private NICs (KVM)
Posted by Marcus Sorensen <sh...@gmail.com>.
I'll take a look and address the concerns later today. #2 I can see
immediately as its an if-else, but I don't see how the change affected #1
from the previous behavior, just looking on my phone.
On Oct 29, 2012 3:21 AM, "Dave Cahill" <dc...@midokura.jp> wrote:
> Hi all,
>
> From LibvirtComputingResource.java, we can see that the CloudStack agent
> fails to start if there is no "private" NIC name set.
>
>
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
>
> 698 getPifs();
> 699 if (_pifs.get("private") == null) {
> 700 s_logger.debug("Failed to get private nic name");
> 701 throw new ConfigurationException("Failed to get private
> nic name");
> 702 }
>
> In other words, the agent fails to start if the entry for "private" in the
> HashMap returned by getPifs() is null.
>
> A recent commit [1] seems to have changed the behavior of getPifs() so that
> this happens more often. The new code has two behaviors which seem strange
> to me:
>
> 1. The value of the "private" entry is effectively set to the interface of
> the *last* bridge in the list returned by "brctl show".
> 2. A single bridge can only be used to set either the "public" or the
> "private" value of the HashMap.
>
> This means that the getPifs() HashMap entry for "private" is null (causing
> the agent to fail to start) in two situations which seem normal:
>
> 1. A bridge with no interfaces defined exists on the system.
> For example, libvirt creates a bridge called virbr0 by default, which has
> no interfaces. When getPifs() sees this entry, it counts it as null. If
> this entry is the last one in the list, "private" is set to null.
>
> 2. The private and public bridge are the same.
> From my reading of the previous code, if _guestBridgeName and
> _publicBridgeName were set to the same value (only one bridge defined), the
> entries in _pifs for "public" and "private" would be the same, and the
> agent would start correctly. However, the new code loops through the
> bridges, setting them as the value of *either* public or private (or
> neither), so if there is only one bridge available, the "private" entry in
> pifs will be null.
>
> I'm just coming up to speed with the code base, so there may easily be a
> good reason for this behavior change. Does anyone have an explanation?
>
> Thanks,
> Dave Cahill.
>
>
> [1]
>
> https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=915babd9
>