| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-14 | |||
| 15:36:50 | dansmith | gibi: stephenfin ^ | |
| 15:37:51 | stephenfin | dansmith: Heavy handed, perhaps, but it did resolve the issue and made sense to me | |
| 15:38:24 | dansmith | stephenfin: it resolves the warning in unit tests by storing an obfuscated uuid in the database which would make it really hard to examine what is going on | |
| 15:38:42 | dansmith | that seems not sensical to me | |
| 15:39:31 | dansmith | and would break anyone who was halfway through a mapping and then walked over this | |
| 15:39:35 | dansmith | like someone doing an FFU | |
| 15:40:11 | dansmith | and.. instance uuid _is_ a uuid, so if there's a warning about the format of it, maybe the warning is wrong or something else needs to change | |
| 15:40:35 | dansmith | but munging the values (which I'm not even sure is legit uuid behavior) seems kinda silly | |
| 15:40:36 | openstackgerrit | Merged openstack/nova master: Updated from global requirements https://review.openstack.org/552774 | |
| 15:42:22 | dansmith | if you're not doing the rotation on all uuids then you're increasing your chances for collision, and since this doesn't (actual instance mappings have unrotated uuids, only the marker is rotated) | |
| 15:43:46 | dansmith | since this is munging data in the database I think I'm going to exercise fast revert | |
| 15:44:12 | stephenfin | dansmith: Gimme a sec. I'm trying to re-review this and see what my reasoning for letting it in was | |
| 15:44:51 | cdent | bauzas: I may have missed some discussion in Dublin, but what's the different between a NUMA_CORE and a VCPU and why keep track of both of them? | |
| 15:46:14 | bauzas | cdent: good question | |
| 15:46:21 | bauzas | cdent: see my alternatives section | |
| 15:47:05 | bauzas | if we shard VCPUs across NUMA nodes, then an instance spreading its cores amongst multilple NUMA nodes could have problems | |
| 15:47:09 | dansmith | mriedem: I forget, in my revert patch should I explain the problems I think there are or just do a straight revert and then discuss on a re-propose | |
| 15:47:10 | dansmith | ? | |
| 15:47:17 | cdent | bauzas: yeah, saw that, but that doesn't really answer the question. If you are counting NUMA_CORES (as a separate thing) why would you record VCPU at all? | |
| 15:47:26 | bauzas | dansmith: fast-revert and discuss later IMO | |
| 15:47:50 | dansmith | bauzas: okay that's what I was thinking too, but it's been a while | |
| 15:47:55 | bauzas | cdent: because operators that wouldn't care about NUMA things wouldn't then need to use a NUMA specific RC | |
| 15:48:55 | bauzas | dansmith: stephenfin: so I need a bit of context in order to chime in | |
| 15:49:07 | cdent | I'll have to think on that some more. Having to track the same thing multiple times doesn't seem simple to me. It may, however, be that simple is not possible. | |
| 15:49:36 | bauzas | cdent: np, I'll be on PTO till thurs starting EOB | |
| 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 | |