| *** hemna6 is now known as hemna | 07:37 | |
| opendevreview | Pierre Libeau proposed openstack/nova master: Move file system freeze after end of mirroring https://review.opendev.org/c/openstack/nova/+/803713 | 08:54 |
|---|---|---|
| kashyap | sean-k-mooney: Will look the SMM thing | 09:21 |
| plibeau3 | bauzas: hello when you have time to read https://review.opendev.org/c/openstack/nova/+/803713/ QEMU community give some input | 09:55 |
| bauzas | plibeau3: ack, I can take a look on it this afternoon | 09:59 |
| * bauzas is fighting with Tempest as of now | 09:59 | |
| stephenfin | sean-k-mooney: Can you look at https://review.opendev.org/c/openstack/nova/+/822749/ today? | 09:59 |
| opendevreview | Imran Hussain proposed openstack/nova master: [nova/libvirt] Support for checking and enabling SMM when needed https://review.opendev.org/c/openstack/nova/+/825496 | 10:34 |
| opendevreview | Imran Hussain proposed openstack/nova master: [nova/libvirt] Support for checking and enabling SMM when needed https://review.opendev.org/c/openstack/nova/+/825496 | 11:10 |
| * Uggla don't find where we can vote for Z release name. Can someone provide the link to vote ? | 11:24 | |
| *** cozyurt is now known as Guest1271 | 11:43 | |
| gibi | Uggla: everybody could nominate Z names, but only the TC votes (this will change in A release) | 11:51 |
| Uggla | gibi, oh ok thx gibi. | 11:52 |
| sean-k-mooney | stephenfin: yep ill look now | 11:59 |
| sean-k-mooney | stephenfin: the only feedback i had is do we want to have the extras installed in the ci but you might be doing that in the next patch | 12:01 |
| sean-k-mooney | which you are | 12:01 |
| sean-k-mooney | so no im happy with both patches | 12:01 |
| stephenfin | openstack server create --flavor m1.small --image fedora-iso --port e3caceae-2c74-4114-9f3f-262a37fc1971 --port e3caceae-2c74-4114-9f3f-262a37fc1971 test-server --wait | 12:02 |
| stephenfin | oslo_db.exception.DBDuplicateEntry: (pymysql.err.IntegrityError) (1062, "Duplic | 12:02 |
| stephenfin | ate entry 'fa:16:3e:62:88:38/e3caceae-2c74-4114-9f3f-262a37fc1971-0' for key 'virtual_interfaces.uniq_virtual_interfaces0address0deleted'") | 12:02 |
| sean-k-mooney | stephenfin: yep | 12:04 |
| sean-k-mooney | repating the port fails there | 12:04 |
| sean-k-mooney | this is a known failure mode | 12:05 |
| sean-k-mooney | we could block that in the api | 12:05 |
| stephenfin | We probably should | 12:05 |
| stephenfin | There were a few bugs related to this when I Googled but none had resolutions in them | 12:05 |
| sean-k-mooney | ya i have seen customer hit this with heat templates and i think the shift on stack installer hit this about 4 mounts ago | 12:09 |
| sean-k-mooney | i previously chatted to mdbooth about it. | 12:09 |
| sean-k-mooney | i know its recently come up again downstream. are you looking at that? | 12:10 |
| stephenfin | yup, spotted downstream | 12:16 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add microversion 1.39 to support any-trait queries https://review.opendev.org/c/openstack/placement/+/826719 | 12:17 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add microversion 1.39 to support any-trait queries https://review.opendev.org/c/openstack/placement/+/826719 | 12:19 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add microversion 1.39 to support any-trait queries https://review.opendev.org/c/openstack/placement/+/826719 | 12:23 |
| dmitriis | sean-k-mooney: can't cleanly apply some changes on top of https://review.opendev.org/c/openstack/nova/+/819494/ since I rebased mine to the tip. But I removed the conflicting modifications in bind-time/plug-time event related functions which are getting removed in that change. | 13:01 |
| dmitriis | (wondering if it's OK to rebase Artom's PRs myself) | 13:02 |
| sean-k-mooney | ack am you could artom will be online soon if you want to wait and ask but i dont really see any harm in that. i would move it after the os-traits patch | 13:03 |
| sean-k-mooney | alternitvily i can review those now and see if we can merge it shortly | 13:04 |
| dmitriis | sean-k-mooney: ack, let me rebase both of his changes and I can cherry-pick cleanly then and reorder the VNIC_SMARTNIC patch and the os-traits patch | 13:07 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Reject duplicate port IDs in server create https://review.opendev.org/c/openstack/nova/+/827070 | 13:07 |
| stephenfin | sean-k-mooney: ^ (gibi, you might interested also) | 13:08 |
| sean-k-mooney | alreday have it open :) | 13:08 |
| sean-k-mooney | i assume a 400 will be returned | 13:08 |
| stephenfin | yup | 13:08 |
| opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: Add nova-ovs-hybrid-plug job https://review.opendev.org/c/openstack/nova/+/817303 | 13:08 |
| stephenfin | I'm really hoping to avoid a microversion for that, since it's so obviously wrong as-is | 13:08 |
| stephenfin | but I can't make that choice alone, of course :) | 13:09 |
| sean-k-mooney | 400 is already a valid return code | 13:09 |
| sean-k-mooney | so no microversion needed | 13:09 |
| stephenfin | right, but it's currently returning a 201 | 13:09 |
| stephenfin | since server create is async | 13:09 |
| sean-k-mooney | its returning a 201 but the server end in error | 13:09 |
| stephenfin | yeah | 13:09 |
| sean-k-mooney | i think we have an excpetion for that | 13:09 |
| * sean-k-mooney looks at microversion doc | 13:09 | |
| stephenfin | so I'm arguing that since every other subsequent server op would fail, this is an acceptable change. We shouldn't break anyones automation | 13:10 |
| sean-k-mooney | https://docs.openstack.org/nova/latest/contributor/microversions.html#when-do-i-need-a-new-microversion | 13:10 |
| sean-k-mooney | so i would argue that thies is the first yes | 13:10 |
| sean-k-mooney | did we silently fail to do what is asked | 13:10 |
| opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: Revert "Revert resize: wait for events according to hybrid plug" https://review.opendev.org/c/openstack/nova/+/819494 | 13:11 |
| sean-k-mooney | we log the error for the duplciate keys in nova-compute but i think the user got a no valid host error in the event log | 13:11 |
| stephenfin | we don't even get as far as scheduling | 13:12 |
| stephenfin | but ultimately we silently fail to do what is asked, yes | 13:13 |
| sean-k-mooney | oh we create teh entry in the virtual interfaces table in the api i gues so we can store any tag infor | 13:13 |
| sean-k-mooney | ok well ya so im pro no microververion for this too and backporting it | 13:13 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Reject duplicate port IDs in server create https://review.opendev.org/c/openstack/nova/+/827070 | 13:14 |
| sean-k-mooney | stephenfin: since your about can you quickly look at https://review.opendev.org/c/openstack/nova/+/817303/11 and the followup revert form artom | 13:15 |
| sean-k-mooney | dmitriis: ping me later today when the full series is rebased and up and ill try and do a full pass form start to finish | 13:16 |
| stephenfin | sean-k-mooney: Sure, I won't do it right now but I'll get to it straight after lunch | 13:16 |
| sean-k-mooney | dmitriis: ill be busy for the next hour or so but ill try to get to it later | 13:16 |
| sean-k-mooney | stephenfin: ack | 13:16 |
| chateaulav | sean-k-mooney: what would be the best method for testing my ci job? | 13:17 |
| dmitriis | sean-k-mooney: ack, on it now. I'll add a release note to the latest commit as well. I'm going to work on a doc change as well after I resubmit. | 13:18 |
| *** dasm|off is now known as dasm | 13:19 | |
| *** dasm is now known as dasm|rover | 13:19 | |
| sean-k-mooney | chateaulav: typically we submit a DNM patch at the end of a series, in that you can disable most of the intree ci jobs and then test your job by doing <ci>-recheck or whatever the comment is that you use for your ci | 13:28 |
| sean-k-mooney | chateaulav: [DNM] means do not merge and typiclay do not review | 13:28 |
| sean-k-mooney | we use it when ever we are doing things to see if they work or when we are workign on infra like the ci | 13:29 |
| gibi | stephenfin: +2 on your fix. thanks | 13:29 |
| chateaulav | gotcha | 13:32 |
| sean-k-mooney | chateaulav: here is an example of working on a first party job https://review.opendev.org/c/openstack/nova/+/727228/1/.zuul.yaml you would do the same for third party just leave the -check-requiremetns template enabled and comment out the check an gate jobs then you can recheck usign your third party ci comment | 13:32 |
| chateaulav | sean-k-mooney: thanks! | 13:32 |
| sean-k-mooney | chateaulav: no problem | 13:32 |
| plibeau4 | lyarwood: https://review.opendev.org/c/openstack/nova/+/820531 when you have time :) | 13:37 |
| gibi | sean-k-mooney: filed bug about placement silently ignoring repeated required query params https://storyboard.openstack.org/#!/story/2009816 I know that you would like to fix it as a bugfix to return http400 (option a) in the bugreport) | 13:43 |
| gibi | bauzas, gmann, melwitt: ^^ Do you agree? | 13:44 |
| bauzas | sorry folks, I had a problem with my internet | 13:44 |
| sean-k-mooney | gibi: that would be my perfernce yes but lets see how the rest feel | 13:44 |
| bauzas | (not my internet, rather my router) | 13:44 |
| sean-k-mooney | gibi: if we think we cant do that without a microverion im ok with what you have previously propsoed in the patch | 13:45 |
| gibi | sean-k-mooney: ack, I will prepare a bugfix as I also think this should be fixed | 13:46 |
| bauzas | gibi: hah, I finally saw the story | 13:47 |
| bauzas | gibi: when I was calling the link, it wasn't giving me the story | 13:47 |
| bauzas | gibi: looks to me a correct bug | 13:48 |
| bauzas | for the solution, yeah, HTTP400 without needing a microversion I think | 13:48 |
| bauzas | but let's wait for gmann's thoughts | 13:49 |
| sean-k-mooney | gibi: oh thats an interesting edge case this also hides invlaid "standard" traits like that interesting. it makes sense but that is even more broken then just ignoring some of your requirements when selecting resouces | 13:50 |
| gibi | yepp it hides invalid things | 13:50 |
| *** cozyurt is now known as Guest1287 | 14:40 | |
| gibi | incoming... | 14:44 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Refactor trait normalization https://review.opendev.org/c/openstack/placement/+/825847 | 14:44 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Extend the RP db query to support any-traits https://review.opendev.org/c/openstack/placement/+/825848 | 14:44 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Reproduce bug story/2009816 https://review.opendev.org/c/openstack/placement/+/827114 | 14:44 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Reject repeated required[N] param https://review.opendev.org/c/openstack/placement/+/827115 | 14:44 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Extra tests around required traits https://review.opendev.org/c/openstack/placement/+/827116 | 14:44 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: DB layer should only depend on trait id not names https://review.opendev.org/c/openstack/placement/+/826490 | 14:44 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Enhance doc of _get_trees_with_traits https://review.opendev.org/c/openstack/placement/+/825780 | 14:44 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Extend the RP tree DB query to support any-traits https://review.opendev.org/c/openstack/placement/+/825849 | 14:44 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add any-traits support for listing resource providers https://review.opendev.org/c/openstack/placement/+/826491 | 14:45 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add any-traits support for allocation candidates https://review.opendev.org/c/openstack/placement/+/826492 | 14:45 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Remove unused compatibility code https://review.opendev.org/c/openstack/placement/+/826493 | 14:45 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add microversion 1.39 to support any-trait queries https://review.opendev.org/c/openstack/placement/+/826719 | 14:45 |
| gmann | sean-k-mooney: stephenfin +1 on changing return code from 201 to 400 without microversion in case of 'Reject duplicate port IDs in' because server goes in error at the end | 14:51 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Extra tests around required traits https://review.opendev.org/c/openstack/placement/+/825846 | 14:51 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Refactor trait normalization https://review.opendev.org/c/openstack/placement/+/825847 | 14:51 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Extend the RP db query to support any-traits https://review.opendev.org/c/openstack/placement/+/825848 | 14:51 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: DB layer should only depend on trait id not names https://review.opendev.org/c/openstack/placement/+/826490 | 14:52 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Enhance doc of _get_trees_with_traits https://review.opendev.org/c/openstack/placement/+/825780 | 14:52 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Extend the RP tree DB query to support any-traits https://review.opendev.org/c/openstack/placement/+/825849 | 14:52 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add any-traits support for listing resource providers https://review.opendev.org/c/openstack/placement/+/826491 | 14:52 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add any-traits support for allocation candidates https://review.opendev.org/c/openstack/placement/+/826492 | 14:52 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Remove unused compatibility code https://review.opendev.org/c/openstack/placement/+/826493 | 14:52 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add microversion 1.39 to support any-trait queries https://review.opendev.org/c/openstack/placement/+/826719 | 14:52 |
| gmann | gibi: sean-k-mooney bauzas for placement API ignoring the extra param in query seems like API change which impact users. We should not do that without microversion because of interop. | 14:52 |
| gibi | gmann: even if ignoring a repeated silently hides an error? | 14:53 |
| gibi | *repeated param | 14:53 |
| gmann | gibi: sean-k-mooney bauzas we fixed that in nova with microversion only - https://specs.openstack.org/openstack/nova-specs/specs/train/implemented/api-consistency-cleanup.html#proposed-change | 14:54 |
| gibi | e.g. the user asked for an RP that has both trait A and trait B but because A is ignored it gets RPs with only trait B | 14:54 |
| gibi | this is a logic error not just inconvinience | 14:55 |
| gibi | so it is not about ignoring invalid query params | 14:56 |
| gmann | gibi: but that is what user asked, 'trait B' at the end of query he mentioned. it can be used in confusion as I am mentioning two value in single field and API should accept it as both but that is wring query | 14:56 |
| gmann | gibi: yeah so this is only if user query in for same field. | 14:57 |
| gibi | I don't think that when the user said required=A&required=B she meant that only apply B filter | 14:57 |
| gmann | how they can do multiple query ? for 'required' A and B both together? | 14:57 |
| gibi | today with required=A,B | 14:58 |
| gibi | after microversion 1.39 required=A&required=B will work too | 14:58 |
| gibi | but today required=A&required=B is paresed as required=B by placement | 14:58 |
| gmann | gibi: so you are fixing it with 400 or allow required=A&required=B to return A and B ? | 15:00 |
| gibi | I have a bugfix without microversion bump that retuns http400 if required is repeated | 15:00 |
| gibi | and I have a feature with a microversion bump that allows repeating and pareses it as A and B | 15:01 |
| gibi | (and that microversion adds other things to required too) | 15:01 |
| gibi | gmann: this is the series, the bottom is the bugifx https://review.opendev.org/q/topic:any-traits-support | 15:01 |
| opendevreview | Stephen Finucane proposed openstack/nova stable/xena: api: Reject duplicate port IDs in server create https://review.opendev.org/c/openstack/nova/+/827120 | 15:02 |
| gmann | ok so changing behavior of " required=A&required=B " with microversion but for older microversion we will change 200 -> 400 right ? | 15:02 |
| sean-k-mooney | gmann: yes | 15:02 |
| sean-k-mooney | today if you are repeating the required parmater you have a bug in the code that is calling placement | 15:03 |
| sean-k-mooney | with the new version there will be a vaild meanign for that and that will be supproted with the new microverion | 15:03 |
| gibi | gmann: yes | 15:04 |
| gmann | 'changing behavior of " required=A&required=B " with microversion' is all good, I am thinking on for old microverison change | 15:04 |
| gibi | btw efried is on gmann's side in the code review... | 15:04 |
| gmann | which break users | 15:04 |
| sean-k-mooney | gmann: it wont break users | 15:04 |
| sean-k-mooney | gmann: user that have a duplciat request are already broken | 15:04 |
| gibi | gmann: it breaks uses if the user query was wrong in the first place | 15:04 |
| efried | ^ | 15:05 |
| gmann | sean-k-mooney: 200 to 400. when user asking with wring query | 15:05 |
| sean-k-mooney | yes it will only be 400 for an invild query | 15:05 |
| sean-k-mooney | where one of the requiremnt was being ignored | 15:05 |
| sean-k-mooney | nova wont generate this but if we were to consider the isolated aggreates feature | 15:06 |
| sean-k-mooney | https://docs.openstack.org/nova/latest/reference/isolate-aggregates.html | 15:06 |
| gibi | I can imagine one case when we break a valid query. required=A,B&required=A,B is valid today and has a good meaning but it will be HTTP 400 after the fix | 15:07 |
| sean-k-mooney | if we had &required:CUSTOM_LICENSED_WINDOWS&required:CUSTOM_SOMETHING_ELSE | 15:07 |
| sean-k-mooney | the the isolated aggreate part would be lost | 15:07 |
| opendevreview | Stephen Finucane proposed openstack/nova stable/wallaby: api: Reject duplicate port IDs in server create https://review.opendev.org/c/openstack/nova/+/827121 | 15:07 |
| sean-k-mooney | gibi: two idential values i guess we could check for and allow that | 15:08 |
| opendevreview | Stephen Finucane proposed openstack/nova stable/victoria: api: Reject duplicate port IDs in server create https://review.opendev.org/c/openstack/nova/+/827122 | 15:08 |
| gibi | sean-k-mooney: we coudl but that gets complicate if we need to allow required=A,B&required=B,A as well | 15:08 |
| sean-k-mooney | gibi: it does and i would still consider that to be invalid today | 15:09 |
| opendevreview | Stephen Finucane proposed openstack/nova stable/ussuri: api: Reject duplicate port IDs in server create https://review.opendev.org/c/openstack/nova/+/827123 | 15:09 |
| sean-k-mooney | it makes logical sense but the parmater is not intended to be repatable today | 15:09 |
| efried | IMO going into the weeds to allow special corner cases is not The Way. I think we should fix this, but it would be best to do it in a microversion. | 15:09 |
| efried | And by "we" of course I mean "y'all" :P | 15:09 |
| sean-k-mooney | efried: we cant backport it then | 15:09 |
| sean-k-mooney | we can certenly fix it in a microverion if that is what we want | 15:10 |
| sean-k-mooney | but if we do that we need to docuemnt that for older microversion only the last requires clasue will be used | 15:10 |
| gibi | we have the microversion to fix it above in that series :) | 15:10 |
| sean-k-mooney | and we need to ensure we dont break that goign forward for older microverion so tech debt | 15:10 |
| opendevreview | Imran Hussain proposed openstack/nova master: [nova/libvirt] Support for checking and enabling SMM when needed https://review.opendev.org/c/openstack/nova/+/825496 | 15:11 |
| sean-k-mooney | gibi: yes i guess it fixed implictly by the new fature | 15:11 |
| sean-k-mooney | *feature | 15:11 |
| gibi | sean-k-mooney: yes | 15:11 |
| gmann | f I am using it required=A,B&required=B with expectation that I need B (onerously wrong way) but it will be broken if we do without microversion | 15:11 |
| efried | such is life. I thought the whole point of the microversion thing was that we weren't locked into major versions where we had to worry about backporting at all. You use whatever microversion you use, from whatever version of other stuff you are using, to support whatever features you need. | 15:11 |
| gmann | and if we are changing the behavior with microversion then we are fixing it anyways so no need to fix for older microversion | 15:11 |
| efried | ^ | 15:12 |
| gmann | gibi: if you would change it from 400 ever then we might think to do without microversion or not. otherwise it is like required=A,B&required=B is 200 now, required=A,B&required=B is 400 after fix required=A,B&required=B is again 200 with microversion | 15:12 |
| gmann | * if you would change it from 400 always | 15:13 |
| sean-k-mooney | efried: iguess we were just tryign to avoid operators having upgrade pain | 15:13 |
| gmann | * if you would change it to 400 always | 15:13 |
| sean-k-mooney | nova will not repeat the parmaters today | 15:13 |
| sean-k-mooney | so for nova this wont matter one way of another | 15:13 |
| efried | yeah, as usual the reality is that it is vanishingly unlikely that anything in the field is going to be affected by this fix; it's a matter of doing The Right Thing software hygiene process-wise. | 15:14 |
| gibi | I feel more hygenic if we not silently ignore query params | 15:14 |
| sean-k-mooney | right so hygine wise accepting known invlaid input to me is wrong | 15:15 |
| efried | um, that's not the point. I don't think anyone is disputing that there's a bug. It's a matter of the proper procedure to get the fix done. | 15:15 |
| opendevreview | Stephen Finucane proposed openstack/nova stable/train: api: Reject duplicate port IDs in server create https://review.opendev.org/c/openstack/nova/+/827126 | 15:15 |
| gibi | I cannot fix the bug on stable branches if it is requires a microversion bump | 15:15 |
| gmann | efried: yeah. | 15:16 |
| efried | stable branches of what? | 15:16 |
| sean-k-mooney | placment | 15:16 |
| gibi | placenet | 15:16 |
| gibi | placement | 15:16 |
| efried | And those stable branches affect... what? Only OpenStack? | 15:16 |
| gmann | it is definitely a bug but our code has made it working in some way even with not expected result | 15:16 |
| gibi | efried: this argument can be used for any placement change. should we then drop all placement stable branches? | 15:17 |
| gmann | or may be expected results as we cannot say users would not be using it for query B only but in mistaken way of leaving duplicate query | 15:17 |
| efried | IMHO documenting the old broken behavior for stable branches would be adequate. | 15:18 |
| efried | Stable consumers who are affected and care can fix their query if they wish. | 15:18 |
| gmann | gibi: sean-k-mooney efried I feel best way for existing user or stable is to update the api-ref "required=A,B&required=B is unexpected behavior until microversion 1.39 and after it work like returning both " | 15:18 |
| gmann | yeah documenting it is enough I think when we are anyways fixing it with microversion | 15:19 |
| gmann | that is how we do for any other bugs also right? 'fix in new microversion and old microverison it is bug and we donot fix' | 15:20 |
| sean-k-mooney | gmann: well the primary reason for supprot it in 1.39 is for the any traits feature | 15:20 |
| gibi | I don't believe that we want to keep around an incorrect behavior just because somebody might depend on it. but at the same time we say stable consumers can fix their query. I would say master consumer can fix their query too after we forbid the wrong behavior | 15:20 |
| gmann | sean-k-mooney: yeah, I mean if we are fixing this query format too | 15:20 |
| sean-k-mooney | gmann: if one of the repeated section does not use in: you still shoudl not be using it | 15:21 |
| gibi | sean-k-mooney: after 1.39 you can repeate required without in: prefix as well | 15:21 |
| sean-k-mooney | yes but im not sure we shoudl actully allow that | 15:21 |
| sean-k-mooney | it simpler too | 15:21 |
| sean-k-mooney | but its not useing the api as intended | 15:22 |
| sean-k-mooney | we can allow it but i dont think we shoudl encurage it | 15:22 |
| sean-k-mooney | after all it we have a limited query string lenght | 15:22 |
| sean-k-mooney | and this is much more wasteful the jsut combining them | 15:22 |
| sean-k-mooney | so it may be valild but not idiomatic | 15:23 |
| * gibi feels burnout on this and goes for vacuuming the carpet instead | 15:23 | |
| gibi | I will be back in an hour | 15:23 |
| gibi | sorry | 15:23 |
| sean-k-mooney | sorry didnt want to burn you out either | 15:24 |
| sean-k-mooney | im not really asking for you to expiclty block the repated case without in specificaly but i woudl like us to docuemnt that it shoudl only be used for that usecase ideally | 15:25 |
| gmann | I think blocking it with microversion and asking to do via "required=A,B" is better way and saying "required=A,B&required=B" in older microversion is unknown behaviour | 15:25 |
| gmann | or even "required=A,B&required=B" is unknown behavior is any microversion , do via "required=A,B" | 15:26 |
| sean-k-mooney | ok if that is what peopel prefer but i dont like having to opt into correct behavior which is what that forces | 15:27 |
| gmann | It was the same issue for nova query param too, we were ignoring the param and users might not get the expected behavior what they think will get. But we did not fix that without microversion and have to fix in new microversion to avoid breaking any users | 15:32 |
| gmann | why I say it break users is we allowed our API to be used that way and it return something and we do not know what users expect with API to be used that wrong way, expectation might be what our code return now. | 15:33 |
| sean-k-mooney | gmann: yes i understand your argument, i just find it hard to accpet that argument when we no any qurty that does this today is invalid | 15:34 |
| sean-k-mooney | /qurty/query | 15:35 |
| sean-k-mooney | if the perference is to document this which it seams you and efried perfer and only fix going forward as part of the new feature we can do that | 15:35 |
| sean-k-mooney | its what gibi initally did before i raiesed the question in the review | 15:36 |
| gmann | yeah, that is better way IMO. documenting the known bug until this microversion and with new version we fixed it. | 15:37 |
| sean-k-mooney | well technially gibi put in functional tests to assert the current behavior for old microverison to ensure he did not break it | 15:37 |
| sean-k-mooney | gmann: to me this is like intentiolly leaving a cve open but ok | 15:37 |
| sean-k-mooney | we know it wont affect nova | 15:38 |
| sean-k-mooney | sicne we never generate this edge case | 15:38 |
| sean-k-mooney | ironic dpeloyed in standalone mode without nova today i dont think uses placment | 15:39 |
| sean-k-mooney | so im not aware of an consumer of placment that are likely to hit this other then endusers making direct curl requests | 15:39 |
| sean-k-mooney | so the impact really shoudl be minor | 15:39 |
| gmann | I know, and its a bug and we have to accept/live with it for older microversion as this is what our API returned even wrong or right does not matter but in some cases it might return right-as-per-usage | 15:40 |
| sean-k-mooney | gmann: i can say very clearly that if we had a customer that relied on this in any way we would not supprot there continued use of it at the expence of the rest of our customers | 15:41 |
| * sean-k-mooney is slictly cranky that we are being force to do the wrong thing downstream to support once customer that did invalide things currenlty | 15:42 | |
| opendevreview | Imran Hussain proposed openstack/nova master: [nova/libvirt] Support for checking and enabling SMM when needed https://review.opendev.org/c/openstack/nova/+/825496 | 15:42 |
| gmann | yeah, that is what API is. If we allowed to use our API wrongly with some usable results then it is what we allowed and cannot change the API. | 15:48 |
| opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: [yoga] Add PCI VPD Capability Handling https://review.opendev.org/c/openstack/nova/+/808199 | 15:53 |
| opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: [yoga] Include pf mac and vf num in port updates https://review.opendev.org/c/openstack/nova/+/824833 | 15:53 |
| opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: [yoga] Introduce remote_managed tag for PCI devs https://review.opendev.org/c/openstack/nova/+/824834 | 15:53 |
| opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: Bump os-traits to 2.7.0 https://review.opendev.org/c/openstack/nova/+/826675 | 15:53 |
| opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: [yoga] Add support for VNIC_TYPE_SMARTNIC https://review.opendev.org/c/openstack/nova/+/824835 | 15:53 |
| opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: Filter computes without remote-managed ports early https://review.opendev.org/c/openstack/nova/+/812111 | 15:53 |
| dmitriis | sean-k-mooney: submitted the changes in https://review.opendev.org/q/topic:2021-09-10-off-path-net-backends with a dependency on https://review.opendev.org/c/openstack/nova/+/819494/ | 15:56 |
| opendevreview | Imran Hussain proposed openstack/nova master: [nova/libvirt] Support for checking and enabling SMM when needed https://review.opendev.org/c/openstack/nova/+/825496 | 16:00 |
| melwitt | gibi: +1 to fix as bug without new microversion | 16:08 |
| stephenfin | bauzas: Around? I think melwitt left this to you to confirm https://review.opendev.org/c/openstack/nova/+/819366 | 16:19 |
| opendevreview | Merged openstack/nova master: api: Reject duplicate port IDs in server create https://review.opendev.org/c/openstack/nova/+/827070 | 16:24 |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 16:30 |
| * gibi is back | 16:33 | |
| gibi | so there is no clear direction, half of us are up to fix the bug without microversion bump and half of use say fix is with the anyhow incoming 1.39 microversion and document the bug on stable | 16:34 |
| gibi | should I create two separate patch series as see which one land first? or will that simply mean both series will have -1 from the other group? | 16:37 |
| bauzas | stephenfin: I'm surprised I can't see my vote already on the powervm deprecation | 16:38 |
| bauzas | stephenfin: damn shit, I should take a picture | 16:39 |
| bauzas | I literrally found the tab open with my +2 vote and a comment without being submitted | 16:39 |
| * bauzas facepalms | 16:39 | |
| bauzas | melwitt: gibi: stephenfin: deprecated powervm, that's it. | 16:40 |
| gibi | bauzas: +1 | 16:40 |
| bauzas | if anyone knows any FF plugin that puts the tab in red when you're on a gerrit vote without submitting since 1 day, this would be appreciated :D | 16:41 |
| bauzas | but I assume this is a bit a corner case | 16:41 |
| melwitt | I guess there could also be a hybrid option that doesn't raise a 400 for the current microversion but honors all of the valid ones and then in the next microversion start the 400? I dunno | 16:43 |
| gibi | melwitt: do you mean start translating required=A&required=B to required=A,B withoout a microverison bump? | 16:44 |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 16:44 |
| gmann | +1 on that, as long as we do not change the return code for old microversion. translating is also ok | 16:45 |
| gibi | but that also breaks the users who realied on the wrong behavior | 16:46 |
| gibi | so I don't get the rules then | 16:46 |
| melwitt | gibi: not sure (sorry) I don't know the detail of how it works. would doing that raise a 400 with invalid trait? I was thinking that if raising a 400 for invalid trait in current microversion is a problem, could just ignore the invalid and only use the valid | 16:46 |
| gibi | melwitt: currently placement ignores repeated required param and only parse the _last_ one out from the query string | 16:46 |
| gibi | this way anything in an earlier requried param is lost | 16:46 |
| gibi | it can be an invalid trait | 16:47 |
| gibi | or a valid one | 16:47 |
| melwitt | sorry, trying to ask if required=VALID_A,INVAILD_B,VALID_C would raise 400? | 16:47 |
| gibi | that is raising 400 today | 16:47 |
| melwitt | ack | 16:47 |
| gibi | required=INVALID_B,&required=VALID_A is accepted and INVALID_B is ignored | 16:47 |
| melwitt | yeah so I wasn't thinking translation then, if the goal is to avoid making old queries fail that used to pass | 16:47 |
| gibi | making old invalid queries pass is up for debate as you see above | 16:49 |
| melwitt | just to collect all the valid ones and honor them and ignore the invalid ones (current microversion) and then in a new microversion could start rejecting invalid with 400. maybe it's a dumb idea but I'm just trying to help | 16:49 |
| melwitt | ah.. k | 16:49 |
| gibi | melwitt: in a new microversion I proposed to translate required=A&required=B to mean required=A,B and if A is invalid then the query is invalid | 16:50 |
| gibi | the question is should we add 400 before the new microversion | 16:51 |
| gibi | one side says it is an interop issue as API behavior is changing | 16:51 |
| gibi | other side says that we are fixing a bug | 16:51 |
| gibi | and one should not opt into a bugfix | 16:51 |
| gmann | and along with interop, any user using it for valid response of getting 'B' will also be broken even they are using it wrongly | 16:51 |
| gibi | and I think those users should fix their query | 16:52 |
| gibi | as their query is highly misleding | 16:52 |
| gmann | but we allowed our API to be succeed for them | 16:52 |
| gibi | yes, we made a bug | 16:52 |
| gibi | I think it was never intentional to parse required=A&required=B as only required=B and ignore the A filter | 16:53 |
| gibi | but we clearly disagree on this | 16:53 |
| gibi | where we draw the line between API behavior and fixable bug? | 16:55 |
| melwitt | yeah.. so I think the worst part about the bug is ignoring the valid repeated params. and I was thinking to avoid 400'ing users who have an accidental invalid trait in a old repeated passing query, we could fix honoring of the valid traits and leave the invalid to be ignored by the query parsing. then begin the 400 in the next microversion | 16:56 |
| gibi | melwitt: but I think gmann argues that if we start handling required=A&required=B as required=A,B today to accept the valid repeats, then that is also a change in API behavior so requires a microversion | 16:57 |
| gmann | gibi: sorry, may be I am not clear. My concern is to change the 200 to 400 for existing users querying required=A&required=B and ok with having 'B' response. | 17:00 |
| gmann | with new microversion we can fix it in any ways, allow or translate or 400 | 17:00 |
| melwitt | yeah, that would cause a 400 if A or B are invalid. I'm saying do a temporary-for-1.39 thing where you scan all required= and only use the valid traits and silently ignore invalid traits without a 400. with the goal being to apply all valid required traits without introducing a 400 in that case | 17:00 |
| gibi | gmann: so would you be supportive to keep 200 response but filter for both A and B _without_ a microversion bump? | 17:03 |
| gibi | melwitt: in 1.39 (in a bump) I'm intended to do a proper translation including rejecting invalids | 17:03 |
| melwitt | I'm trying to think of other past cases where the behavior was clearly wrong, have we never introduced 400 for that before? | 17:03 |
| stephenfin | bauzas: :D nw, thank you! | 17:03 |
| gmann | gibi: and no change in 1.39 ? | 17:04 |
| gibi | gmann: in 1.39 there a new `in:` prefix is introduced | 17:04 |
| gibi | required=in:A,B means A or B | 17:04 |
| gibi | but we keep required=A,B as A and B | 17:05 |
| gmann | gibi: ok, so that is different new things which is ok. what about required=A&required=B ? return A and B or 400 in >1.39 ? | 17:05 |
| melwitt | gmann: 1.38 is the current microversion | 17:06 |
| gibi | I propose that in 1.39 required=A&required=B is translated to required=A,B | 17:06 |
| gmann | melwitt: I mean gibi new microversion 1.39 | 17:06 |
| melwitt | earlier I wrongly thought 1.39 was current | 17:06 |
| gibi | gmann: but sean suggest to make that 400 instead there too | 17:06 |
| gmann | I too initially until I checked in doc :) | 17:06 |
| gibi | sorry for not being clear about that | 17:07 |
| gmann | gibi: +1 on making 400 there which will be a clear usage instead of supporting multiple way | 17:07 |
| melwitt | I think translation is fine in 1.39 | 17:07 |
| gmann | I mean with mivroversion | 17:07 |
| melwitt | I was thinking what to do for <= 1.38 | 17:07 |
| gmann | melwitt: but then we end up supporting 1. required=A&required=B 2. required=in:A,B 3. required=A,B | 17:08 |
| gibi | to note that is the easiest to support from code perspective :) | 17:08 |
| gibi | to reject 3. I have to add extra logic | 17:08 |
| gmann | IMO, in new microversion we can make first two to avoid confusion | 17:08 |
| melwitt | what's wrong with supporting repeated and non repeated in 1.39? | 17:09 |
| gibi | also note that repeating required=in: is needed to be able to express (A or B) and (C or D) | 17:09 |
| gmann | anyways with new microversion, we can see what all to support. but my only concern is for older microversion we do not change anything what it is today | 17:09 |
| gmann | and document in api-ref that required=A&required=B is unknown behavior until v1.39 | 17:10 |
| gibi | gmann: do we have a description that defines the line between API behavior (that is unfixable in a bugfix) and non API behavior that fixabelk in a bugfix | 17:10 |
| gibi | ? | 17:10 |
| melwitt | even honoring valid traits? I think at the very least it should be changed to honor the valid traits | 17:10 |
| gmann | melwitt: yeah, for invalid I am ok. my concern is on required=VALID_A&required=VALID_B return VALID_B response today | 17:12 |
| gmann | gibi: that is always debatable :). But IMO anything working successfully today even with wrong usage of API is what we should not change. in this case required=A&required=B, user gets 'B' response is successfully case. | 17:13 |
| gibi | gmann: does successfull means http 2xx response? | 17:14 |
| gmann | and we do not know user expectation is only to get B and by mistake they added A too in early query param | 17:14 |
| gmann | gibi: ^^ | 17:14 |
| gmann | this case ^^ not just 200 | 17:14 |
| melwitt | hm, ok. I guess we disagree there, I think that it should be fixed to return VALID_A and VALID_B even for the current microversion | 17:15 |
| gmann | melwitt: I am fine with that but not with returning 400 in this case for current microversion | 17:15 |
| gibi | melwitt: I agree that we disagree :) and I would simply reject repeated params in the current microverison | 17:15 |
| gibi | gmann: I don't get your last point to melwitt | 17:16 |
| gibi | gmann: if we start returning A and B response for required=A&required=B but the user wants to get B only then it is a behavior change | 17:16 |
| melwitt | fwiw I'm not as concerned about the 400, my main concern is honoring the valid traits in the current microversion | 17:16 |
| gibi | as she get B so far but not any more | 17:17 |
| gibi | melwitt: yeah, it seems 3 of us has 3 different area of concern :) | 17:17 |
| gibi | fun :0 | 17:17 |
| gibi | ;) | 17:17 |
| melwitt | yeah 😂 | 17:17 |
| gmann | gibi: well, we at least does break them in term of return code. and say required=A&required=B we meant for return A and B and we fixed it now. | 17:17 |
| gmann | but returning 400 for them break them as they get something and then they get error | 17:18 |
| gibi | gmann: I'm pretty sure if the relied on gettin RPs with B traits only and now getting empty result as we returning onyl RPs with both A and B will break tem | 17:18 |
| gibi | them | 17:18 |
| gmann | I mean we fix the code to make it return A and B is ok but making them error is issue | 17:18 |
| gmann | gibi: we had similar (not exactly same ) issue in nova query param for silently ignoring few/unknown also and we could not change it without microversion | 17:19 |
| gibi | OK, I'm dropping the bugfix from the patchseries of microversion 1.39. I still intend to land 1.39 this cycle. I don't have such mandate for the bugfix itself. so I prioritize | 17:20 |
| gmann | gibi: sure, in that case we can just leave the current behavior as it is and not breaking anything and say "new microversion gives correct behavior" | 17:20 |
| gmann | I am if we want to change for current microversion then we "make it more correct is fine but rejecting the request is not" | 17:21 |
| melwitt | wait, I don't understand why the bug fix can't go in 1.39 if 1.39 is not released yet? | 17:21 |
| gmann | I think gibi saying bug fix for older microversion also | 17:21 |
| gibi | melwitt: the bug will disappera in 1.39 | 17:22 |
| gibi | melwitt: but I will not try to fix it in <1.39 now | 17:22 |
| gibi | as it seem we cannot agree | 17:22 |
| melwitt | ah ok | 17:22 |
| gibi | 1.39 alway planned to allow repeating the required query param | 17:23 |
| gibi | as in: needs to be repeated for (A or B) and (C or D) case | 17:23 |
| melwitt | ok, so repeated was not officially supported < 1.39. sorry I had missed that | 17:24 |
| gmann | yeah, it was always nor documented neither we knew how it work until gibi found it? | 17:24 |
| gibi | melwitt: <1.39 there was not defined behavior for repeat | 17:24 |
| gibi | the code happened to pares the last repetition | 17:25 |
| gibi | parse | 17:25 |
| gibi | and ignore the rest | 17:25 |
| melwitt | gotcha.. thanks | 17:25 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Extra tests around required traits https://review.opendev.org/c/openstack/placement/+/825846 | 17:28 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Refactor trait normalization https://review.opendev.org/c/openstack/placement/+/825847 | 17:28 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Extend the RP db query to support any-traits https://review.opendev.org/c/openstack/placement/+/825848 | 17:28 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: DB layer should only depend on trait id not names https://review.opendev.org/c/openstack/placement/+/826490 | 17:28 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Enhance doc of _get_trees_with_traits https://review.opendev.org/c/openstack/placement/+/825780 | 17:28 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Extend the RP tree DB query to support any-traits https://review.opendev.org/c/openstack/placement/+/825849 | 17:28 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add any-traits support for listing resource providers https://review.opendev.org/c/openstack/placement/+/826491 | 17:28 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add any-traits support for allocation candidates https://review.opendev.org/c/openstack/placement/+/826492 | 17:28 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Remove unused compatibility code https://review.opendev.org/c/openstack/placement/+/826493 | 17:28 |
| opendevreview | Balazs Gibizer proposed openstack/placement master: Add microversion 1.39 to support any-trait queries https://review.opendev.org/c/openstack/placement/+/826719 | 17:28 |
| melwitt | dansmith, bauzas: just a fyi, oslo.limit got a new release so I cleaned up the unified limits set to use it and removed the haxx | 17:29 |
| gibi | sean-k-mooney, gmann, melwitt, efried: I restored the patch series of microversion 1.39 not to change the behavior of the repeated required param handling in < 1.39 microversion https://review.opendev.org/q/topic:any-traits-support | 17:30 |
| gibi | so < 1.39 if the required param is repeated only the _last_ instance is parsed a rest is ignored | 17:30 |
| bauzas | melwitt: cool, as I said, you're my next priority once I'm done with trying to work on Tempest :p | 17:30 |
| melwitt | k :) | 17:31 |
| dansmith | melwitt: nice | 17:34 |
| sean-k-mooney | gibi: ack | 18:07 |
| *** Uggla is now known as Uggla|afk | 18:17 | |
| opendevreview | Rajat Dhasmana proposed openstack/nova master: WIP: Add support for volume backed server rebuild https://review.opendev.org/c/openstack/nova/+/820368 | 18:20 |
| opendevreview | Rajat Dhasmana proposed openstack/python-novaclient master: WIP: Add parameter to rebuild boot volume https://review.opendev.org/c/openstack/python-novaclient/+/827163 | 18:27 |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 18:38 |
| *** tkajinam is now known as Guest1307 | 18:43 | |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 18:45 |
| opendevreview | Merged openstack/nova master: Add nova-ovs-hybrid-plug job https://review.opendev.org/c/openstack/nova/+/817303 | 18:49 |
| opendevreview | Merged openstack/nova master: Deprecate the powervm driver https://review.opendev.org/c/openstack/nova/+/819366 | 19:35 |
| chateaulav | can i get a second look on https://review.opendev.org/c/openstack/nova/+/822053, not sure what happened but it seems to be wanting to merge... i think im stuck and not sure what i broke | 19:42 |
| melwitt | chateaulav: it show it's in merge conflict, should just need a rebase | 19:56 |
| melwitt | *shows | 19:57 |
| chateaulav | melwitt: yeah, tried, i think i got it. had to hard reset the branch. gonna add all the changes back and see if it resolves now | 19:57 |
| chateaulav | thanks | 19:57 |
| melwitt | ack | 20:04 |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 20:43 |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 20:48 |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 20:50 |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 20:53 |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 20:55 |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 21:11 |
| opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | 21:17 |
| *** dasm|rover is now known as dasm|off | 22:29 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!