| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-10 | |||
| 12:32:15 | lyarwood | mdbooth: it should be pretty obvious that you can't disconnect a volume before detaching it from the guest tbh | |
| 12:32:30 | lyarwood | mdbooth: and the tests should catch anyone trying to do this | |
| 12:33:54 | kashyap | lyarwood: Sometimes explicit is better than implicit. What is obvious to you would be a far cry from it for a fresh pair of eyes trying to get up2speed. | |
| 12:34:20 | kashyap | I'm personally a fan of documenting in comments, even if sometimes it's a bit obvious if you're in the know | |
| 12:36:28 | ygl | hi all | |
| 12:36:35 | ygl | can someone help me with my issue | |
| 12:43:13 | openstackgerrit | sahid proposed openstack/nova master: hardware: only take into account small pages https://review.openstack.org/532168 | |
| 12:51:59 | lyarwood | *sigh* | |
| 12:52:33 | lyarwood | kashyap: so how would the comment help someone reviewing that method for the first time? | |
| 12:52:49 | lyarwood | kashyap: given that the call to detach the encryptor is now hidden from them in _disconnect_volume | |
| 12:53:18 | lyarwood | kashyap: I'm all for helping first time readers through code but it makes the entire thing more confusing IMHO | |
| 12:55:18 | kashyap | lyarwood: Hmm, if you think it'll confuse more, I'll defer to you. | |
| 12:55:35 | kashyap | lyarwood: I noticed what you said is missing in your review comment | |
| 12:56:35 | kashyap | lyarwood: Then maybe you'd want to note that the call to detach the encryptor is elsewhere :-) | |
| 13:01:43 | takashin | alex_xu: Are you aroud? | |
| 13:02:06 | alex_xu | takashin: yea | |
| 13:02:23 | takashin | alex_xu: Is there API meeting today? | |
| 13:03:16 | alex_xu | takashin: yes, but passed few weeks, there is no people show up, then I didn't run it, is there anything you want to discuss, we can discussed at here I think | |
| 13:03:55 | takashin | alex_xu: Okay. I have 2 patches for reviews. | |
| 13:04:07 | takashin | api-ref: Parameter verification for servers.inc: https://review.openstack.org/#/c/528201/ | |
| 13:04:13 | gmann | alex_xu: takashin i was away too since 2-3 weeks. we can resume from next week may be | |
| 13:04:16 | takashin | api-ref: Example verification for servers.inc: https://review.openstack.org/#/c/529520/ | |
| 13:04:38 | takashin | gmann: thanks. | |
| 13:05:00 | takashin | alex_xu: gmann: Would you review the patches? | |
| 13:05:03 | alex_xu | gmann: cool, we will run it | |
| 13:05:20 | alex_xu | takashin: I add them to my review list, will try to reach them | |
| 13:05:40 | gmann | takashin: ywa, i remember to review those half way last week, ll do tomorrow for sure | |
| 13:05:55 | takashin | alex_xu: gmann: Thank you. | |
| 13:06:00 | takashin | That's all. | |
| 13:06:33 | alex_xu | takashin: did you see my comment https://review.openstack.org/#/c/459483/, I think that is thing we should keep consistent, and I think that isn't worth another microversion | |
| 13:07:22 | takashin | alex_xu: I saw your comment. I will fix it tomorrow. | |
| 13:07:34 | alex_xu | takashin: thanks | |
| 13:08:17 | alex_xu | takashin: gmann btw, there is API patch closed to merge https://review.openstack.org/#/c/330406, I'm reviewing it, but still still found something, it will be great you guys can help review it also, the API patch is really huge :) | |
| 13:08:18 | mdbooth | lyarwood: Anyway, like I said the patch is a huge improvement, I don't see any issues in the code. It's simpler and it fixes at least 3 bugs. I'm just cautious about removing context from a driver which is already plenty opaque in places. | |
| 13:09:00 | gmann | alex_xu: sure, added in my tomorrow list | |
| 13:09:24 | takashin | alex_xu: okay. I will review it tomorrow. | |
| 13:10:33 | alex_xu | gmann: takashin thanks! | |
| 13:14:40 | kashyap | mdbooth: Do you recall top off your head, in what scenarios Nova calls 'qemu-img info' for _running_ guests? | |
| 13:14:50 | kashyap | If not, don't worry, I'll go look into code | |
| 13:15:30 | mdbooth | kashyap: Not off the top of my head, but pretty sure there are some. | |
| 13:16:02 | mdbooth | Look at live migration. Maybe imagecache reaper. | |
| 13:19:47 | kashyap | mdbooth: Okido, I'm in a discussion w/ the QEMU Block folks, and they're asking this. | |
| 13:19:54 | efried | mgoddard Where's set_traits_for_provider (https://review.openstack.org/#/c/532290/1/nova/compute/resource_tracker.py@890) defined? I can't find it in master or in your series. | |
| 13:19:54 | kashyap | I'm sure we do, just have to audit | |
| 13:20:09 | efried | mgoddard I ask because I'm actually in the process of implementing that method right now. | |
| 13:20:21 | efried | mgoddard Want to avoid duplication of effort if possible. | |
| 13:31:43 | mgoddard | efried: hi. I'm still implementing that one. I can submit what I have for review if you'd like to see it | |
| 13:32:25 | efried | mgoddard I would, yes. I'll show you mine if you show me yours :) | |
| 13:33:16 | mgoddard | efried: well I don't usually do this, but go on then | |
| 13:34:06 | efried | mgoddard You're probably a little further along BUT one of the key things I'm doing there is exposing a new exception base class for placement API conflicts and raising subclasses thereof from this method and its brethren (e.g. set_aggregates_for_provider, tbd) when they encounter 409s. | |
| 13:34:52 | efried | mgoddard It's in the middle of a rather messy restack, won't be ready to show for a little while yet. | |
| 13:35:22 | efried | mgoddard But now that I know you're also wanting it for the ironic traits bp, seems it needs to be peeled out of that series. | |
| 13:36:21 | efried | which is probably not super hard. Though at the moment it's based on a change that raises a conflict exception for RP creation, which I think *is* tied pretty heavily into that series. | |
| 13:44:24 | kashyap | mdbooth: Just noting for the record, looked for the past few minutes: | |
| 13:44:26 | kashyap | _rebase_with_qemu_img() , _live_snapshot() and _get_instance_disk_info_from_config() [from nova/virt/libvirt/driver.py] | |
| 13:44:42 | kashyap | nova/virt/libvirt/imagebackend.py: | |
| 13:44:59 | kashyap | - cache() --> fetch_func_sync() --> get_disk_size() --> qemu_img_info() | |
| 13:45:04 | sean-k-mooney | efried: QQ is there a top level api for placement aggregates? e.g. can i list all aggregates or list all resocue providers in an aggregate given the aggregate uuid? | |
| 13:45:21 | efried | sean-k-mooney sec... | |
| 13:46:04 | efried | sean-k-mooney Hum, I thought there was, cause I'm gonna need it. Still looking... | |
| 13:46:46 | sean-k-mooney | efried: no rush. you can get teh aggregates a resouce provider is part of but at least looking at the master docs the recprical api does not appear to exist | |
| 13:46:56 | cdent | you can use member_of to get all rps in a given aggregate | |
| 13:47:07 | efried | Ahh, that's it, thanks cdent | |
| 13:47:17 | cdent | https://developer.openstack.org/api-ref/placement/#list-resource-providers | |
| 13:47:57 | cdent | It's not clear how you're supposed to discovery an aggregate, though, other than by looking at https://developer.openstack.org/api-ref/placement/#list-resource-provider-aggregates | |
| 13:48:06 | cdent | or knowing the uuid prior | |
| 13:48:12 | mdbooth | kashyap: I'm not convinced cache() would be called on a running instance. As always with that code, though, it's far from obvious without checking carefully. | |
| 13:48:28 | mdbooth | kashyap: cache() mostly means 'create' | |
| 13:48:29 | kashyap | mdbooth: Yeah, I should've been careful in pointing that out | |
| 13:48:37 | efried | cdent Yeah. That said, is there a use case for that? | |
| 13:48:50 | sean-k-mooney | cdent: ah yes cool. that still leaves me with one question. how do i create the aggreate in the first place with out a top level aggregates api? | |
| 13:49:18 | cdent | efried: not that I'm aware, but just as I was thinking of it I stumbled on "How do I know the aggregates" | |
| 13:49:27 | cdent | sean-k-mooney: it gets created when you use it | |
| 13:49:40 | kashyap | mdbooth: I didn't do a thorough audit, though. Taking notes as I find instances of it & then see where they're called on a running guest | |
| 13:49:47 | kashyap | For live snapshot we do for sure | |
| 13:50:00 | cdent | so if you PUT to /resource_provider/{uuid}/aggregates with a new uuid there ya go | |
| 13:50:01 | efried | sean-k-mooney You "create" it by assigning it to a provider via PUT /rp/{uuid}/aggs | |
| 13:50:02 | sean-k-mooney | cdent: so the first time i add a resouce provider to an agregate it creates the uuid | |
| 13:50:04 | efried | yeah, what he said. | |
| 13:50:12 | mdbooth | kashyap: Did you look in the imagecache periodic task? | |
| 13:50:12 | cdent | yes | |
| 13:50:43 | kashyap | mdbooth: Not yet; so far I'm just noting down instances where it's called. E.g: | |
| 13:50:46 | kashyap | nova/virt/libvirt/imagebackend.py | |
| 13:50:49 | kashyap | - cache() --> fetch_func_sync() | |
| 13:50:51 | kashyap | --> get_disk_size() [from nova/virt/libvirt/driver.py] | |
| 13:50:54 | kashyap | --> qemu_img_info() [from nova/virt/images.py] | |
| 13:50:56 | kashyap | - verify_base_size() --> get_disk_size() | |
| 13:50:59 | kashyap | - class LVM() --> create_image() --> create_lvm_image() --> get_disk_size() --> qemu_img_info() | |
| 13:51:00 | sean-k-mooney | cdent: so the problem of determining an un used uuid for the aggregate is left to the client | |
| 13:51:09 | cdent | yes | |
| 13:51:12 | kashyap | (And then see the calls for the live guest.) | |
| 13:51:12 | efried | sean-k-mooney I knew I wrote that code: https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L431 | |
| 13:51:44 | sean-k-mooney | cdent: ok cool. more reading for me to do :) | |
| 13:51:48 | cdent | sean-k-mooney: there's some stuff in the very early rp specs about such things | |
| 13:51:49 | efried | sean-k-mooney There shouldn't be any such thing as an "unused" aggregate UUID. Unless you mean "used only for one provider", which is pointless. | |
| 13:52:14 | cdent | but if I remember right, the thinking was that the client would already have some identifier in min | |
| 13:52:15 | cdent | d | |
| 13:52:30 | efried | ...because when you remove the last association, placement ought to get rid of that agg ID. | |
| 13:53:02 | efried | cdent Yeah, that's a weird one. In the PowerVM SSP case, the only thing that makes sense is to give the agg the same UUID as the shared storage pool | |
| 13:53:22 | efried | Though I *suppose* I could give the agg the UUID of the cluster instead. It's effectively the same thing in powervm land. | |