| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-23 | |||
| 13:06:26 | cdent | sdague, bauzas: If you pass in a float on that thing it will raise an AtributeError, at import time. Is that sufficient, or would you prefer that exception(s) raised from parse_version_string be more custom? I tend to prefer built in exceptions for this kind of thing. | |
| 13:07:16 | bauzas | cdent: I think sdague asked for an unittest verifying that behaviour | |
| 13:07:26 | cdent | yes, i've written the test | |
| 13:07:31 | bauzas | not what happens if someone uses that deco incorrectly | |
| 13:07:58 | bauzas | cdent: tbc, the former is enough from my PoV | |
| 13:08:20 | bauzas | if we have a docstring that explains the deco is awaiting for a string, and then the developer is using it incorrectly, then meh | |
| 13:09:08 | bauzas | it's like not following a manual when using a chainsaw | |
| 13:11:28 | sdague | cdent: as long as that would fail a commit I'm good with it | |
| 13:11:46 | cdent | sdague: yeah, code will boom | |
| 13:11:53 | sdague | cdent: wfm | |
| 13:13:31 | cdent | sdague: yeah: AttributeError: 'float' object has no attribute 'split'\nThe test run didn't actually run any tests" | |
| 13:13:48 | cdent | (that's on the functional tests) | |
| 13:13:53 | sdague | cool | |
| 13:22:34 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Don't use floats in microversion handling https://review.openstack.org/449119 | |
| 13:27:23 | cdent | sdague, bauzas: that ^ should do it | |
| 13:29:32 | sdague | cdent: frickler has a comment on the last patch | |
| 13:31:29 | bauzas | sdague: frickler missed the transformation into a tuple | |
| 13:32:35 | cdent | yeah, it's a tuple by then | |
| 13:33:05 | sdague | so, is there a reason not to move APIVersionRequest into the common library and reuse all of that? | |
| 13:33:59 | cdent | sdague: no reason other than time | |
| 13:34:56 | cdent | sdague: I also thought that writing something fresh would allow for some compare and contrast so we could have a mix of both in whatever the common thing was | |
| 13:35:08 | cdent | there are some differences in approach | |
| 13:35:12 | cdent | (nothing major) | |
| 13:35:23 | sdague | yeh, it feels like mostly we're just rediscovering all the bugs again :) | |
| 13:35:27 | cdent | heh | |
| 13:35:48 | sdague | I honestly remember this exact issue when we hit 2.10 | |
| 13:36:03 | efried | lifeless Yes, https://github.com/testing-cabal/mock/issues/398 is me. | |
| 13:36:47 | efried | mriedem I've got a doc appt during the nova meeting today. I left my update in the agenda. | |
| 13:38:11 | cdent | sdague: sure. the other two issues were: didn't want to reuse nova code because I thought we'd be moving soon; and when I first did this I couldn't seem to untangle the way in which nova was doing version_handler from other parts of the wsgi stack. | |
| 13:38:30 | cdent | In other words I found the code too hard to read, so wrote some more. Which is sad but true. | |
| 13:39:47 | cdent | (it's the stuff in Controller that bit me) | |
| 13:44:25 | mriedem | efried: ok | |
| 13:46:16 | sdague | cdent: ok, but you've effectively replicated the APIVersionRequest object as a tuple now. It feels like that's going to be a pretty direct transition | |
| 13:46:37 | openstackgerrit | Eric Fried proposed openstack/nova master: PowerVM Driver: spawn/delete #1: no-ops https://review.openstack.org/438119 | |
| 13:46:47 | openstackgerrit | Hironori Shiina proposed openstack/nova-specs master: Ironic: Cold migration support https://review.openstack.org/449155 | |
| 13:47:31 | cdent | sdague; that part is not the interesting/useful/verbose part of the handling though. It is the users of the Version concept, and those parts between placement and nova are much different (simply because the framework is too) | |
| 13:47:44 | cdent | I agree with you that there's some duplication, and that's unfortunate, but it's where we are right now | |
| 13:48:12 | cdent | and I'm not too het up about repeating bug fixes, it is good for learning, for refactoring, and refreshing, especially when they are caught before they leak | |
| 13:48:42 | cdent | we had the review process for all this work, and it missed some things, that's okay, that's life. | |
| 14:00:11 | mriedem | nova meeting time | |
| 14:11:27 | moshele | sean-k-mooney: hi, can you review the https://review.openstack.org/#/c/398277/ | |
| 14:18:15 | openstackgerrit | Gábor Antal proposed openstack/nova master: Transform instance.volume_attach notification https://review.openstack.org/401992 | |
| 14:18:16 | openstackgerrit | Gábor Antal proposed openstack/nova master: Transform instance.volume_detach notification https://review.openstack.org/408676 | |
| 14:19:26 | jgriffith | lyarwood sorry, I thought you were indicating that your patches that replaced mine had merged; still waiting before refactoring everything | |
| 14:20:12 | jgriffith | lyarwood looks like there's still some dialogue to hash out | |
| 14:21:00 | johnthetubaguy | I think lyarwood is out for a week, not 100% sure | |
| 14:22:10 | openstackgerrit | Gábor Antal proposed openstack/nova master: Transform instance.volume_attach notification https://review.openstack.org/401992 | |
| 14:55:19 | sdague | where are servers assigned to server_groups? | |
| 14:56:02 | dansmith | sdague: hint group=foo | |
| 14:56:54 | sdague | oh, it's buried inside hints? | |
| 14:57:32 | gibi | mriedem: during the nova meeting I got the info from sjmc7 about the needed transformations | |
| 14:57:47 | sdague | which aren't documented... that's the awesome :) | |
| 14:58:30 | gibi | mriedem: < sjmc7> gibi: the only notification we rely on that's not implemented is compute.instance.volume.attach/detach | |
| 14:58:54 | gibi | mriedem: so we are on a good track as those notifications already in focus | |
| 14:59:47 | Kevin_Zheng | https://bugs.launchpad.net/nova/+bug/1675276 | |
| 14:59:47 | openstack | Launchpad bug 1675276 in OpenStack Compute (nova) "Volumes attached to shelved instance may contain incorrect device_name" [Undecided,In progress] - Assigned to Zhenyu Zheng (zhengzhenyu) | |
| 15:00:25 | mriedem | Kevin_Zheng: the device_name is never guaranteed | |
| 15:00:39 | bauzas | sdague: yup | |
| 15:00:39 | dansmith | sdague: yep, create a group foo, then boot an instance with hint group=foo | |
| 15:00:43 | mriedem | Kevin_Zheng: we've talked about just removing the device_name from the os-volume_attachments API in the past | |
| 15:00:46 | bauzas | sdague: you create a group thru the API | |
| 15:00:53 | bauzas | and what dansmith said | |
| 15:01:15 | bauzas | no way to remove an instance from a group, long-lived story | |
| 15:01:43 | Kevin_Zheng | mriedem: Hmm OK，what I dicovered is | |
| 15:01:43 | mriedem | there have been specs for that though, adding and removing members | |
| 15:01:46 | mriedem | i remember those from kilo | |
| 15:01:56 | bauzas | yup | |
| 15:02:02 | bauzas | and I can name the person :) | |
| 15:02:08 | bauzas | who is now a Watcher core :) | |
| 15:02:26 | bauzas | anyway, my point is | |
| 15:02:27 | Kevin_Zheng | cinder can save wrong device name for shelved instance | |
| 15:02:53 | Kevin_Zheng | could you check for detail？ on my bug report？ | |
| 15:02:59 | bauzas | I'm not sure the servergroup API is the good abstract model for describing colocation rules for placement between instances | |
| 15:03:01 | sdague | yeh, it's just without reading nova code you can't use any of this, there is nothing in the API ref that connects all this together (slight asside, but a little relevant for figuring out a path forward) | |
| 15:03:15 | mriedem | Kevin_Zheng: yeah i just read it | |
| 15:03:26 | mriedem | Kevin_Zheng: i think we have to pass a mountpoint to the cinder os-attach API | |
| 15:04:05 | Kevin_Zheng | device name didnt got checked for shelved instance | |
| 15:04:20 | Kevin_Zheng | so I can | |
| 15:04:35 | Kevin_Zheng | pass vda and cinder will save vda | |
| 15:05:02 | mriedem | Kevin_Zheng: right we don't check the device name for the instance during attach to a shelved offloaded instance because the instance does not have a host, so we can't call the compute to reserve the device name, like during normal volume attach | |
| 15:05:29 | Kevin_Zheng | yeah，I was thinking do | |
| 15:05:45 | mriedem | Kevin_Zheng: that's why we create the BDM with device_name=None here http://git.openstack.org/cgit/openstack/nova/tree/nova/compute/api.py#n3678 | |
| 15:05:48 | Kevin_Zheng | do something once we got the right name | |
| 15:05:55 | mriedem | otherwise we do http://git.openstack.org/cgit/openstack/nova/tree/nova/compute/api.py#n3687 | |
| 15:06:13 | mriedem | Kevin_Zheng: are you proposing that we set the device_name in the BDM after unshelve? | |
| 15:06:20 | madhaviy | @mriedem, one question on nova/network/neutronv2/api's _check_external_network_attach() method. This is checking whether user/context can attach a network which is marked as external. so when this code is executed during deploy/spawn, policy checking is been done on compute host. Do we need to validate this even before | |
| 15:06:24 | Kevin_Zheng | yeah I did some learning this afternoon | |
| 15:06:30 | gibi | bauzas: there was a proposal from jaypipes to revisit the server group concept here https://review.openstack.org/#/c/183837/4/specs/liberty/approved/generic-scheduling-policies.rst | |
| 15:06:37 | mriedem | Kevin_Zheng: i think that might still be a lie though, | |
| 15:06:52 | mriedem | because just because the user asked for /dev/vda doesn't mean that's where it will show up on the guest | |
| 15:06:58 | mriedem | even though we stored that in cinder | |
| 15:07:03 | Kevin_Zheng | yeah | |
| 15:07:16 | mriedem | this is really why we need to just have a microversion to take device_name out of the volume attach API :) | |
| 15:07:21 | mriedem | because we can't honor it | |
| 15:07:27 | mriedem | we don't even try with the libvirt driver | |
| 15:07:29 | mriedem | we ignore it | |
| 15:07:32 | Kevin_Zheng | yeah | |
| 15:07:45 | Kevin_Zheng | I will try to put up one | |
| 15:07:56 | Kevin_Zheng | if nobody does | |
| 15:08:03 | mriedem | thanks, this is something we've talked about in previous summits and meetups and just never had someone write a spec | |