| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-22 | |||
| 15:17:51 | mriedem | since we didn't change the guest | |
| 15:17:58 | mriedem | you'd get a fault recorded as to why the suspend failed | |
| 15:18:01 | mriedem | which is probably good enough | |
| 15:18:05 | bauzas | set it back to ACTIVE | |
| 15:18:08 | bauzas | then | |
| 15:18:14 | bauzas | mmm, good call | |
| 15:18:46 | mriedem | yeah i guess you'd revert to the original vm_state, which right now can only be active in the API | |
| 15:19:47 | mriedem | johnthetubaguy: replied to your question in the multiattach api change https://review.openstack.org/#/c/271047/ - i think we're covered for the attach flow, but in a different way | |
| 15:20:39 | johnthetubaguy | mriedem: ah... got it | |
| 15:22:46 | cdent | efried: so, based on your response are you aserting "don't worry, the provider tree will always be right because we've merged enough code for that to be true". If so, cool, but that wasn't clear from earlier discussion on the patches. | |
| 15:24:05 | efried | cdent I'll grant you we still have work to do on concurrency management, though we've asserted that that's only a theoretical problem at the moment due to The Big Semaphore and the single-source-of-control-ness for compute node RPs at the moment. | |
| 15:24:18 | mgoddard_ | cdent, efried: I think a simple answer here is that the resource tracker will always have called set_inventory_for_provider prior to calling set_traits_for_provider, and this ensures that the RPs are present | |
| 15:25:21 | efried | mgoddard_ cdent Ohhh, are we worried about having populated the cache with the relevant provider at this point? I didn't pick up on that at all. | |
| 15:25:47 | cdent | efried: yes, my query has been, all along: is the provider tree active for this code path? | |
| 15:26:51 | efried | cdent That's definitely a legitimate concern, because as documented on set_traits_for_provider (https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L1042) we don't attempt to create the provider. But furthermore, we don't do _ensure_provider either, so there had better have been something prior that populated the cache for that guy. | |
| 15:27:37 | efried | cdent I'm sure I'm just being obtuse, but I didn't understand that from your comments at all :( | |
| 15:27:40 | mgoddard_ | efried: yes, it's set_inventory_for_provider | |
| 15:28:58 | efried | mgoddard_ That's good; and I think it's worth adding a code comment to that effect to affirm that it's been considered. Good call cdent | |
| 15:29:27 | cdent | mgoddard_: how/where does set_inventory_for_provider get involved in the management of the ProviderTree? | |
| 15:29:53 | efried | cdent It calls _ensure_resource_provider first thing. | |
| 15:29:57 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add the nova-multiattach job https://review.openstack.org/532689 | |
| 15:30:06 | efried | cdent Which creates the provider if it doesn't exist, but in any case populates/refreshes the cache | |
| 15:31:02 | cdent | okay, that's the missing piece of the pie, thank you. | |
| 15:31:16 | mgoddard_ | efried: good call, I'll add a comment | |
| 15:31:31 | efried | cdent The other code path, update_compute_node, does the same (_ensure_resource_provider) | |
| 15:31:43 | cdent | I just wanted to be sure, because of what people had said earlier about the potential for confusion | |
| 15:32:27 | cdent | those comments about potential for confusion had made it seem like there was a chance that the provider tree could either be wrong or even not exist | |
| 15:32:37 | cdent | but since it is established by _ensure_* is cool | |
| 15:32:51 | efried | cdent I doubt it's perfect | |
| 15:33:30 | cdent | and how does that make you feel? | |
| 15:34:36 | efried | cdent Dirty. So dirty. | |
| 15:34:52 | cdent | woot | |
| 15:35:03 | efried | cdent For one thing, as noted above, I've convinced myself that concurrency isn't an issue YET. | |
| 15:35:33 | efried | cdent And we're also working on the theory of merge big stuff early so we can shake out the bugs. | |
| 15:35:50 | efried | cdent Not that I think it's a great policy to count on shaking out bugs later rather than avoiding them by careful inspection beforehand... | |
| 15:36:13 | efried | cdent But we also can't get into analysis paralysis. Gotta walk the line. | |
| 15:36:22 | cdent | i like merge big stuff early | |
| 15:36:33 | cdent | as long as we actually exercise | |
| 15:36:54 | edleafe | "early" != "days before feature freeze" | |
| 15:37:04 | johnthetubaguy | mriedem: gibi: apologies, lots of things been getting in my way, but I am +2 on the multi-attach now, went back though the merged chain, I don't feel qualified for +W for some reason, but gibi you might be happy with that? | |
| 15:37:16 | efried | Well, unless feature freeze is deliberately early in the cycle, to allow time to exercise. | |
| 15:38:49 | mriedem | johnthetubaguy: thanks; just cleaning up the patch that adds the CI job so it's run in the check/gate queue | |
| 15:39:11 | johnthetubaguy | mriedem: ah, cool, I did see the -1 on there | |
| 15:39:23 | mriedem | i think zuul got lost in the long chain of deps | |
| 15:39:24 | mriedem | rechecking it | |
| 15:39:34 | johnthetubaguy | cool | |
| 15:40:30 | mriedem | johnthetubaguy: sdague: we'll need this bug fix to get the multiattach job in - relies on not using pike UCA https://review.openstack.org/#/c/532214/ | |
| 15:40:42 | mriedem | which means you can't snapshot a paused instance with older libvirt | |
| 15:43:39 | gibi | johnthetubaguy: thanks for the review | |
| 15:45:56 | gibi | mriedem: is it OK for you that I +W the multiattach api patch as john is +2 on it or I should wait for new job to run? | |
| 15:46:03 | efried | rgerganov If you're willing to have https://review.openstack.org/#/c/536348/ rebased onto https://review.openstack.org/#/c/535517/ instead of https://review.openstack.org/#/c/531260/ I'll keep it up to date as I work on those WIPs. | |
| 15:46:17 | mriedem | gibi: should be fine to +W - the CI results have already passed for awhile now | |
| 15:46:19 | efried | rgerganov At the moment you're off on a side branch | |
| 15:46:21 | mriedem | i'm just changing the job config | |
| 15:46:32 | gibi | mriedem: OK, thanks | |
| 15:47:41 | efried | gibi Thanks for the reviews! Knocking 'em out today | |
| 15:50:55 | mriedem | edleafe: looks like https://review.openstack.org/#/c/526436/ needs a rebase? | |
| 15:51:32 | jaypipes | cdent: I see you liked my country music song joke. | |
| 15:51:42 | cdent | quite | |
| 15:52:02 | jaypipes | there's been a plethora of jokes and movie references in reviews on efried's latest patch series. | |
| 15:52:11 | jaypipes | I've been having quite a bit of fun. | |
| 15:52:16 | edleafe | mriedem: working on it | |
| 15:52:39 | edleafe | multiattach stepped on RPC versions | |
| 15:53:31 | mriedem | oh yeah | |
| 15:53:34 | mriedem | it was a race | |
| 15:53:47 | amorin | hello everybody | |
| 15:54:36 | jaypipes | amorin: mornin. | |
| 15:54:59 | amorin | I'd like to know if there is a reason of this if iso9660 line: | |
| 15:55:02 | amorin | https://github.com/openstack/nova/blob/stable/newton/nova/virt/libvirt/driver.py#L6602 | |
| 15:55:12 | amorin | I mean, other possibility is vfat afaik | |
| 15:55:34 | amorin | what if nova is transfering the config drive from remote if it is vfat? | |
| 15:56:31 | amorin | jaypipes: evenin :p | |
| 15:56:39 | amorin | (almost 5 pm here) | |
| 15:56:45 | tovin07 | mriedem, hi | |
| 15:57:59 | jaypipes | amorin: well, evening then :) | |
| 15:58:13 | amorin | :) | |
| 15:59:44 | efried | UGT | |
| 16:00:11 | jaypipes | amorin: as for your question... no idea. perhaps mdbooth or lyarwood might know the answer on that one. | |
| 16:00:42 | amorin | jaypipes mdbooth thanks | |
| 16:02:12 | mdbooth | amorin: What's the question? | |
| 16:02:46 | mdbooth | amorin: Ah, you're wondering why the handling difference between iso9660 and vfat? | |
| 16:02:49 | amorin | what if we copy the config-drive no matter its kind (vfat or iso) | |
| 16:02:55 | amorin | yup | |
| 16:03:04 | efried | cdent "For future reference, in the future this loop could be replaced with a single request to POST /allocations, clearing the allocations for all the consumers." <== This must have been a difficult comment to write. The war between "use lots of little API calls" and "use stuff I wrote!" :P | |
| 16:03:13 | amorin | I understand that libvirt is able to copy it if its vfat | |
| 16:03:28 | amorin | but it seems that if nova copy it first, | |
| 16:03:30 | mdbooth | It was (is? but I doubt it) a bug in libvirt/qemu in the handling of iso9660 disks | |
| 16:03:34 | amorin | then libvirt will do nothing | |
| 16:03:46 | efried | cdent Only joshing you of course. The POST is a great idea there. | |
| 16:03:50 | mdbooth | Did you look at the referenced lp bug? | |
| 16:03:56 | amorin | mdbooth: yes | |
| 16:04:12 | amorin | seems that libvirt is still failing with iso | |
| 16:04:39 | amorin | I was just wondering if copying vfat with nova is a bad idea or not | |
| 16:05:26 | sean-k-mooney | amorin: mdbooth i would guesss the bug in libvirt is related to iso beeing treated as cdroms and vfat ect disk being considered hdds or somthin in that vain? | |
| 16:06:12 | sean-k-mooney | amorin: well one way to check would be remove that line and look at the livemigration gate jobs. it might result in both nova and neutron coping the config drive | |
| 16:06:18 | openstackgerrit | Mark Goddard proposed openstack/nova master: Send traits to ironic on server boot https://review.openstack.org/508116 | |
| 16:06:18 | openstackgerrit | Mark Goddard proposed openstack/nova master: Add get_traits() method to ComputeDriver https://review.openstack.org/532290 | |
| 16:06:19 | openstackgerrit | Mark Goddard proposed openstack/nova master: Implement get_traits() for the ironic virt driver https://review.openstack.org/532288 | |
| 16:06:20 | amorin | problem is, imagine you already spawn instances with iso kind, nova needs to copy it before (because of libvirt bug), but in the meantime, you updated the nova config, so CONF.config_drive = vfat | |
| 16:06:32 | amorin | then you never enter this if, and live-migration fail | |