incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marcus Sorensen <shadow...@gmail.com>
Subject Re: Behavior with regard to private NICs (KVM)
Date Tue, 30 Oct 2012 05:18:02 GMT
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 <dcahill@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 <shadowsor@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 <shadowsor@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" <dcahill@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.

Mime
View raw message