| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-04-23 | |||
| 18:07:25 | efried | sean-k-mooney: We have ?traits=in:X,Y,Z, don't we?? | |
| 18:07:32 | aspiers | I'm fine with that - it's certainly going to make it easier to justify in the spec | |
| 18:07:38 | sean-k-mooney | efried: there is a spec for that | |
| 18:07:59 | sean-k-mooney | which is the one i linked https://review.opendev.org/#/c/649992/ | |
| 18:10:41 | efried | aspiers: One can envisage starting off with a specific resource class, and then moving to a generic one when the next one is introduced. Resulting in an interesting upgrade problem where you may have to "reshape" existing inventories/allocations from the old resource class to the new one. And we still support mismatched conductor/compute environments, IIRC, though I can never remember which one has to go first, so that could also | |
| 18:11:24 | efried | aspiers: So it's still a debate, yes; it could be made to work either way, yes; but IMO for reasons stated, generic is a bit more future-proof without actually losing you anything. | |
| 18:12:35 | efried | sean-k-mooney: Hmph, I coulda sworn we already had required=in:..., but I don't see it in the api-ref. | |
| 18:13:19 | efried | yup, that spec. | |
| 18:14:08 | sean-k-mooney | ya so they were previously approved for stein but no implemented | |
| 18:15:39 | sean-k-mooney | anyway with a generic resocue class and specific trats we could support that usecase if we hav requrie=in | |
| 18:15:49 | sean-k-mooney | or we could reshape | |
| 18:16:05 | sean-k-mooney | we do however already have the trait | |
| 18:22:44 | efried | mriedem: Would you be able to run the nova meeting this Thursday? I got suckered^wroped into school runs. I should be back around :15-:20. | |
| 18:31:31 | openstackgerrit | Eric Fried proposed openstack/nova master: Hacking N363: Don't use spec[_set]='string' https://review.opendev.org/650370 | |
| 18:36:00 | openstackgerrit | Erik Olof Gunnar Andersson proposed openstack/nova master: Pass on region when we don't have a valid ironic endpoint https://review.opendev.org/654692 | |
| 18:41:19 | mriedem | efried: yeah i think i can do that | |
| 18:41:39 | efried | thanks mriedem | |
| 18:42:06 | openstackgerrit | Surya Seetharaman proposed openstack/nova master: [WIP] Support adding the reason behind a server lock https://review.opendev.org/648662 | |
| 18:42:29 | efried | eandersson: Hmph, I see why you did that; perhaps I need a separate bug for min/max | |
| 18:42:45 | efried | as currently stacked, min/max would have to be partial and yours would have to be closes | |
| 18:42:46 | eandersson | Nah I think your bug is the real fix | |
| 18:42:57 | eandersson | Mine is just a fallback bug fix | |
| 18:43:28 | eandersson | but I am fine with separate bug reports as well | |
| 18:43:41 | efried | eandersson: Mine should really have a bug that says, "ksa endpoint lookup never worked because of min_version" | |
| 18:44:05 | efried | Your bug is about region_name not being respected, which is true regardless of my fix. | |
| 18:44:42 | efried | eandersson: You want to open the other or should I? | |
| 18:45:22 | efried | I mean, credit's to you for finding the problem (and pretty much for tracking down the cause too) so... | |
| 18:47:16 | eandersson | Haha | |
| 18:47:23 | eandersson | Actually yours fixes my bug 99.9% | |
| 18:47:25 | eandersson | of the time | |
| 18:47:52 | eandersson | The only time your fix does not fix my bug is if ServiceNotFound is raised | |
| 18:48:09 | efried | which probably would mean that ironicclient would raise the same | |
| 18:48:17 | eandersson | Very likely | |
| 18:49:07 | efried | I can live with that. I can change mine to Partial-Bug and yours can be Closes-Bug. Or we can swap the order of the fixes. But I don't think we're allowed to have Partial-Bug *after* Closes-Bug. mriedem, a ruling? | |
| 18:49:19 | eandersson | The only difference is that the ironicclient is less opiniated (in it's current implementation) | |
| 18:49:27 | eandersson | Sure | |
| 18:49:35 | efried | ...I guess we can break them apart too. There's actually no reason for them to be in a series, I don't think. | |
| 18:50:32 | mriedem | efried: don't think that matters too much, but partial-bug will change the assignee, whereas related-bug wouldn't | |
| 18:50:37 | mriedem | so i'd use related-bug or separate bugs | |
| 18:51:24 | efried | they're truly both fixing part of the bug. | |
| 18:51:39 | eandersson | I assumed that partial-bug wouldn't change the assignee | |
| 18:51:41 | efried | but okay, if the assignee is the only bit of process that matters | |
| 18:51:46 | eandersson | The more you know | |
| 18:52:17 | efried | and we can always change the assignee | |
| 18:52:27 | efried | so effit, let's go as is. | |
| 18:52:31 | eandersson | Sounds good | |
| 18:53:33 | efried | mriedem: series at https://review.opendev.org/#/c/654457/ when you get a chance -- ironic ksa stuff never worked (/me sheepish); but ironicclient covered for us; but wasn't respecting region_name when it did, we were just getting lucky most of the time. | |
| 18:53:51 | efried | what other core can tackle ksa stuff? | |
| 18:53:57 | tssurya | mriedem: silly question - does the addition of an existing key (locked) to filters/sorting have anything to do with microversions ? as in we just allow them without any checks from the moment we implement them right ? | |
| 18:54:15 | eandersson | It was also partially broken on the ironic client side https://github.com/openstack/python-ironicclient/commit/c038f1db67e7809513d5535648d15c3590b191d5 | |
| 18:54:40 | mriedem | tssurya: there is a whitelist on filters and sorting params, | |
| 18:54:54 | tssurya | right, | |
| 18:54:55 | mriedem | specifying filters that aren't in the whitelist are just ignored for now (gmann has a spec to make that an error), | |
| 18:55:03 | mriedem | specifying sort params that aren't in the whitelist is a 400 | |
| 18:55:22 | mriedem | so you can't add locked as a filter/sort param without a microversion bump | |
| 18:55:39 | mriedem | efried: ack | |
| 18:55:59 | mriedem | tssurya: that's how i verified you couldn't just filter/sort on the instance.hidden field in my change | |
| 18:56:01 | tssurya | mriedem: ah because of change in behaviour for sort params? | |
| 18:56:07 | mriedem | yes | |
| 18:56:19 | mriedem | i thought we said in your spec we weren't going to start filtering/sorting on locked? | |
| 18:56:35 | tssurya | oh we did ? :D | |
| 18:56:43 | mriedem | oh nvm | |
| 18:56:45 | mriedem | "Filtering/Sorting: The locked key will be added to the existing list of valid sorting/filtering keys so that instances can be filtered/sorted based on this field." | |
| 18:56:45 | tssurya | the last iteration was that we would | |
| 18:56:51 | mriedem | i might have been thinking of locked_by | |
| 18:56:54 | tssurya | yea | |
| 18:57:06 | tssurya | okay since I have the bump anyways might as well add this too | |
| 18:57:10 | mriedem | yeah so you'll need a microversion | |
| 18:57:10 | tssurya | thanks mriedem | |
| 18:57:14 | mriedem | np | |
| 19:08:33 | mriedem | gmann: you might want to comment on this https://review.opendev.org/#/c/648919/4/specs/train/approved/int-replace-default-value-of-the-flavor-swap.rst@77 | |
| 19:09:00 | mriedem | it's a small inconsistency cleanup in the api spec so people have said it should be merged into your general cleanup spec, but that's just adding more stuff to your already somewhat large api change | |
| 19:23:29 | mriedem | efried: questions in https://review.opendev.org/#/c/654457/ | |
| 19:23:43 | efried | ... | |
| 19:24:21 | efried | eandersson: ^ | |
| 19:25:12 | mriedem | why not squash those 2 changes together? | |
| 19:27:11 | efried | They're pretty unrelated. | |
| 19:27:47 | efried | one fixes how we do ksa stuff, the other fixes how we construct the ironicclient | |
| 19:29:00 | mriedem | it seems the answer to my question here https://review.opendev.org/#/c/654457/2/nova/virt/ironic/client_wrapper.py@131 though is 'ironicclient will try to sort it out' but needs the region_name passed along as well (if you've got multiple regions and are configuring nova-compute to talk to different ones) | |
| 19:31:08 | openstackgerrit | Surya Seetharaman proposed openstack/nova master: [WIP] Support adding the reason behind a server lock https://review.opendev.org/648662 | |
| 19:31:51 | efried | mriedem: Yes. | |
| 19:32:00 | efried | Though I think that was actually not the case until recently. | |
| 19:32:16 | efried | I think the logic to "figure it out" was broken until https://github.com/openstack/python-ironicclient/commit/c038f1db67e7809513d5535648d15c3590b191d5 ? | |
| 19:33:58 | efried | mriedem: so, do you want these squashed? Am I being silly asserting that they're doing different things? | |
| 19:37:05 | mriedem | well, it looks like it might be better to keep them separate because of how ironicclient handles the kwargs in older releases, it looks like region_name was only since Ifc7b45d047c8882a41021e1604b74d17eac2e6e8 in rocky and before that it was os_region_name | |
| 19:37:15 | mriedem | so backports to queens for eandersson's change could be a bit trickier | |
| 19:37:21 | mriedem | especially since we don't have integration testing for this stuff | |
| 19:41:24 | mriedem | ok +2 on the bottom and +W on the top | |
| 19:41:39 | efried | thanks mriedem. Any idea who else should look at the bottom one? | |
| 19:41:52 | mriedem | https://review.opendev.org/#/q/topic:bug/1825583+(status:open+OR+status:merged) | |
| 19:41:54 | mriedem | cleans that up | |
| 19:41:57 | mriedem | you guys and your silly tp's | |
| 19:42:00 | mriedem | *tb's | |
| 19:42:09 | mriedem | efried: ummm, melwitt? | |
| 19:42:40 | artom | Wheel of cortune | |
| 19:42:46 | efried | nice | |
| 19:43:18 | artom | That's basically the extent of my contribution here. Puns. | |
| 19:49:58 | eandersson | Not backporting mine beyond Rocky is fine, as the main patch will fix most cases. | |
| 19:50:13 | eandersson | And endpoint_override is a valid work around. | |
| 20:00:11 | mriedem | and you're on rocky so you don't have to care about queens :) | |