Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-03-14
15:49:44 dansmith bauzas: after further examination, rotating one uuid out of a set is not a legit thing to do to a uuid and maintain uniqueness, so I think it's flawed just on that basis alone
15:49:47 bauzas cdent: so take your time
15:49:52 cdent roger
15:50:27 bauzas cdent: that spec is maybe a strawman
15:50:33 dansmith i.e. if your key space is 0-4, and you have keys 1, 2, 3, you can't just rotate 2, or you will end up with a conflict
15:50:34 bauzas cdent: so I'm open to alternatives
15:50:38 dansmith you have to rotate them all equally
15:50:41 cdent cool
15:50:43 stephenfin dansmith: So my understanding of this was that we're creating something akin to a fake InstanceMapping so we could keep track of where we were in the process
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

Earlier   Later