Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-01-22
15:14:39 efried cdent I'm still pretty confused what you're getting at here https://review.openstack.org/#/c/532290/
15:14:47 bauzas mriedem: btw. I have a question for you
15:15:04 bauzas mriedem: I'm hardly-testing my vGPU changes on a dedicated box
15:15:16 bauzas mriedem: and I'm testing all the instance operations
15:15:31 bauzas mriedem: for providing a good release note about what can be supported and what not
15:15:38 openstackgerrit Matthew Booth proposed openstack/nova master: Local disk serial numbers for the libvirt driver https://review.openstack.org/529380
15:15:46 efried cdent If ironic gets its traits and they're not different from what's in the cache, we won't make the API call. Which is as it should be. Am I missing something?
15:15:50 bauzas mriedem: so, the point is, when I tried to suspend an instance, libvirt popped up an exception
15:15:53 cdent efried: I'm asking for the authors of the patch to acknowledge that they are aware of the quoted lines of provider tree code. This is because elsewhere in the stack there's been discussion of "this doesn't work with provider tree yet"
15:16:03 bauzas mriedem: basically because we don't accept to hot-unplug mediated devices
15:16:11 cdent It could be all that is needed is "yeah, we know, it's fine"
15:16:54 bauzas mriedem: now, the question is, would you be OK if I would just transform that libvirt exception in a more-readable exception like "sorry, but you can't suspend your instance because $vGPUs"
15:17:03 bauzas and leave the instance into ERROR
15:17:20 bauzas of course, the user can reset the state or rebuild
15:17:48 mriedem bauzas: i don't think the instance should be in ERROR state in that case,
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

Earlier   Later