| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-09-24 | |||
| 20:19:41 | dansmith | URIs are not, AFAIK, in apache | |
| 20:19:50 | jaypipes | but whatevers, I need to go puke on chef again. | |
| 20:20:20 | mriedem | or, we need to lowercase aggregate members everywhere...? | |
| 20:20:20 | dansmith | mriedem: yes I think we should revert rajesh's patch and fix the NotFound on original add | |
| 20:20:44 | mriedem | how do we fix the not found on original add if the db is case insensitive? | |
| 20:21:05 | dansmith | we just check the result | |
| 20:21:14 | dansmith | if host_name == compute.host | |
| 20:21:15 | dansmith | or whatever | |
| 20:21:16 | jaypipes | dansmith: and you'd need to fix PG manually since it will undoubtedly have allowed 2 records, one for compute0 and one for COMPUTE0 | |
| 20:21:29 | mriedem | jaypipes: mysql manually | |
| 20:21:32 | mriedem | PG won't allow this by default | |
| 20:21:34 | jaypipes | in the host_aggregate_hosts table (or whatever the heck it's named) | |
| 20:21:41 | dansmith | yeah PG is already fine | |
| 20:21:43 | dansmith | no | |
| 20:21:49 | jaypipes | what are you guys talking about? | |
| 20:21:49 | dansmith | you won't have ever found COMPUTE0 | |
| 20:21:51 | dansmith | so we would fail | |
| 20:22:13 | dansmith | jaypipes: we look up the host before we add the aggegate_host | |
| 20:22:20 | jaypipes | PG will allow multiple records to be created, one for agg -> COMPUTE0 and one for agg -> compute0 | |
| 20:22:22 | dansmith | in mysql we find it wrongly, case insensitive | |
| 20:22:27 | dansmith | in PG, we never find it, | |
| 20:22:43 | jaypipes | ah. if that fetch is done in the API layer... | |
| 20:22:45 | dansmith | so we won't add the aggregate_host | |
| 20:22:45 | jaypipes | is it? | |
| 20:22:46 | dansmith | we don't need the constraint to help us in PG, | |
| 20:22:47 | dansmith | because we never get that far | |
| 20:22:48 | dansmith | right | |
| 20:22:58 | dansmith | mriedem says it is | |
| 20:23:09 | dansmith | [13:15:46] <mriedem>mapping = objects.HostMapping.get_by_host(context, host_name) | |
| 20:23:22 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L5289 | |
| 20:24:05 | mriedem | dansmith: so you want to add: if mapping.host == host_name? in the api? | |
| 20:24:09 | mriedem | for the case check? | |
| 20:24:17 | dansmith | yeah | |
| 20:24:26 | mriedem | yeah that seems easy enough | |
| 20:24:28 | dansmith | wait, are you asking if I want to write that patch? | |
| 20:24:35 | dansmith | or just if I like that approach? | |
| 20:24:40 | mriedem | i was confirming that was your idea | |
| 20:24:45 | dansmith | indeed | |
| 20:24:57 | mriedem | i expect you're already half dressed for vacation | |
| 20:25:03 | dansmith | indeed :) | |
| 20:25:05 | mriedem | (un)dressed | |
| 20:25:33 | mriedem | dansmith: are you going to propose the revert at least? | |
| 20:25:37 | melwitt | will this change in anyway cause existing tooling to stop working? that is, do we need a microversion? | |
| 20:25:41 | dansmith | I can if you want | |
| 20:25:52 | mriedem | we already regressed existing tooling | |
| 20:25:59 | dansmith | melwitt: this is making the api consistent with the database, if your database is PG | |
| 20:25:59 | openstack | Launchpad bug 1793747 in OpenStack Compute (nova) "Fails to boot instance using Blazar flavor if compute host names are in uppercase" [High,Triaged] - Assigned to Neha Alhat (nehaalhat) | |
| 20:25:59 | mriedem | https://bugs.launchpad.net/nova/+bug/1793747 | |
| 20:26:01 | openstackgerrit | Merged openstack/python-novaclient stable/pike: import zuul job settings from project-config https://review.openstack.org/601395 | |
| 20:26:01 | dansmith | right | |
| 20:26:03 | openstackgerrit | Merged openstack/python-novaclient stable/ocata: import zuul job settings from project-config https://review.openstack.org/601391 | |
| 20:26:03 | openstackgerrit | Merged openstack/python-novaclient stable/ocata: Use generic user for both zuul v2 and v3 https://review.openstack.org/601932 | |
| 20:26:09 | melwitt | I mean the addition of the case sensitive check, not the revert | |
| 20:26:16 | dansmith | the check is ^ | |
| 20:26:26 | mriedem | shouldn't need to opt out of broken behavior | |
| 20:26:29 | dansmith | right | |
| 20:26:29 | mriedem | so this isn't a microversion | |
| 20:26:45 | melwitt | ok, just wondering if this might somehow break anyone's existing automation | |
| 20:27:19 | dansmith | we already broke it | |
| 20:27:28 | dansmith | this makes it work again | |
| 20:27:36 | dansmith | anyone's automation is going to be using the actual hostname, | |
| 20:27:43 | dansmith | either from the system or from our own API | |
| 20:27:50 | dansmith | those things would have broken silently just now | |
| 20:27:53 | dansmith | this will unbreak them | |
| 20:27:59 | jaypipes | this is partly why I hate even reviewing anything like that patch any more... always seems that they get reverted. | |
| 20:28:00 | dansmith | the combo | |
| 20:28:41 | mriedem | i should have sniffed this out sooner on the backports given i knew about the earlier work from HPE to try and solve the case insensitivity issues in the api | |
| 20:29:02 | mriedem | mdbooth was also working on something related to this recently | |
| 20:29:36 | mriedem | http://lists.openstack.org/pipermail/openstack-dev/2018-August/thread.html#133202 | |
| 20:29:51 | melwitt | yeah, that was the metadata keys case insensitive thing | |
| 20:29:59 | dansmith | mriedem: | |
| 20:30:44 | mriedem | beware then https://review.openstack.org/#/c/504885/ | |
| 20:30:48 | openstackgerrit | Dan Smith proposed openstack/nova master: Revert "Make host_aggregate_map dictionary case-insensitive" https://review.openstack.org/604898 | |
| 20:30:56 | melwitt | it was confusing thinking about it, but the proposal was to make the key column case sensitive by changing the collation, but that would break anyone who was relying on the case insensitive behavior. and then I got fatigued thinking about it | |
| 20:31:30 | dansmith | yep, | |
| 20:31:31 | melwitt | so I was trying to determine whether there would be any similar thing here | |
| 20:31:38 | dansmith | the difference here is that a hostname with a different name is not legit | |
| 20:31:42 | dansmith | in an aggregate mapping | |
| 20:31:56 | dansmith | since metadata is free-form, it is legit (although silly) to use two keys that differ only by case | |
| 20:32:09 | dansmith | er, "hostname with a different case" | |
| 20:32:32 | melwitt | ok, I see | |
| 20:33:06 | dansmith | mriedem: I can stack the check on top of this if you'll write the tests when I run out of time | |
| 20:35:04 | dansmith | mriedem: we could also just use the name from the mapping instead of the one they asked for, | |
| 20:35:18 | dansmith | which means they won't get a NotFound, and we'll add the mapping they intended | |
| 20:35:27 | dansmith | but they may fail similarly later if they keep using the wrong name | |
| 20:35:34 | dansmith | so I tend to err on the side of fail fast, | |
| 20:35:41 | dansmith | especially since PG will already cause us to behave that way | |
| 20:35:44 | dansmith | thoughts? | |
| 20:35:51 | mriedem | agree on the api being explicitly | |
| 20:35:53 | mriedem | *explicit | |
| 20:38:15 | melwitt | yeah it sounds like fail fast would be better | |
| 20:40:56 | openstackgerrit | Jack Ding proposed openstack/nova master: Correct instance port binding for rebuilds/reboots https://review.openstack.org/603844 | |
| 20:47:08 | jaypipes | yeah, I definitely won't be reviewing 504885. Tired of approving these patches and then needing them to be reverted. | |
| 20:47:25 | jaypipes | I'll just stick to reviewing anything that isn't in the API layer I guess. | |
| 20:47:41 | jaypipes | since our API layer is so friggin eggshells. | |
| 20:49:26 | openstackgerrit | Dan Smith proposed openstack/nova master: Enforce case-sensitive hostnames in aggregate host add https://review.openstack.org/604906 | |
| 20:49:36 | dansmith | mriedem: all yours if that fails other tests | |
| 20:50:15 | openstackgerrit | Dan Smith proposed openstack/nova master: Enforce case-sensitive hostnames in aggregate host add https://review.openstack.org/604906 | |
| 20:55:00 | mriedem | ack | |
| 21:05:42 | melwitt | reminder to everyone: forum topic submission deadline is Wed Sep 26 | |