| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-26 | |||
| 15:29:04 | dansmith | sean-k-mooney: ^ | |
| 15:29:18 | artom | this == the resource tracker, and claims | |
| 15:29:22 | sean-k-mooney | no we need to do it for pinned cpus too | |
| 15:29:25 | mriedem | BBBZZZTTT | |
| 15:29:35 | sean-k-mooney | hugepages can be totally done in placmenet however | |
| 15:29:37 | artom | We need to say CPU 3 is used, not just "we have 1 CPU used out of 4" | |
| 15:29:46 | sean-k-mooney | we just need to do the work to move it | |
| 15:29:50 | mriedem | before removing the claims for them, the RT also "tracked" vcpu/ram/disk, just not at some individual device level | |
| 15:29:55 | mriedem | so the RT did both before train | |
| 15:29:59 | artom | mriedem, yep | |
| 15:30:04 | mriedem | hence me and dansmith asking "what is this?" | |
| 15:32:08 | sean-k-mooney | artom: there is a part that im not fully following in your code. i need to do a full review again but where are you calling into the hardware module to calulate the new numa toplogy/cpus/hugepages | |
| 15:32:25 | artom | sean-k-mooney, not directly, it's hidden in the claim | |
| 15:32:43 | sean-k-mooney | that code definetly exsited in the stien version but i have not traced it in the new version | |
| 15:33:03 | sean-k-mooney | ok can i redeploy this again today and test it by the way | |
| 15:33:09 | artom | sean-k-mooney, yeah, at fist I wanted to avoid claims entirely because their code is, umm, interesting | |
| 15:33:17 | dansmith | artom: you're calling that in the claim to validate page_size, | |
| 15:33:20 | artom | But if we don't want races we have to use them | |
| 15:33:21 | dansmith | but nothing else right? | |
| 15:33:50 | dansmith | from the claim I mean | |
| 15:33:58 | artom | dansmith, the page size is special because the "normal" virt.hardware code would allow a page size change | |
| 15:33:59 | dansmith | he probably wants _get_live_migrate_numa_info() in the libvirt patch | |
| 15:34:09 | artom | Since it only goes by the flavor extra spec | |
| 15:34:09 | dansmith | https://review.opendev.org/#/c/634828/48/nova/virt/libvirt/driver.py | |
| 15:34:15 | artom | So we need extra logic to block it | |
| 15:34:20 | dansmith | yeah, I know | |
| 15:34:27 | sean-k-mooney | dansmith: artom shoudl be calling the hardware moduel/RT to caluate a new numa toplogy, validate page size and claim pCPUs on the destation | |
| 15:34:27 | dansmith | he's asking where the new guest numa topo gets created I think | |
| 15:34:45 | dansmith | sean-k-mooney: yeah, I'm not sure about that last bit | |
| 15:34:54 | dansmith | validate page size yes, in the claim | |
| 15:35:05 | dansmith | calculate a new topology, in the above link | |
| 15:35:14 | dansmith | but not sure I see where the dest claims that, tbh | |
| 15:35:19 | sean-k-mooney | dansmith: it shoudl all be don in can_fit_numa_to_host or something like that let me see if i can find it | |
| 15:35:31 | artom | sean-k-mooney, https://github.com/openstack/nova/blob/master/nova/compute/claims.py#L138 | |
| 15:35:53 | sean-k-mooney | yes | |
| 15:35:55 | sean-k-mooney | hardware.numa_fit_instance_to_host | |
| 15:36:11 | sean-k-mooney | is the entry point to all the nuam/hugepage/pinning code | |
| 15:36:20 | dansmith | oh, that's already there because of regular migrations right? | |
| 15:36:41 | sean-k-mooney | it looks at all those requets and returns a numa toplogy object that fultiles all of the request or raise an excepiton | |
| 15:36:42 | artom | dansmith, that's actually the non-move Claim, so even booting instances use it | |
| 15:36:48 | dansmith | okay | |
| 15:36:52 | sean-k-mooney | dansmith: its tere for cold migration | |
| 15:36:59 | dansmith | yeah | |
| 15:37:01 | dansmith | whatever | |
| 15:38:48 | sean-k-mooney | if hardware.numa_fit_instance_to_host does not raise one of like50 exceptions then the host is valid and it returns the numa toplogy | |
| 15:39:40 | artom | exception.TooHardIGiveUpDoItYourself | |
| 15:39:52 | artom | exception.TheCloudWasAMistak | |
| 15:58:59 | sean-k-mooney | artom: :) that might be clearer then some of the messages we raise | |
| 16:01:52 | artom | sean-k-mooney, hah! exception.TurnBackWhileYouCan | |
| 16:30:39 | stephenfin | bauzas: If you haven't already gone home, want to send this through? https://review.opendev.org/#/c/672336/ | |
| 16:31:05 | bauzas | stephenfin: sure, that will help not going crazy with my visa application form | |
| 16:32:48 | bauzas | stephenfin: ouch, a bit hard to review given we need to make sure we don't regress on the test coverage | |
| 16:32:55 | bauzas | did efried checked it too ? | |
| 16:33:50 | efried | yeah, I did. | |
| 16:33:51 | stephenfin | Yeah, I haven't removed anything | |
| 16:33:55 | stephenfin | It's just moving things around | |
| 16:34:06 | stephenfin | I add a good few things to that later so I wanted it broken up | |
| 16:43:19 | bauzas | ok, let's make a trust bond | |
| 16:47:13 | efried | fungi: stephenfin is quoting you here https://review.opendev.org/#/c/677969/ as agreeing we don't need eggs. If that's so, would you mind throwing a +1 on there for me? | |
| 16:47:56 | fungi | efried: sure, just a sec | |
| 16:50:43 | efried | fungi, stephenfin: okay, just as a sanity check, I codesearched: http://codesearch.openstack.org/?q=egg%3Dnova&i=nope&files=&repos= | |
| 16:51:58 | fungi | as far as i know, that's merely how you tell pip what to name the package it's installing. in modern usage it's actually going to build a wheel from the code there | |
| 16:52:20 | fungi | because it's not starting from an actual package, pip doesn't know what the package name for that repository should be | |
| 16:52:49 | fungi | i believe the name of the option is a bit of unfortunate legacy naming for the sake of backwards compatibility | |
| 16:52:52 | efried | o...kay | |
| 16:53:15 | fungi | i'll double-check the reference materials to confirm | |
| 17:00:06 | efried | dansmith: Are we allowed to rename a field like this https://review.opendev.org/#/c/678447/3/nova/db/sqlalchemy/models.py or do we have to burn it and just create a new one? | |
| 17:00:28 | dansmith | efried: no you can't do that | |
| 17:00:57 | dansmith | efried: well, wait, nothing has ever been written there right? | |
| 17:01:00 | efried | right | |
| 17:01:03 | dansmith | that one specifically | |
| 17:01:09 | efried | right | |
| 17:01:14 | efried | never used for anything at all | |
| 17:01:14 | dansmith | technically you could although it's... icky | |
| 17:01:28 | dansmith | this is why, btw, we should never merge db changes until the patches above are at least close | |
| 17:01:31 | efried | https://review.opendev.org/#/c/678447/3/nova/db/sqlalchemy/migrate_repo/versions/401_add_resources.py help? | |
| 17:01:39 | sean-k-mooney | efried: i was discussing this with alex_xu this morning | |
| 17:01:50 | efried | yeah, I still have the bruises from the original smackdown dansmith | |
| 17:01:56 | sean-k-mooney | i was suggesting just ahveing anothe migration to nuke it and add the new field | |
| 17:02:38 | sean-k-mooney | given that nothing has used it and we have not released with it | |
| 17:03:17 | sean-k-mooney | the previous version did this which was worse https://review.opendev.org/#/c/678447/2/nova/db/sqlalchemy/migrate_repo/versions/398_add_resources.py | |
| 17:03:58 | efried | dansmith: I'll -2 this for reasons stated if you want to review/approve it. | |
| 17:12:10 | fungi | efried: stephenfin: https://pip.pypa.io/en/stable/user_guide/#requirements-files #4 has an example of that syntax, and the terminology is still semi-relevant since sdists include a packagename.egg-info directory for their metadata, but the egg_info block in setup.cfg looks like it's related to old d2to1 functionality related to some sphinx extension for reporting version information (which make sense | |
| 17:12:11 | fungi | given the names of the options in it) | |
| 17:12:50 | fungi | that has appeared in nova's setup.cfg since 2010 according to git, and seems to have been cargo-culted into numerous projects who copied their setup.cfg from nova's | |
| 17:13:41 | efried | shocking | |
| 17:13:43 | fungi | it seems to be entirely vestigial at this stage, and a lot of newer projects don't include it (those newer projects which do include it have, i suspect, just copied it from older projects not knowing any better) | |
| 17:14:18 | efried | okay, well, I suppose it's probably an easy enough thing to revert & backport if we find out in four years that we broke somebody. | |
| 17:14:25 | efried | Thanks fungi | |
| 17:14:46 | fungi | yeah, if all the copying started from nova, then maybe the undoing also has to start there ;) | |
| 17:15:12 | fungi | mordred and/or dhellmann may have sufficient context to explain what that block was actually for once upon a time | |
| 17:15:35 | fungi | the fact that it references svn dates it nicely | |
| 17:18:38 | mordred | fungi: what did I do? | |
| 17:19:13 | mordred | oh that. I'd love if it went away | |
| 17:19:18 | fungi | mordred: discussing the necessity of the [egg_info] block in setup.cfg | |
| 17:19:25 | fungi | (or lack of necessity) | |
| 17:19:58 | mordred | oh - wait - I thought it was the boilerplate in setup.py for eventlet ... one sec | |
| 17:20:45 | fungi | mordred: egg_info.{tag_build,tag_date,tag_svn_revision} was something to do with a sphinx extension? | |
| 17:21:12 | fungi | i tried piecing together old commit messages and changelog entries from d2to1 to work out why it was there | |
| 17:21:46 | mordred | ah. I believe that is some text that setuptools would write to setup.cfg if it wasn't there | |