Re: [openstack-dev] [nova] Do we still want to lowercase metadata keys?
On Mon, 13 Aug 2018 at 16:56, Chris Friesen wrote: > > On 08/13/2018 08:26 AM, Jay Pipes wrote: > > On 08/13/2018 10:10 AM, Matthew Booth wrote: > > >> I suspect I've misunderstood, but I was arguing this is an anti-goal. > >> There's no reason to do this if the db is working correctly, and it > >> would violate the principal of least surprise in dbs with legacy > >> datasets (being all current dbs). These values have always been mixed > >> case, lets just leave them be and fix the db. > > > > Do you want case-insensitive keys or do you not want case-insensitive keys? > > > > It seems to me that people complain that MySQL is case-insensitive by > > default > > but actually *like* the concept that a metadata key of "abc" should be > > "equal > > to" a metadata key of "ABC". > > How do we behave on PostgreSQL? (I realize it's unsupported, but it still has > users.) It's case-sensitive by default, do we override that? > > Personally, I've worked on case-sensitive systems long enough that I'd > actually > be surprised if "abc" matched "ABC". :) To the best of my knowledge, the hypothetical PostgreSQL db works exactly how you, me, and pretty much any developer would expect :) Honestly, though, SQLite is probably more interesting as it's at least used for testing. SQLite's default collation is binary, which is obviously case sensitive as you'd expect. As a developer I'm heavily biased in favour of implementing the simplest fix with the simplest and most obvious behaviour, which is to change the default collation to do what everybody expected it did in the first place (which is what Rajesh's patch does). As Jay points out, though, I do concede that those pesky users may be impacted by fixing this bug if they've come to rely on accidental buggy behaviour. The question really comes down to how we can determine what the user impact is for each solution. And here I'm talking about all the various forms of metadata, assuming that whatever solution we picked we'd apply to all. So: - What API calls allow a user to query a thing by metadata key? I believe these API calls would be the only things affected by fixing the collation of metadata keys. If we know what they are we can ask what the impact of changing the behaviour would be. Setting metadata keys isn't subject to regression, as this was previously broken. Matt -- Matthew Booth Red Hat OpenStack Engineer, Compute DFG Phone: +442070094448 (UK) __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Do we still want to lowercase metadata keys?
On 08/13/2018 11:56 AM, Chris Friesen wrote: On 08/13/2018 08:26 AM, Jay Pipes wrote: On 08/13/2018 10:10 AM, Matthew Booth wrote: I suspect I've misunderstood, but I was arguing this is an anti-goal. There's no reason to do this if the db is working correctly, and it would violate the principal of least surprise in dbs with legacy datasets (being all current dbs). These values have always been mixed case, lets just leave them be and fix the db. Do you want case-insensitive keys or do you not want case-insensitive keys? It seems to me that people complain that MySQL is case-insensitive by default but actually *like* the concept that a metadata key of "abc" should be "equal to" a metadata key of "ABC". How do we behave on PostgreSQL? (I realize it's unsupported, but it still has users.) It's case-sensitive by default, do we override that? Personally, I've worked on case-sensitive systems long enough that I'd actually be surprised if "abc" matched "ABC". :) You have worked with case-insensitive systems for as long or longer, maybe without realizing it: All URLs are case-insensitive. If a user types in http://google.com they go to the same place as http://Google.com because DNS is case-insensitive [1] and has been since its beginning. Users -- of HTTP APIs in particular -- have tended to become accustomed to case-insensitivity in their HTTP API calls. This case is no different, IMHO. Best, -jay [1] https://tools.ietf.org/html/rfc4343#section-4 __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Do we still want to lowercase metadata keys?
On 08/13/2018 08:26 AM, Jay Pipes wrote: On 08/13/2018 10:10 AM, Matthew Booth wrote: I suspect I've misunderstood, but I was arguing this is an anti-goal. There's no reason to do this if the db is working correctly, and it would violate the principal of least surprise in dbs with legacy datasets (being all current dbs). These values have always been mixed case, lets just leave them be and fix the db. Do you want case-insensitive keys or do you not want case-insensitive keys? It seems to me that people complain that MySQL is case-insensitive by default but actually *like* the concept that a metadata key of "abc" should be "equal to" a metadata key of "ABC". How do we behave on PostgreSQL? (I realize it's unsupported, but it still has users.) It's case-sensitive by default, do we override that? Personally, I've worked on case-sensitive systems long enough that I'd actually be surprised if "abc" matched "ABC". :) Chris __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Do we still want to lowercase metadata keys?
On Mon, 13 Aug 2018 at 15:27, Jay Pipes wrote: > Do you want case-insensitive keys or do you not want case-insensitive keys? > > It seems to me that people complain that MySQL is case-insensitive by > default but actually *like* the concept that a metadata key of "abc" > should be "equal to" a metadata key of "ABC". > > In other words, it seems to me that users actually expect that: > > > nova aggregate-create agg1 > > nova aggregate-set-metadata agg1 abc=1 > > nova aggregate-set-metadata agg1 ABC=2 > > should result in the original "abc" metadata item getting its value set > to "2". Incidentally, this particular example won't work today: it will just throw an error. I believe the same would apply to user metadata on an instance. IOW this particular example doesn't regress if you fix the bug. The regression would be anything user-facing which queries by metadata key. What does that? Matt -- Matthew Booth Red Hat OpenStack Engineer, Compute DFG Phone: +442070094448 (UK) __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Do we still want to lowercase metadata keys?
On Mon, 13 Aug 2018 at 15:27, Jay Pipes wrote: > > On 08/13/2018 10:10 AM, Matthew Booth wrote: > > On Mon, 13 Aug 2018 at 14:05, Jay Pipes wrote: > >> > >> On 08/13/2018 06:06 AM, Matthew Booth wrote: > >>> Thanks mriedem for answering my previous question, and also pointing > >>> out the related previous spec around just forcing all metadata to be > >>> lowercase: > >>> > >>> (Spec: approved in Newton) https://review.openstack.org/#/c/311529/ > >>> (Online migration: not merged, abandoned) > >>> https://review.openstack.org/#/c/329737/ > >>> > >>> There are other code patches, but the above is representative. What I > >>> had read was the original bug: > >>> > >>> https://bugs.launchpad.net/nova/+bug/1538011 > >>> > >>> The tl;dr is that the default collation used by MySQL results in a bug > >>> when creating 2 metadata keys which differ only in case. The proposal > >>> was obviously to simply make all metadata keys lower case. However, as > >>> melwitt pointed out in the bug at the time that's a potentially user > >>> hostile change. After some lost IRC discussion it seems that folks > >>> believed at the time that to fix this properly would seriously > >>> compromise the performance of these queries. The agreed way forward > >>> was to allow existing keys to keep their case, but force new keys to > >>> be lower case (so I wonder how the above online migration came > >>> about?). > >>> > >>> Anyway, as Rajesh's patch shows, it's actually very easy just to fix > >>> the MySQL misconfiguration: > >>> > >>> https://review.openstack.org/#/c/504885/ > >>> > >>> So my question is, given that the previous series remains potentially > >>> user hostile, the fix isn't as complex as previously believed, and it > >>> doesn't involve a performance penalty, are there any other reasons why > >>> we might want to resurrect it rather than just go with Rajesh's patch? > >>> Or should we ask Rajesh to expand his patch into a series covering > >>> other metadata? > >> > >> Keep in mind this patch is only related to *aggregate* metadata, AFAICT. > > > > Right, but the original bug pointed out that the same problem applies > > equally to a bunch of different metadata stores. I haven't verified, > > but the provenance was good ;) There would have to be other patches > > for the other metadata stores. > > Yes, it is quite unfortunate that OpenStack has about 15 different ways > of storing metadata key/value information. > > >> > >> Any patch series that tries to "fix" this issue needs to include all of > >> the following: > >> > >> * input automatically lower-cased [1] > >> * inline (note: not online, inline) data migration inside the > >> InstanceMeta object's _from_db_object() method for existing > >> non-lowercased keys > > > > I suspect I've misunderstood, but I was arguing this is an anti-goal. > > There's no reason to do this if the db is working correctly, and it > > would violate the principal of least surprise in dbs with legacy > > datasets (being all current dbs). These values have always been mixed > > case, lets just leave them be and fix the db. > > Do you want case-insensitive keys or do you not want case-insensitive keys? > > It seems to me that people complain that MySQL is case-insensitive by > default but actually *like* the concept that a metadata key of "abc" > should be "equal to" a metadata key of "ABC". > > In other words, it seems to me that users actually expect that: > > > nova aggregate-create agg1 > > nova aggregate-set-metadata agg1 abc=1 > > nova aggregate-set-metadata agg1 ABC=2 > > should result in the original "abc" metadata item getting its value set > to "2". > > If that isn't the case -- and I have a very different impression of what > users *actually* expect from the CLI/UI -- then let me know. I don't know what users want, tbh, I was simply coming from the POV of not breaking the current behaviour. Although I think you're pointing out that either solution breaks the current behaviour: 1. You lower case everything. This breaks users who query user metadata and don't expect keys to be modified. 2. You fix the case sensitivity. This breaks users who add 'Foo' and now expect to query 'foo'. You're saying that although (2) is an artifact of a bug, there could equally be people relying on it. Eurgh. Yeah, that sucks. Objectively though, I think I still like Rajesh's patch better because: * It's vastly simpler to implement correctly and verifiably, and therefore also less prone to future bugs. * It's how it was originally intended to work. * It's simpler to document. Of these, the first is by far the most persuasive. Matt -- Matthew Booth Red Hat OpenStack Engineer, Compute DFG Phone: +442070094448 (UK) __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Do we still want to lowercase metadata keys?
On 08/13/2018 10:10 AM, Matthew Booth wrote: On Mon, 13 Aug 2018 at 14:05, Jay Pipes wrote: On 08/13/2018 06:06 AM, Matthew Booth wrote: Thanks mriedem for answering my previous question, and also pointing out the related previous spec around just forcing all metadata to be lowercase: (Spec: approved in Newton) https://review.openstack.org/#/c/311529/ (Online migration: not merged, abandoned) https://review.openstack.org/#/c/329737/ There are other code patches, but the above is representative. What I had read was the original bug: https://bugs.launchpad.net/nova/+bug/1538011 The tl;dr is that the default collation used by MySQL results in a bug when creating 2 metadata keys which differ only in case. The proposal was obviously to simply make all metadata keys lower case. However, as melwitt pointed out in the bug at the time that's a potentially user hostile change. After some lost IRC discussion it seems that folks believed at the time that to fix this properly would seriously compromise the performance of these queries. The agreed way forward was to allow existing keys to keep their case, but force new keys to be lower case (so I wonder how the above online migration came about?). Anyway, as Rajesh's patch shows, it's actually very easy just to fix the MySQL misconfiguration: https://review.openstack.org/#/c/504885/ So my question is, given that the previous series remains potentially user hostile, the fix isn't as complex as previously believed, and it doesn't involve a performance penalty, are there any other reasons why we might want to resurrect it rather than just go with Rajesh's patch? Or should we ask Rajesh to expand his patch into a series covering other metadata? Keep in mind this patch is only related to *aggregate* metadata, AFAICT. Right, but the original bug pointed out that the same problem applies equally to a bunch of different metadata stores. I haven't verified, but the provenance was good ;) There would have to be other patches for the other metadata stores. Yes, it is quite unfortunate that OpenStack has about 15 different ways of storing metadata key/value information. Any patch series that tries to "fix" this issue needs to include all of the following: * input automatically lower-cased [1] * inline (note: not online, inline) data migration inside the InstanceMeta object's _from_db_object() method for existing non-lowercased keys I suspect I've misunderstood, but I was arguing this is an anti-goal. There's no reason to do this if the db is working correctly, and it would violate the principal of least surprise in dbs with legacy datasets (being all current dbs). These values have always been mixed case, lets just leave them be and fix the db. Do you want case-insensitive keys or do you not want case-insensitive keys? It seems to me that people complain that MySQL is case-insensitive by default but actually *like* the concept that a metadata key of "abc" should be "equal to" a metadata key of "ABC". In other words, it seems to me that users actually expect that: > nova aggregate-create agg1 > nova aggregate-set-metadata agg1 abc=1 > nova aggregate-set-metadata agg1 ABC=2 should result in the original "abc" metadata item getting its value set to "2". If that isn't the case -- and I have a very different impression of what users *actually* expect from the CLI/UI -- then let me know. -jay __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [nova] Do we still want to lowercase metadata keys?
On Mon, 13 Aug 2018 at 14:05, Jay Pipes wrote: > > On 08/13/2018 06:06 AM, Matthew Booth wrote: > > Thanks mriedem for answering my previous question, and also pointing > > out the related previous spec around just forcing all metadata to be > > lowercase: > > > > (Spec: approved in Newton) https://review.openstack.org/#/c/311529/ > > (Online migration: not merged, abandoned) > > https://review.openstack.org/#/c/329737/ > > > > There are other code patches, but the above is representative. What I > > had read was the original bug: > > > > https://bugs.launchpad.net/nova/+bug/1538011 > > > > The tl;dr is that the default collation used by MySQL results in a bug > > when creating 2 metadata keys which differ only in case. The proposal > > was obviously to simply make all metadata keys lower case. However, as > > melwitt pointed out in the bug at the time that's a potentially user > > hostile change. After some lost IRC discussion it seems that folks > > believed at the time that to fix this properly would seriously > > compromise the performance of these queries. The agreed way forward > > was to allow existing keys to keep their case, but force new keys to > > be lower case (so I wonder how the above online migration came > > about?). > > > > Anyway, as Rajesh's patch shows, it's actually very easy just to fix > > the MySQL misconfiguration: > > > > https://review.openstack.org/#/c/504885/ > > > > So my question is, given that the previous series remains potentially > > user hostile, the fix isn't as complex as previously believed, and it > > doesn't involve a performance penalty, are there any other reasons why > > we might want to resurrect it rather than just go with Rajesh's patch? > > Or should we ask Rajesh to expand his patch into a series covering > > other metadata? > > Keep in mind this patch is only related to *aggregate* metadata, AFAICT. Right, but the original bug pointed out that the same problem applies equally to a bunch of different metadata stores. I haven't verified, but the provenance was good ;) There would have to be other patches for the other metadata stores. > > Any patch series that tries to "fix" this issue needs to include all of > the following: > > * input automatically lower-cased [1] > * inline (note: not online, inline) data migration inside the > InstanceMeta object's _from_db_object() method for existing > non-lowercased keys I suspect I've misunderstood, but I was arguing this is an anti-goal. There's no reason to do this if the db is working correctly, and it would violate the principal of least surprise in dbs with legacy datasets (being all current dbs). These values have always been mixed case, lets just leave them be and fix the db. > * change the collation of the aggregate_metadata.key column (note: this > will require an entire rebuild of the table, since this column is part > of a unique constraint [3] Rajesh's patch changes the collation of the table, which I would assume applies to its columns? I assume this is going to be a moderately expensive, but one-off, operation similar in cost to adding a new unique constraint. > * online data migration for migrating non-lowercased keys to their > lowercased counterpars (essentially doing `UPDATE key = LOWER(key) WHERE > LOWER(key) != key` once the collation has been changed) > None of the above touches the API layer. I suppose some might argue that > the REST API should be microversion-bumped since the expected behaviour > of the API will change (data will be transparently changed in one > version of the API and not another). I don't personally think that's > something I would require a microversion for, but who knows what others > may say. Again, I was considering this is an anti-goal. As I understand, Rajesh's patch removes the requirement to make this api change. What did I miss? Thanks, Matt > > Best, > -jay > > [1] > https://github.com/openstack/nova/blob/16f89fd093217d22530570e8277b561ea79f46ff/nova/objects/aggregate.py#L295 > and > https://github.com/openstack/nova/blob/16f89fd093217d22530570e8277b561ea79f46ff/nova/objects/aggregate.py#L331 > and > https://github.com/openstack/nova/blob/16f89fd093217d22530570e8277b561ea79f46ff/nova/objects/aggregate.py#L356 > > > [2] > https://github.com/openstack/nova/blob/16f89fd093217d22530570e8277b561ea79f46ff/nova/objects/aggregate.py#L248 > > [3] > https://github.com/openstack/nova/blob/16f89fd093217d22530570e8277b561ea79f46ff/nova/db/sqlalchemy/api_models.py#L64 > > __ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Matthew Booth Red Hat OpenStack Engineer, Compute DFG Phone: +442070094448 (UK) __ OpenStack Development Mailing List (not for usage questi
Re: [openstack-dev] [nova] Do we still want to lowercase metadata keys?
On 08/13/2018 06:06 AM, Matthew Booth wrote: Thanks mriedem for answering my previous question, and also pointing out the related previous spec around just forcing all metadata to be lowercase: (Spec: approved in Newton) https://review.openstack.org/#/c/311529/ (Online migration: not merged, abandoned) https://review.openstack.org/#/c/329737/ There are other code patches, but the above is representative. What I had read was the original bug: https://bugs.launchpad.net/nova/+bug/1538011 The tl;dr is that the default collation used by MySQL results in a bug when creating 2 metadata keys which differ only in case. The proposal was obviously to simply make all metadata keys lower case. However, as melwitt pointed out in the bug at the time that's a potentially user hostile change. After some lost IRC discussion it seems that folks believed at the time that to fix this properly would seriously compromise the performance of these queries. The agreed way forward was to allow existing keys to keep their case, but force new keys to be lower case (so I wonder how the above online migration came about?). Anyway, as Rajesh's patch shows, it's actually very easy just to fix the MySQL misconfiguration: https://review.openstack.org/#/c/504885/ So my question is, given that the previous series remains potentially user hostile, the fix isn't as complex as previously believed, and it doesn't involve a performance penalty, are there any other reasons why we might want to resurrect it rather than just go with Rajesh's patch? Or should we ask Rajesh to expand his patch into a series covering other metadata? Keep in mind this patch is only related to *aggregate* metadata, AFAICT. Any patch series that tries to "fix" this issue needs to include all of the following: * input automatically lower-cased [1] * inline (note: not online, inline) data migration inside the InstanceMeta object's _from_db_object() method for existing non-lowercased keys * change the collation of the aggregate_metadata.key column (note: this will require an entire rebuild of the table, since this column is part of a unique constraint [3] * online data migration for migrating non-lowercased keys to their lowercased counterpars (essentially doing `UPDATE key = LOWER(key) WHERE LOWER(key) != key` once the collation has been changed) None of the above touches the API layer. I suppose some might argue that the REST API should be microversion-bumped since the expected behaviour of the API will change (data will be transparently changed in one version of the API and not another). I don't personally think that's something I would require a microversion for, but who knows what others may say. Best, -jay [1] https://github.com/openstack/nova/blob/16f89fd093217d22530570e8277b561ea79f46ff/nova/objects/aggregate.py#L295 and https://github.com/openstack/nova/blob/16f89fd093217d22530570e8277b561ea79f46ff/nova/objects/aggregate.py#L331 and https://github.com/openstack/nova/blob/16f89fd093217d22530570e8277b561ea79f46ff/nova/objects/aggregate.py#L356 [2] https://github.com/openstack/nova/blob/16f89fd093217d22530570e8277b561ea79f46ff/nova/objects/aggregate.py#L248 [3] https://github.com/openstack/nova/blob/16f89fd093217d22530570e8277b561ea79f46ff/nova/db/sqlalchemy/api_models.py#L64 __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
[openstack-dev] [nova] Do we still want to lowercase metadata keys?
Thanks mriedem for answering my previous question, and also pointing out the related previous spec around just forcing all metadata to be lowercase: (Spec: approved in Newton) https://review.openstack.org/#/c/311529/ (Online migration: not merged, abandoned) https://review.openstack.org/#/c/329737/ There are other code patches, but the above is representative. What I had read was the original bug: https://bugs.launchpad.net/nova/+bug/1538011 The tl;dr is that the default collation used by MySQL results in a bug when creating 2 metadata keys which differ only in case. The proposal was obviously to simply make all metadata keys lower case. However, as melwitt pointed out in the bug at the time that's a potentially user hostile change. After some lost IRC discussion it seems that folks believed at the time that to fix this properly would seriously compromise the performance of these queries. The agreed way forward was to allow existing keys to keep their case, but force new keys to be lower case (so I wonder how the above online migration came about?). Anyway, as Rajesh's patch shows, it's actually very easy just to fix the MySQL misconfiguration: https://review.openstack.org/#/c/504885/ So my question is, given that the previous series remains potentially user hostile, the fix isn't as complex as previously believed, and it doesn't involve a performance penalty, are there any other reasons why we might want to resurrect it rather than just go with Rajesh's patch? Or should we ask Rajesh to expand his patch into a series covering other metadata? Matt -- Matthew Booth Red Hat OpenStack Engineer, Compute DFG Phone: +442070094448 (UK) __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
