Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-03-14
15:50:45 bauzas dansmith: right, I don't like touching things
15:50:58 dansmith stephenfin: that's the marker functionality, yes
15:51:09 bauzas but that's not the only marker we had, right?
15:51:25 bauzas like, I provided a marker IIRC for the RequestSpec migration thing
15:51:47 bauzas now the code is gone, but lemme see how I did that and if that can help
15:51:53 stephenfin dansmith: Right, so if were re-using the UUID of an existing instance we were going to bump into the UNIQUE constraint, as noted on line 1127 there
15:51:56 dansmith bauzas: right, and also, this is storing an _actual_ uuid in this field
15:52:04 dansmith bauzas: so if the warning is complaining about it, then it's wrong
15:52:18 stephenfin and to work around that, alaski put in a patch that munged the instance UUID to avoid said constraint
15:52:31 stephenfin dansmith: UUIDs need to have hyphens in them, no?
15:52:45 bauzas aaaaah I see
15:53:00 stephenfin https://en.wikipedia.org/wiki/Universally_unique_identifier#Format
15:53:01 bauzas stephenfin: the RFC doesn't really mention that IIRC
15:53:14 bauzas it's just a python uuid thing IIRC
15:53:17 dansmith stephenfin: ah, I see what's going on
15:53:40 bauzas stephenfin: https://tools.ietf.org/html/rfc4122#page-5
15:53:50 bauzas oops https://tools.ietf.org/html/rfc4122#section-4.1
15:54:06 dansmith stephenfin: it does't change the fact that we're rotating one uuid in a given namespace, and storing it in the DB, just to get past a uuid format warning
15:54:08 mriedem dansmith: i always try to explain why i'm reverting something
15:54:14 mriedem because the later patch might never come
15:54:33 dansmith mriedem: okay, I just didn't want to argue over the rationale and delay the revert, but fair enough
15:54:42 stephenfin dansmith: It seemed no worse than stripping out the spaces. It was still very much a reversible operation
15:55:14 stephenfin However, you're suggesting that it's not actually valid thing to do. I didn't know that and that makes a revert the right thing to do, if so
15:55:20 dansmith stephenfin: it's completely worse because (a) it could create a conflict, (b) it breaks the format that people in the middle of this operation may have already stored and (c) it's much harder to manually inspect if there is a problem
15:55:37 cfriesen cdent: for the VCPU vs NUMA_CORES thing, also bear in mind the proposed specs for shared/dedicated instances on the same host, and even on the same instance, as well as sahid's spec to run the emulator threads on a separate pool of host pCPUs
15:55:56 stephenfin Are UUID conflicts a thing? Aren't there, like, a bajillion permutations?
15:56:03 cdent cfriesen: yeah, it makes my head swim
15:56:06 cfriesen cdent: make that shared/dedicated vCPUs within the same instance
15:56:08 bauzas okay, so I marked the last Requestspec I checked by using a NULL UUID for the instance https://github.com/openstack/nova/commit/09f2d4d5ec3a699176d70c2407ced0ce7cd58197#diff-cbbdc4d7c140314a7e0b2d97ebcd1f9c
15:56:28 stephenfin That was noted in the commit message and I agreed with it https://review.openstack.org/#/c/539323/5//COMMIT_MSG@30
15:56:34 dansmith stephenfin: but it's wrong
15:56:36 bauzas now, I need to understand why the marker is different for instance_mappings
15:56:38 stephenfin :)
15:56:45 cfriesen cdent: if we could figure out an efficient generic way to do dynamic allocation of pCPUs for shared vs dedicated it would simplify things, but it seems too complicated to do generically
15:57:41 bauzas cfriesen: context is https://review.openstack.org/#/c/552924/
15:58:16 openstackgerrit Dan Smith proposed openstack/nova master: Revert "Make the InstanceMapping marker UUID-like" https://review.openstack.org/552937
15:58:24 dansmith bauzas: mriedem stephenfin ^
15:58:38 bauzas cfriesen: cdent: what I'd like is to keep the root RP responsible for the general resource classes for upgrade and consistency purposes, and just add NUMA-specific resources to the children
15:59:17 bauzas the fact that the operator has to handle flavors accordingly (ie. having a NUMA_CORES value that matches with the flavor VCPU) is something important to note, but not a big deal
16:02:49 bauzas stephenfin: see https://docs.openstack.org/nova/latest/contributor/policies.html#reverts-for-retrospective-vetos
16:03:38 stephenfin bauzas: Good to know :)
16:04:45 cfriesen bauzas: so what would the extra-specs look like? would we drop the existing "dedicated" cpu policy?
16:04:52 dansmith stephenfin: wanna discuss why #1 is an issue?
16:05:20 cfriesen bauzas: and what about the "isolate" hyperthreading policy, how would we handle the sibling LCPUs?
16:05:26 dansmith or are you saying it's so unlikely that you don't care?
16:05:33 bauzas cfriesen: I imagine some time being where two things would continue to work
16:05:56 bauzas cfriesen: I'm not trying to boil the ocean and have all the NUMA policies be done in Placement
16:06:22 stephenfin dansmith: Sure. I'd been going under the assumption that UUID conflicts were night-on impossible. Shuffling bits around on an existing UUID wouldn't change that
16:07:00 cfriesen bauzas: agreed, I just don't have a good picture of what the extra specs would look like when asking for dedicated cpus
16:07:07 dansmith stephenfin: https://en.wikipedia.org/wiki/Universally_unique_identifier#Collisions
16:07:20 dansmith stephenfin: "collisions can occur only when an implementation varies from the standards, either inadvertently or intentionally"
16:07:44 dansmith stephenfin: mutating one uuid (even predictably) within a given namespace of others that are not so mutated would amount to a different implementation right?
16:07:48 bauzas dansmith: right, like I said, nothing in the RFC tells "-" is mandatory AFAIK
16:07:56 dansmith bauzas: well, there's also that
16:08:06 dansmith stephenfin: uuids boil down to a very large number space
16:08:10 edleafe dansmith: so is your main concern the possibility of collisions?
16:08:30 dansmith stephenfin: if you reduce that to a human-conceivable range and then populate it with some values, rotating one within the space, you can easily cause a conflict
16:08:56 dansmith edleafe: my main concerns are in the revert.. collisions are one aspect
16:09:12 bauzas cfriesen: from https://docs.openstack.org/nova/pike/admin/flavors.html#extra-specs-numa-topology
16:09:13 dansmith edleafe: totally agree it's very unlikely but it's fundamentally a wrong thing to do to mutate one like that
16:10:25 arvindn05 mriedem: wanted to discuss your review on https://review.openstack.org/#/c/541507/. Are you going to be online an hr from now?
16:10:37 edleafe dansmith: just to be clear: it wasn't to pretty up the unit tests. When o.vo switches from warning to raising, it will break this code
16:10:49 stephenfin dansmith: I imagine it would, yes
16:11:01 edleafe And adding spaces to a UUID is worse munging, IMO
16:11:09 stephenfin and I certainly can't prove it wouldn't
16:11:15 mriedem arvindn05: yes
16:11:17 stephenfin Yeah, that was my thinking ^
16:11:24 stephenfin If we're going to munge, do it right
16:11:28 dansmith edleafe: which may never happen, if you look at the discussion around doing the uuid format checking a couple years ago..
16:11:51 stephenfin bauzas: What was this about using a null UUID as the marker?
16:11:54 arvindn05 mriedem: thanks!
16:11:57 dansmith stephenfin: edleafe: but changing dashes to spaces doesn't change the unique elements of the uuid, it only changes the uniqueness of the string as a SQL hack
16:12:10 bauzas stephenfin: I was walking over the RequestSpec tablre
16:12:21 dansmith using the null uuid is a completely legit way to do this, as bauzas did for reqspec
16:12:34 dansmith but it doesn't help as much in this case because of where the constraints lie
16:13:44 cfriesen bauzas: I understand the current extra specs, just wasn't sure what it would look like with the placement stuff added to it.
16:14:11 openstackgerrit Chris Dent proposed openstack/nova-specs master: Spec for isolating configuration of placement database https://review.openstack.org/552927
16:15:03 bauzas dansmith: yeah, I'm trying to load again the instance mapping context in mind
16:15:29 bauzas and why it has to be different from my walkthrough over reqspec
16:15:32 dansmith bauzas: instance mapping is storing the uuids of the instances, so it can't store a duplicate one since there is a unique constraint on that column
16:16:07 dansmith there's no room inside it to stash the uuid elsewhere and use the null uuid as the marker key
16:16:29 bauzas disclaimer: I have goblins at home that make noise, I need to take my Bose headset
16:20:16 bauzas dansmith: mmmm, I was just considering the Nul UUID for storing the last RequestSpec record I was walking thru
16:20:42 bauzas now, I'm looking at instance_mappings
16:22:37 dansmith bauzas: you were storing the instance uuid inside a blob field, which was not required to be unique (nor would be) and thus it didn't conflict with the actual reqspec for the instance
16:22:47 dansmith bauzas: and using the nil uuid to find the marker each time
16:22:58 bauzas yeah, also because I was wrong 5 mins ago
16:23:12 dansmith in instance mapping, we can't store the instance's uuid exactly again because it's a UC field
16:23:21 bauzas I wasn't walking over reqspec, but rather over the instances table
16:23:27 bauzas right
16:24:04 bauzas dansmith: I wasn't storing the instance field, I was storing the request spec of the last instance I checkedc
16:24:12 dansmith right
16:24:40 bauzas now, I'm looking at the instance mappings migration script
16:24:59 dansmith we could have used the nil uuid here and then stashed the instance's real uuid in the project field or something like that, but that would be rather nasty as well and could have affected runtime in other ways
16:25:04 dansmith and, this is done and in people's systems
16:26:00 bauzas right
16:26:07 bauzas now the thing is done
16:26:35 cdent edleafe: is member_of is a state of ready for review?
16:26:39 cdent s/is/in/
16:28:12 bauzas gibi: so, I just saw https://github.com/openstack/oslo.versionedobjects/commit/0e3526710f67b3b4ebab60864ea060fa9caf9537

Earlier   Later