Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-11-05
08:59:11 bauzas oh shit, I forgot to add the conditional I promised to dansmith ^_^
09:06:36 bauzas actually, we don't need it \o/
09:26:31 gibi bauzas: I'm confused about the naming here https://review.opendev.org/#/c/761457/2/nova/tests/functional/regressions/test_bug_1902925.py@31
09:27:03 bauzas that's what happens when you copy/paste some methods...
09:35:37 gibi bauzas: when you respin it, could you update the doc here too https://review.opendev.org/#/c/761458/2/nova/compute/manager.py@3355
09:37:14 gibi besides these, the fix looks good to me
09:44:17 gibi lyarwood, elod: when the bugfix ^^ is merged to victoria we need to push a point release
09:44:31 gibi as this is a critical upgrade issue to V
09:46:11 gibi bauzas: btw, one more request, could you add an upgrade reno to the fix? It would help making visible that upgrading to V needs this fix
09:47:28 bauzas gibi: sure for both
09:47:34 gibi thanks
09:47:40 bauzas I was just about to upload but I killed it
09:51:49 lyarwood gibi: ack
10:02:48 lyarwood so are we not testing rebuild in grenade?
10:02:53 openstackgerrit Sylvain Bauza proposed openstack/nova master: Add a regression test for 5.12 compute API issue https://review.opendev.org/761457
10:02:53 openstackgerrit Sylvain Bauza proposed openstack/nova master: Fix the compute RPC 5.12 issue https://review.opendev.org/761458
10:03:09 bauzas gibi: done ^
10:03:36 elod gibi: thx, I've planned to propose release patches for stein + train + ussuri + victoria today, but then I'll wait with the victoria release patch :]
10:03:51 elod lyarwood: fyi ^^^
10:03:51 bauzas elod: hopefully, we'll merge it today
10:04:03 bauzas the fix is simple
10:04:08 elod bauzas: \o/
10:04:31 elod then we just have to wait the gate :]
10:04:31 lyarwood elod: ack thanks
10:04:47 lyarwood I guess we don't test rebuilds in a mixed upgrade state
10:05:13 lyarwood and that's why grenade multinode didn't hit this
10:05:33 gibi bauzas: looking
10:06:20 bauzas lyarwood: stephenfin: gibi: I'm not telling you were bad about reviewing (i also sometimes misses some issues), but maybe it would be nice for you to look at both the fix https://review.opendev.org/#/c/761458/ but also to review https://review.opendev.org/#/c/761452/ to understand how RPC API works
10:06:46 bauzas again, no worries at all
10:07:24 bauzas it's more for providing a knowledge help for you folks about how RPC versions work
10:07:34 stephenfin ah, I knew that and forgot about it :(
10:07:45 bauzas if you knew it, all good then
10:08:16 gibi bauzas: yeah, thanks for the pointers. I'm wondering if we can make some test enhancements to catch these in the future
10:08:17 lyarwood I didn't even review the broken patch here so I'm not sure what you're trying to say
10:08:21 lyarwood ....
10:08:27 bauzas but hopefully the proxy change I'm providing is nice for knowing how to have a major version
10:09:14 bauzas lyarwood: not about any previous reviews, just for helping you to know what to review when you have a change with a RPC modification
10:09:56 lyarwood sure
10:11:14 lyarwood bauzas: look forward to your reference docs patches
10:14:27 stephenfin gibi: We could probably hash the signature or something?
10:14:41 bauzas actually, I could write something in https://docs.openstack.org/nova/latest/contributor/code-review.html
10:14:55 bauzas stephenfin: gibi: testing it is not simple
10:14:55 stephenfin i.e. identify all the position, non-optional arguments and generate/save a hash for those
10:15:09 stephenfin then compare each time, like we do for o.vos
10:15:14 bauzas since the arguments are different between RPC versions
10:15:14 gibi stephenfin: that would be the ovo way yes
10:15:26 stephenfin *positional
10:15:38 lyarwood bauzas: https://docs.openstack.org/nova/latest/reference/rpc.html I was thinking more in here
10:15:44 lyarwood bauzas: but either way
10:15:52 bauzas stephenfin: we have non-positional arguments that are unrelated to RPC versions
10:16:10 bauzas we just keep them optional
10:16:49 bauzas lyarwood: oh, TIL this page was existing
10:17:02 bauzas gibi: we fixed it by code reviews
10:17:49 bauzas ah, this is already documented https://docs.openstack.org/nova/latest/contributor/code-review.html#rpc-api-versions
10:17:53 gibi bauzas: sure, code review is the fallaback, human intelligence is king, but if we can automate it then we could avoid failing humans like me at the original code rview
10:18:12 lyarwood bauzas: ah cool
10:18:20 bauzas but I guess "The manager-side method needs to tolerate older calls as well as newer calls" is maybe too much overall, and we need to explain it more
10:18:47 bauzas gibi: we could enforce owners to propose functional tests
10:18:53 bauzas for testing the RPC pins
10:19:06 bauzas like I did in my regression test
10:19:29 bauzas this would be a simpliest approach
10:19:52 gibi I guess enforce by code review
10:20:01 bauzas that, yeah
10:20:06 gibi I agree
10:20:13 bauzas but from what I've seen, nobody is really doing it
10:20:16 gibi still I want to automate it if possible :D
10:20:39 lyarwood shouldn't we cover mixed compute upgrades in the multinode grenade job?
10:20:46 bauzas gibi: well, we don't really set new versions a lot right?
10:20:50 gibi becuase all are code review rules are as good as the way we enforce them
10:21:00 gibi bauzas: we do it less and less, I agree
10:21:01 bauzas like, we only had one rpc minor bump per release since a while
10:21:16 bauzas gibi: well, we have a code review documentation
10:21:24 bauzas and I expect cores to know it at least
10:21:39 bauzas I mean, that's a breaking change to accept a RPC change
10:22:43 bauzas maybe we rushed over accepting some feature that was long overdue, but maybe considering to require a functest would ensure that we would put the burden on code owners
10:22:45 gibi bauzas: you are correct that we assume that core reviews catch these kind of problems, but they don't as you found
10:23:16 gibi if we do these thing less and less it means that we will easier to forget what to look at in these changes
10:24:07 gibi as we don't excersize this knowledge
10:26:26 gibi so I agree that one thing is to raise awerness for this issue as you did.
10:27:16 gibi but also I will think about some kind of automation as I cannot promise I won't forget this rule again 6 months from now when we bump the next
10:44:06 bauzas gibi: ahah lol, i had to rush off home because I forgot my kids at the school :whoops:
10:44:38 bauzas gibi: fwiw, the change we merged was a bit hairy, so I do understand that it was difficult to find the problem
10:45:02 bauzas gibi: that's why I said we should at least ask to provide a functional test, that's it
10:45:41 gibi yeah, I should not forget to ask a functional test pining to old RPC version when a new RPC version is proposed
12:05:33 brinzhang0 gibi: hi good morning
12:05:59 brinzhang0 gibi: Hope you can review
12:06:00 brinzhang0 Cyborg shelve/unshelve support patch https://review.opendev.org/#/c/729563/ :D
12:06:33 gibi brinzhang0: add to my queue
12:06:42 gibi added
12:06:51 brinzhang0 gibi: thanks
13:36:01 bauzas brinzhang0: gibi: hah, this time the new argument is nullable :p
13:36:23 bauzas but maybe it's time to ask for a functional testclass verifying the RPC API ? :)
14:11:28 openstackgerrit Balazs Gibizer proposed openstack/nova master: Bump the lowest eventlet version to 0.26.1 https://review.opendev.org/761427
15:12:05 iurygregory Hi nova folks, a friend of mine using openstack queens asked me if it's possible to update the config drive of an instance?
15:12:31 bauzas gibi: the next nova meeting is in 45 mins, right?
15:12:36 bauzas tz change
15:14:35 gibi bauzas: yes, the meeting is at 16:00 UTC which is 17:00 CET
15:14:43 bauzas cool cool
15:14:48 bauzas nicer for us :)
15:15:23 gibi :)

Earlier   Later