That wasn't the patch I thought it was. Regarding 5e80e5d33d9a295b91cdba9377f52d9d963d802a, we should probably do that for IpAssocCommand as well. I'm not sure we have the db fix in 4.3 yet, and so a fix like this would be required for IpAssocCommand (and perhaps other unfound things).
On Tue, Jun 3, 2014 at 3:22 PM, Marcus <shadow...@gmail.com> wrote: > Hmm.. ok. I guess we can apply the bandaid patch as well > > > On Tue, Jun 3, 2014 at 12:16 PM, Edison Su <edison...@citrix.com> wrote: > >> I checked in a commit: 5e80e5d33d9a295b91cdba9377f52d9d963d802a, which >> will fix some of the mess of vlan id. >> >> > -----Original Message----- >> > From: Marcus [mailto:shadow...@gmail.com] >> > Sent: Tuesday, June 03, 2014 9:57 AM >> > To: Daan Hoogland >> > Cc: dev >> > Subject: Re: VPC's VR missing public NIC eth1 >> > >> > Ok, thanks. It seems there are other cases where the Command being >> > passed from the mgmt server has inconsistent broadcastUri as well, this >> > should blanket fix them. In the meantime there's a growing group of 4.3 >> > upgraders who are getting pitchforks out over at CLOUDSTACK-6464, so we >> > may want to have something in 4.3.1 too. >> > >> > >> > On Tue, Jun 3, 2014 at 12:30 AM, Daan Hoogland <daan.hoogl...@gmail.com >> > >> > wrote: >> > >> > > one clarification, I was not suggesting changing vlan://x back to x, >> > > just the case where x==untagged. I had a little analog discussion with >> > > Hugo and he convinced me that untagged has no special meaning in SDN >> > > cases, maybe for vxlan. So the problem I saw is at least smaller then >> > > in my mind. >> > > >> > > I have committed the db change to update 4.3.0 to 4.4.0. It will need >> > > heavy testing. And I didn't extensively look into other tables that >> > > need such a change. networks is the likely candidate but there may be >> > > others. >> > > >> > > >> > > On Mon, Jun 2, 2014 at 6:38 PM, Marcus <shadow...@gmail.com> wrote: >> > > > Just to recap... I was trying to review the issue in my head and >> > > > thought >> > > it >> > > > might be useful to write it down. >> > > > >> > > > in 4.3 we got the BroadcastDomainType enum introduced, and many >> > > > parts of >> > > the >> > > > code were changed to use that when dealing with the vlan id. This >> > > > code, among other things, returns a vlan id in URI format, >> > > > describing both the technology used to provide the virtual lan, >> > > > along with the id. Along the >> > > way >> > > > this seems to have caused the value itself to be stored as a URI >> > > > (still >> > > not >> > > > sure where, by whom, or if it was intentional). That was fine and >> > > > seemed >> > > to >> > > > work after some fixing, until there was an upgrade done where the >> > > existing >> > > > database value was NOT in URI format. We had a few places where the >> > > > code >> > > was >> > > > never changed to use BroadcastDomainType to 'normalize' the info >> > > > from the database (e.g. the IpAssocVpcCommand the mgmt server >> > > > constructs), so upgrades are broken. >> > > > >> > > > Most places in the code as it is now are working with a live value >> > > > of 'vlan://x', regardless of whether the database has 'vlan://x' or >> > > > just >> > > 'x', >> > > > thanks to this code it returns the same 'vlan://' for either stored >> > > value. >> > > > For these places it shouldn't matter if we fix the old databases to >> > > > store 'vlan://x' or the 4.3 installs to go back to 'x'. >> > > > >> > > > However, there are a few places that are broken, like this >> > > IpAssocVpcCommand >> > > > the mgmt server creates and CLOUDSTACK-5505. If we switch the db >> > > > value >> > > back, >> > > > we have to identify all of the outstanding ones and fix them. In >> > > addition, >> > > > new code since then may have perhaps assumed that the db value is >> > > 'vlan://', >> > > > and might have bothered to pass through the interpolation, so they >> > > > may >> > > break >> > > > as well. If we had full coverage on the test suite it would be easy >> > > > to change the value back in the DB of a 4.3 or 4.4 install and see >> > > > what >> > > breaks. >> > > > >> > > > If we don't switch the value back, and instead update old databases >> > > > to >> > > the >> > > > current way, it fixes the immediate issue but we end up with code >> > > > doing >> > > the >> > > > same thing in two different ways. Some places will be using the raw >> > > > db >> > > value >> > > > and other places will be asking for it to be normalized, and both >> > > > will >> > > have >> > > > the same result, which is kind of messy and prone to causing issues >> > > > down >> > > the >> > > > road if something changes again to separate these two. >> > > > >> > > > >> > > > On Mon, Jun 2, 2014 at 10:01 AM, Marcus <shadow...@gmail.com> >> > wrote: >> > > >> >> > > >> I'm not sure the KVM code needs to be changed, you're asking it to >> > > >> deal with an inconsistency from the mgmt server. Don't you find it >> > > >> odd that >> > > one >> > > >> Command from the mgmt server provides >> > > >> broadcastUri="vlan://untagged" and another provides >> > > >> broadcastUri="untagged"? I'm not sure I understand why changing >> > > >> 'untagged' into a URI format changes its meaning, but it seems >> > > like >> > > >> that doesn't make any sense to you, so perhaps we can break that >> > > >> out >> > > into a >> > > >> separate column so that we can still capture the info, if needed. >> > > >> >> > > >> If we don't like URI format for the vlan id, that's fine, but we >> > > >> need to do changes to the 4.3 installs and fix 4.4. As mentioned, I >> > > >> remember >> > > there >> > > >> being a decent amount of work to handle the "vlan://" when it was >> > > >> introduced, and that will need to be done again to change it back. >> > > >> I'm >> > > not >> > > >> against that, but I'm not going to be the one doing that work, >> > > >> either >> > > :-) >> > > >> >> > > >> >> > > >> >> > > >> On Mon, Jun 2, 2014 at 3:47 AM, Daan Hoogland >> > > >> <daan.hoogl...@gmail.com> >> > > >> wrote: >> > > >>> >> > > >>> I don't think this should be solved this way afterall. 'untagged' >> > > >>> actually means no vlan, so it should not be prepended with >> 'vlan://'. >> > > >>> I think the kvm code should be fixed for this not the generic >> code. >> > > >>> >> > > >>> On Fri, May 30, 2014 at 10:59 PM, Daan Hoogland < >> > > daan.hoogl...@gmail.com> >> > > >>> wrote: >> > > >>> > On Fri, May 30, 2014 at 10:51 PM, Marcus <shadow...@gmail.com> >> > > wrote: >> > > >>> >> Looks good to me, aside from he debug statement. >> > > >>> > >> > > >>> > Ah, the first line was not in my line of sight. >> > > >>> > -- >> > > >>> > Daan >> > > >>> >> > > >>> >> > > >>> >> > > >>> -- >> > > >>> Daan >> > > >> >> > > >> >> > > > >> > > >> > > >> > > >> > > -- >> > > Daan >> > > >> > >
