| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-25 | |||
| 18:55:39 | mriedem | so as far as i know when we unshelve we're not using the new image meta anyway | |
| 18:56:04 | sean-k-mooney | oh well it could be broken in both cases | |
| 18:56:11 | sean-k-mooney | i didnt actully check the code | |
| 18:56:54 | sean-k-mooney | we do get the metadata from the volume when initally spawning the instace | |
| 18:57:03 | mriedem | yeah i know, and on rebuild | |
| 18:57:06 | sean-k-mooney | i assume we would do it again on unselve but maybe not | |
| 18:58:15 | mriedem | no, | |
| 18:58:33 | mriedem | because on unshelve you either boot from the shelve snapshot (if not volume-backed), otherwise you boot from the root volume, | |
| 18:58:36 | mriedem | which before this couldn't change | |
| 18:59:32 | sean-k-mooney | right but on unselve we would have hit the schuler right. i guess it uses the embeded image metadata so it does not have to go back to cinder | |
| 18:59:58 | mriedem | yes unshelve hits the scheduler | |
| 19:00:10 | mriedem | and it would use whatever is in the request spec from the original server create | |
| 19:00:42 | mriedem | which uses the volume image meta here https://github.com/openstack/nova/blob/af78b13c24d4abf393d17ac57e9135204ef12b73/nova/compute/api.py#L886 | |
| 19:01:10 | sean-k-mooney | ya. so we could enable this to work for unselve by updating the request spec or just regtiving the metadata form the volume but i dont know how to "fix" start of a powered off instance | |
| 19:01:41 | sean-k-mooney | i would expect the image to go into an error state on start if its requirements chagne | |
| 19:01:51 | sean-k-mooney | but that is not very friendly | |
| 19:02:03 | sean-k-mooney | or start with the old requirement i guess | |
| 19:02:46 | mriedem | inititally the spec never said anything about supporting detach/attach of the root volume for powered off instances | |
| 19:02:50 | mriedem | only shelved offloaded | |
| 19:02:56 | mriedem | which is how hpe wrote it up long ago | |
| 19:02:57 | sean-k-mooney | yep in mitaka | |
| 19:03:25 | mriedem | i do remember that the RequestSpec.image.id is *not* set for volume-backed servers, so some of our scheduler filters don't work on volume-backed servers | |
| 19:03:52 | mriedem | like IsolatedHostsFilter | |
| 19:04:21 | mriedem | during server create, we do calculate the numa topology requirements from the flavor and image meta, and for volume-backed we get that image meta from the root volume, like you said | |
| 19:04:34 | mriedem | so technically on unshelve we could screw up and not honor the image meta in the new root volume | |
| 19:04:42 | mriedem | b/c we don't update the request spec | |
| 19:05:57 | sean-k-mooney | right. i think that is just an oversight however. e.g. readign the spec i would have assume we woudl cater for that edgecase in the implemnation and they jsut chose not to document it in the spec | |
| 19:06:29 | mriedem | i wouldn't be so sure | |
| 19:06:41 | sean-k-mooney | anyway it looks like newton was still only allowing this for shelved instance so its only allowed for powered off instance in the stein spec | |
| 19:07:16 | mriedem | i'm pretty sure i didn't think about the root image meta changing when s10 pushed for supporting attach/detach of the root volume on stopped instances | |
| 19:07:17 | sean-k-mooney | mriedem: im giving people the benifit of the doubt but perhaps it was not taught of | |
| 19:09:40 | sean-k-mooney | so with 2 weeks to feature freeze i dont really want to reopen the design of this given how long people have been waiting for it. | |
| 19:10:37 | sean-k-mooney | i could propose a revision to the spec to reduce it to only shelved insntace or to document the behavior that will happen for poered off instace | |
| 19:11:16 | sean-k-mooney | *powered off | |
| 19:11:47 | sean-k-mooney | im going to grab dinner but ill be back in an hour or so | |
| 19:12:08 | sean-k-mooney | actully i have to push a small revision to a patch then dinner | |
| 19:13:47 | mriedem | sean-k-mooney: i commented on https://review.openstack.org/#/c/614750/ as well to recap our irc discussion. i'll talk with Kevin_Zheng about it tonight since we have a meeting anyway | |
| 19:14:55 | mriedem | i don't think we can support the stopped case - that would essentially get us back to volume-backed rebuild with a new image that we don't really honor | |
| 19:16:11 | sean-k-mooney | mriedem: ya that was my feeling too but i could see use allowing it and refusing to boot the instacne if the requirement could not be supported | |
| 19:16:20 | sean-k-mooney | that felt overly complicated however | |
| 19:16:42 | sean-k-mooney | mriedem: can we shelve a powered off instance? | |
| 19:17:24 | mriedem | yes | |
| 19:17:31 | mriedem | which i think i said in the spec review when this came up, | |
| 19:17:46 | mriedem | just stop the instance, shelve it, swap the root volume, and unshelve, | |
| 19:17:47 | sean-k-mooney | ok so you jsut need to shelve it and then unshelve after changing volumes and you can more or less achive the same use case | |
| 19:17:52 | mriedem | but people thought that was too complicated | |
| 19:18:11 | sean-k-mooney | perhaps but ill take complicated over broken any day | |
| 19:22:29 | mriedem | sean-k-mooney: i guess i did think about this https://review.openstack.org/#/c/600628/7/specs/stein/approved/detach-boot-volume.rst@127 | |
| 19:25:27 | sean-k-mooney | mriedem ya looks like you did but it never made it into the spec in the end. | |
| 19:26:12 | sean-k-mooney | mriedem: atleast a concreate proposal for what to do in this case was not added to the spec. your comment are obviously there | |
| 19:28:30 | openstackgerrit | sean mooney proposed openstack/os-vif master: modify functional base.py to allow using vscode https://review.openstack.org/638058 | |
| 19:30:22 | mriedem | yeah i just don't want to get into a case where someone swaps the root volume on a stopped instance, then they rebuild it with what they thought was the original volume and rebuild blows up saying you can't rebuild a volume-backed instance with a new image, or they pass the image_id of the new root volume and the scheduler kicks it out saying it's not valid for the current host | |
| 19:30:40 | mriedem | s/original volume/original image/ | |
| 20:04:11 | openstackgerrit | Merged openstack/nova master: quota: remove QuotaEngine.register_resources() https://review.openstack.org/615613 | |
| 20:51:04 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Introduce live_migration_claim() https://review.openstack.org/635669 | |
| 20:51:04 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: New objects to transmit NUMA config from dest to source https://review.openstack.org/634827 | |
| 20:51:05 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: LM: add support for sending NUMAMigrateData to the source https://review.openstack.org/634828 | |
| 20:51:05 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: LM: add support for updating NUMA-related XML on the source https://review.openstack.org/635229 | |
| 20:51:06 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: RPC changes to prepare for NUMA live migration https://review.openstack.org/634605 | |
| 20:51:06 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: [WIP needs more tests] Full NUMA live migration support https://review.openstack.org/634606 | |
| 20:51:26 | artom | So that was "sync" push - let's run this in my test env to see what I've broken | |
| 20:57:22 | dansmith | dang | |
| 20:57:28 | dansmith | I was just working my way through them | |
| 20:57:41 | dansmith | now I have to start all over aain | |
| 20:57:45 | dansmith | guess I'll wait for next monay | |
| 20:57:52 | dansmith | also, wtf is up with my keys | |
| 20:58:56 | sean-k-mooney | dansmith: are you on vacation until monday? or just monday is the next time you will be able to review it | |
| 20:59:11 | sean-k-mooney | dansmith: just wondering if you will be around later in the week is all | |
| 20:59:54 | artom | dansmith, I'm sorry, are you asking for "monay"? I thought RH payed you well ;) | |
| 21:01:09 | dansmith | artom: monay monay | |
| 21:01:23 | artom | It's a Smith-man world! | |
| 21:01:32 | dansmith | sean-k-mooney: I'm giving artom a hard time implying that I only review large, late patch series on monday mornings | |
| 21:02:20 | sean-k-mooney | hehe in that case feel free to add my/adrianc's sriov patch set to that list :P | |
| 21:02:41 | sean-k-mooney | but on a serios note artom i deployed v21 on friday | |
| 21:02:46 | sean-k-mooney | *serious | |
| 21:03:04 | artom | v21? | |
| 21:03:06 | sean-k-mooney | am ill redeploy tomorow ish do you think it will change much | |
| 21:05:08 | sean-k-mooney | this was the top patch in the series on friday https://review.openstack.org/#/c/635229/21 | |
| 21:05:10 | artom | sean-k-mooney, dunno, lemme run my tests against what I have now, see what breaks | |
| 21:05:45 | sean-k-mooney | it looks like https://review.openstack.org/#/c/634606/30 is not the top patchset in the series | |
| 21:06:25 | artom | sean-k-mooney, that's changed. Claim creating and cleanup needs to happen in the same patch, along with a version bump because we need to check that source and/or dest support claims | |
| 21:07:31 | sean-k-mooney | right but https://review.openstack.org/#/c/634606/30 is now the final patch in the series right | |
| 21:07:38 | artom | So there's 2 service bumps to keep the patches reasonnably sized | |
| 21:07:42 | artom | 1 for the RPC stuff | |
| 21:07:54 | artom | And 1 for the final "we support NUMA LM" switchover | |
| 21:08:18 | artom | sean-k-mooney, correct | |
| 21:08:45 | artom | The overall mechanics haven't really changed, but they way they're presented/ordered and some details have | |
| 21:09:01 | sean-k-mooney | i can put off deploying the it for another few days and just to a code review tomorow and focus on other tasks if that helps. it will take me the better part of a day to run all the test i want too on it so i was going to wait till it sables a bit more | |
| 21:10:24 | artom | sean-k-mooney, your call, really. Ideally any tests that are missing from https://review.rdoproject.org/r/#/c/18832/ would get added there, so that we don't have to manually redo everything everytime there's a new patchset | |
| 21:10:32 | sean-k-mooney | artom: ok what i want to be able to do is send an email to the list and say i test all these senarios with this reviesion and heres what work and didnt | |
| 21:10:56 | sean-k-mooney | there are quite a lot of tests that arent covered | |
| 21:11:06 | artom | For instance, I realized I wasn't saving the new instance numa topology (fixed now), so I should add that to the tests | |
| 21:12:01 | artom | sean-k-mooney, oh I'm sure there are :) So... if you like it, just add them directly, or just write them out in English and I'll code them up | |
| 21:12:12 | sean-k-mooney | i might spend some time writing automated test but i also want to manually run them and verify it. | |
| 21:12:17 | mriedem | efried: jaypipes: i've gone through the next bw provider change in the series https://review.openstack.org/#/c/622421/ - i might be overly harsh on this one | |
| 21:13:04 | openstackgerrit | Merged openstack/nova master: conf: Deprecated 'defer_iptables_apply' https://review.openstack.org/445436 | |
| 21:20:05 | jaypipes | mriedem: I'm still trudging through them... sorry for the slowness. :( | |
| 21:21:00 | mriedem | that's fine | |
| 21:21:04 | mriedem | i'm about reviewed out for the day | |
| 21:22:42 | sean-k-mooney | jaypipes: hi am do you want to pushout the os-vif release to tomrow morning. i can send an update to the list with where things are or should i propose a relase with the current master. | |