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 <[email protected]> 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 <[email protected]>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 <[email protected]> >> 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" <[email protected]> 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.
