Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-01-17
09:27:28 bauzas gibi: I have zero context about the original patch
09:27:43 bauzas but I saw we add fixtures for monkeypatching eventlet, right?
09:27:57 gibi right
09:28:11 bauzas as we pull nova.utils now, we also pull fixtures
09:28:14 gibi it is used for the fasteners rw lock
09:28:19 bauzas that's what I saw
09:28:23 gibi but that is only used in test
09:29:22 gibi so the rw lock wrapper can be moved back to the test code tree
09:29:54 gibi or at least that is my working assumption now
09:32:14 tkajinam bauzas, gibi hmmm... sorry do you mind blocking https://review.opendev.org/c/openstack/nova/+/824830 by -2 or -Workflow ?
09:32:52 tkajinam I'll look into that cleaner approach. we need to update requirements in rdo but it doesn't make much sense if we revert it very soon
09:32:57 bauzas tkajinam: no I guess we can move on
09:33:08 bauzas or revert if needed
09:33:24 tkajinam ok
09:33:48 bauzas gibi: do you think you could do something before we merge https://review.opendev.org/c/openstack/nova/+/824830 ?
09:34:05 bauzas I'd personnally let the fix be merged
09:34:17 bauzas I've tagged the bug as critical as it's holding other projects
09:34:47 bauzas and once the gate is back happy, we can sort the things up and revert what we need
09:34:56 bauzas but without any rush
09:35:02 bauzas agreed ?
09:35:12 gibi let land the fix
09:36:16 tkajinam ack. thank you for your time and thoughts, bauzas and gibi !
09:40:25 gibi tkajinam: thank you for proposing the fix
10:31:23 opendevreview Merged openstack/nova master: Close Glance image if downloading failed. https://review.opendev.org/c/openstack/nova/+/815347
11:14:54 opendevreview Merged openstack/nova master: Add fixtures to requirements https://review.opendev.org/c/openstack/nova/+/824830
11:35:05 sean-k-mooney fixtures should be in test-requirements not requirements but otherwise adding it makes sense
11:35:25 sean-k-mooney oh it was moved that is not correct
11:36:55 sean-k-mooney is see how its used https://review.opendev.org/c/openstack/nova/+/824280/1/nova/utils.py and where the dep comes form
11:37:04 sean-k-mooney btu we should not use fixtures for that
11:40:42 sean-k-mooney is there a patch up to remove fixutres form nova.utils. you can do exactly the same with unittest.mock
11:40:52 gibi sean-k-mooney: the RW lock is only used in our test code so we move the RW wrapper in our test path and move back the fixtures deps to test-reqs
11:40:58 gibi I'm about to push a patch for it
11:41:10 sean-k-mooney ok
11:41:29 sean-k-mooney is this a differnt RWlock then the tempest one
11:41:37 sean-k-mooney im not sure why we woudl need it in nova code
11:42:09 sean-k-mooney as in i dont think we need the interprocess one
11:42:10 gibi sean-k-mooney: hehe, it is the same RW lock
11:42:50 gibi hm, no it is not inter process
11:43:04 sean-k-mooney ok just a normal rwlock
11:43:27 gibi yepp
11:44:02 sean-k-mooney ok that makes more sense to me i was like why have we suddenly got multiple processes in our func/unit tests :)
11:44:16 gibi :)
12:35:39 opendevreview Balazs Gibizer proposed openstack/nova master: Move ReaderWriterLock to the test tree https://review.opendev.org/c/openstack/nova/+/824931
12:39:04 gibi sean-k-mooney, bauzas, melwitt: ^^
12:51:00 bauzas gibi: +2s
12:53:48 sean-k-mooney +1 from me also this is a better way to fix the orginal bug
13:50:23 sean-k-mooney nova/healthcheck/manager.py:48:62: N310: timeutils.utcnow() must be used instead of datetime.now()
13:50:40 sean-k-mooney that hacking check shoudl proably be fixed to point out it means timeutils form oslo_utils
13:50:47 sean-k-mooney not the timeutils package
13:57:55 gibi sean-k-mooney: good point, it is probably an easy fix
13:58:38 sean-k-mooney i assume that is commign from hacking based on the N prefix
13:59:09 sean-k-mooney but i have nto looked is that a nova hackign check or a global one
13:59:53 sean-k-mooney nova
13:59:55 sean-k-mooney https://github.com/openstack/nova/blob/master/nova/hacking/checks.py#L209-L221
14:00:04 sean-k-mooney so ya trivial fix
14:00:22 gibi N means Nova :)
14:00:27 gibi afaik
14:00:39 sean-k-mooney H is for genereal hacking
14:00:55 gibi wondering what letter heat and horizon uses :)
14:01:17 sean-k-mooney well assuming they use hacking
14:01:25 sean-k-mooney mybe l for local
14:04:31 gibi heat uses Heat304 for example, clever, it does not need to be a single letter
14:04:59 gibi horizon uses M
14:07:43 gibi sean-k-mooney, bauzas: btw, can I get eyes on this bugfix https://review.opendev.org/q/topic:bug/1952941 I'm still on the hook to backport this to victoria due to downstream upgrade failures.
14:09:07 sean-k-mooney ah that one sure
14:15:24 gibi thanks
14:16:18 sean-k-mooney im +1 on that and the repoducer below it, thanks for following that pattern it makes it supper clear that the fix is correct
14:17:26 sean-k-mooney you have a seperate change too to stop persiting the numa toloplogy in the request spec correct?
14:17:56 sean-k-mooney https://review.opendev.org/c/openstack/nova/+/820215
14:18:45 sean-k-mooney after the current backportable change is done i woudl still be happy to proceed with that too
14:20:10 gibi sean-k-mooney: yeah, I'm happy to move forward with https://review.opendev.org/c/openstack/nova/+/820215 if there is a consensus
14:20:26 gibi the current state of that patch already show that the idea is feasible
14:21:25 sean-k-mooney ya its not required but since it will prevent this type of error in the future its proably worth it in the long run
15:01:12 bauzas gibi: ack, sorry was on meeting
15:01:21 bauzas gibi: will look at it later today
15:01:39 gibi bauzas: thanks, no worries
15:02:35 bauzas gibi: gmann: btw. if we need to update a microversion for unshelve (adding a new host), are we OK if we could remove https://github.com/openstack/nova/blob/1ddb8f83adef964a8ca050994a43adc6175994f1/nova/api/openstack/compute/schemas/shelve.py#L31 ?
15:03:07 bauzas availability_zone is Optional in the doc :)
15:18:51 artom Yeah, it's a weird one
15:19:00 gibi bauzas: looking
15:19:01 artom AZ isn't actually optional, the entire request body is
15:19:04 sean-k-mooney well the az is optional
15:19:24 artom If you have a body, AZ needs to be *something*, could be None
15:19:35 bauzas you need to either provide no dict or if you provide the dict, you need to add the AZ key
15:19:46 artom ... which could be None :P
15:20:00 bauzas so, when adding a new argument like "host", we would also need to add AZ
15:20:10 sean-k-mooney yep but there is no other property you can currently pass in the dict
15:20:32 bauzas yeah, that's why I'm asking to remove this for the new API microversion also adding the new argument
15:20:41 bauzas sean-k-mooney: correct, for the moment
15:20:43 sean-k-mooney artom it would have to be the litral value null potentailly but this is not the only place where its slightly odd
15:20:55 gibi I think both unshelve to an AZ or unshelve to a specific host make sense
15:21:16 bauzas sean-k-mooney: but once Uggla will add a new key like "host", that would mean that the AZ key should be required
15:21:29 sean-k-mooney yep likely you dont need to sepcify both but if you did we shoudl vlaidated the host to az relationship
15:21:43 artom No...? Having to specify both host and AZ seems weird
15:21:51 artom I'd say one or the other, but not both?
15:21:53 bauzas that's why I'm saying we should remove the requiring when we modify this API
15:22:11 bauzas in a new microversion of course
15:22:15 sean-k-mooney bauzas: well as currently specified but in generally unshele:{"host":"my-host"}
15:22:20 sean-k-mooney i think shoudl be valid
15:22:21 artom To my mind what would be clearest is: either empty body, or AZ with a non-null value, or host with a non-null value

Earlier   Later