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 >> > >
