Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-12-02
14:56:23 sean-k-mooney well does it actully fix the bug
14:56:30 sean-k-mooney or just make it less bad
14:56:58 sean-k-mooney ye are talking about https://bugs.launchpad.net/nova/+bug/1844296 right
14:56:58 openstack Launchpad bug 1844296 in OpenStack Compute (nova) "Stale BDM records remain in the DB after n-api to n-cpu RPC timeouts during reserve_block_device_name" [Undecided,In progress] - Assigned to Lee Yarwood (lyarwood)
14:57:15 sean-k-mooney and this patch https://review.opendev.org/#/c/682594/
14:57:25 lyarwood sean-k-mooney: yeah, it cleans up after one type of failure, it doesn't stop the failure from happening.
14:57:38 sean-k-mooney ah i see
14:57:51 sean-k-mooney so its treating the symtoms not the cause
14:58:11 lyarwood sean-k-mooney: yeah and very specific symptoms at that.
14:58:47 sean-k-mooney you could make it partial-bug if you plan to try and fix it someother way on master in a follow up patch
14:59:19 lyarwood ah yeah I forgot about partial
14:59:21 lyarwood that might do
14:59:37 sean-k-mooney related bug makes sense too but i can see why you might what cloeses bug for stabel if you dont think any complete fix woudl be backportable
15:00:41 sean-k-mooney Partial-Bug i think is the most correct but that wont mark it as "fixed" on stable
15:00:45 mriedem see my comments from PS4
15:00:50 mriedem 1. Can we sort out the various bugs I've mentioned to see which ones this fixes and then duplicate whatever is a duplicate and decide on one (probably the oldest) bug for the same issue.
15:00:55 mriedem 2. I don't trust unit tests for this type of stuff since it involves (a) more than one service and (b) the database. Can we write a functional test to recreate the bug and then assert the fix resolves it?
15:01:26 mriedem i believe i sorted out the bazillion related bugs here https://review.opendev.org/#/c/692940/
15:02:35 lyarwood gah, I'm not sure how I missed that, sorry mriedem
15:03:03 mriedem probably distracted by all of the comments inline about races and locks and such that might mean this doesn't fix a bug
15:04:14 openstackgerrit Dan Smith proposed openstack/nova master: Add a way to exit early from a wait_for_instance_event() https://review.opendev.org/695985
15:10:00 mriedem if you want i could write the functional test if you don't want to bother with that
15:11:40 mriedem lyarwood: since the bdm.create is the last thing that happens on the compute, in what case would this cleanup catch and actually clean anything up?
15:11:55 mriedem like just the split second that we actually created the record and then got a messaging timeout?
15:13:03 lyarwood mriedem: happy to write the func test
15:13:26 lyarwood mriedem: and yeah but that's asssuming that is a split second and n-cpu isn't stuck somehow
15:13:38 lyarwood mriedem: as mdbooth suggested
15:14:02 lyarwood mriedem: FWIW we have seen this downstream at least, so either way it's possible to hit and reasonable to cleanup
15:14:16 mriedem sure, so if n-cpu is stuck on let's say getting the instance uuid lock, or doing something in the driver like _get_device_name_for_instance, we wouldn't create the bdm in that case anyway
15:14:32 mriedem where/how was it triggered downstream?
15:14:48 mriedem slow compute and the api timed out?
15:15:05 mriedem because if so, the change to make reserve_block_device_name using the long_rpc_timeout will fix that
15:15:54 lyarwood mriedem: https://bugzilla.redhat.com/show_bug.cgi?id=1752734 - I didn't get a root cause out of this as the env wasn't logging in DEBUG unfortunatley
15:15:55 openstack bugzilla.redhat.com bug 1752734 in openstack-nova "Invalid bdm record remains when reserve_block_device_name rpc call times out" [High,On_dev] - Assigned to lyarwood
15:16:16 lyarwood mriedem: but we did end up with stale BDM records in the DB after seeing timeouts
15:16:53 sean-k-mooney lyarwood: ok so there is a customer case attached to that so its not a ci issue
15:17:38 mriedem 3. nova-api again requests to nova-compute to do attaching operation 4. nova-compute updates bdm with retrieved connection info 5. if some error like timeout detected between 3-4, remove bdm record created at 2
15:17:43 mriedem so that's not the same thing
15:18:35 mriedem that's (1) reserve_block_device_name worked and created a bdm and then (2) when the api cast to attach_volume something timed out and the bdm was orphaned
15:19:37 lyarwood mriedem: yeah I think that description is wrong, there's reserve_block_device_name failures documented in the next private comment of the bug
15:20:26 lyarwood sean-k-mooney: ^ can confirm that, however I don't think we can share the actual logs here.
15:20:49 mriedem even so, the change to make reserve_block_device_name using the long_rpc_timeout will fix that
15:20:58 mriedem but i don't think that long rpc timeout goes back to queens
15:21:21 sean-k-mooney am i was reading it and im not sure the comment need to be private. we cant share the raw logs but i do not see antynig in comment 1 that is really an issue
15:21:36 mriedem if it's just a messagingtimeout i can understand that
15:21:48 sean-k-mooney MessagingTimeout: Timed out waiting for a reply to message ID fa3d55d2d8c248ba82a13c940350ea9c
15:21:49 lyarwood super secret volume uuids
15:21:52 lyarwood ;)
15:22:10 sean-k-mooney so the long rpc might fix it ya
15:22:14 mriedem it's a 60 second timeout by default with a blocking api - if the compute is slow or we're waiting on the instance uuid lock (maybe because a periodic is doing something with the instance at the time of attach), we could reasonably timeout
15:22:37 mriedem https://review.opendev.org/#/c/566696/ only goes back to rocky though, so you can't get the long_rpc_timeout fix to queens
15:22:43 mriedem so no use in saying that's the backportable fix
15:23:33 mriedem anyway, it just seems like a really small window where the fix in the api would handle it since the bdm create is the last thing to happen - if we timeout before that we still orphan the bdm record
15:23:54 mriedem and i know i'm contradicting my "what's the harm?" replies to mdbooth in the comments :)
15:24:07 mriedem the better longer term fix is the wip i posted, but that's not backportable
15:24:45 lyarwood yeah agreed on that part, we could backport long_rpc_timeout downstream *cough*
15:24:51 lyarwood but yeah
15:25:07 mriedem also your bug is definitely a duplicate of https://bugs.launchpad.net/nova/+bug/1425352
15:25:07 openstack Launchpad bug 1425352 in OpenStack Compute (nova) "A volume remains attached and cannot be detached after attaching it fails" [Low,Confirmed]
15:25:59 sean-k-mooney lyarwood: the issue would be the oslo.messaging min version bump
15:26:24 mriedem sean-k-mooney: correct we're not backporting that to queens-em upstream
15:27:14 lyarwood okay that's likely not an issue downstram
15:27:19 lyarwood downstream*
15:27:26 sean-k-mooney i dont know
15:27:37 sean-k-mooney i would not nessisarliy be comfrotable with that
15:27:46 sean-k-mooney but we could explore it as an option
15:28:07 sean-k-mooney it would depend on what version of oslo.messaging we are actully using downstream
15:28:16 lyarwood >= 5.29.0
15:28:29 lyarwood sean-k-mooney: when did this come in?
15:28:34 sean-k-mooney 6.3.0
15:29:08 lyarwood ah
15:29:11 lyarwood that would be an issue
15:29:17 sean-k-mooney https://github.com/openstack/requirements/blob/stable/queens/upper-constraints.txt#L145
15:29:23 sean-k-mooney so queens was capped a 5.35.6
15:29:31 lyarwood yeah guess what we use downstream
15:29:31 sean-k-mooney so 6.3.0 proably will break things
15:29:39 lyarwood okay that's dream is dead
15:29:42 lyarwood that*
15:30:14 sean-k-mooney is it higher then the upper-constraints for queens? cause i hate when we do that
15:30:42 lyarwood no it's not
15:30:52 sean-k-mooney cool
15:30:57 mriedem you could create your own workaround option as a timeout for this specific call that overrides the default rpc_response_timeout just for your queens-only change and drop it for long_rpc_timeout when you start using that for this call from upstream
15:32:44 sean-k-mooney ya is there an advantage to not useing the same config option
15:33:01 sean-k-mooney is it jsut to make it clear it applies to just this one call
15:37:24 mriedem lyarwood: i left some comments in the patch to try and summarize irc discussion and duplicated the bug to the existing one for the same issue. i'd hold off on writing a functional test until more people weigh in on what the correct direction is here given backports and such.
15:38:02 gibi sean-k-mooney: could you quickly respin https://review.opendev.org/#/c/695118/ to fix stephenfin nits in the conf doc and the release notes? The change looks good to me too but I guess you want to backpor it so a follow up won't help
15:38:58 sean-k-mooney yep i can do that in 5 mins thanks for taking a look
15:39:35 gibi sean-k-mooney: thanks. Ping me and I will +2
15:39:50 mriedem would be nice if any of the blizzard people would ack that change since they reported it
15:45:01 mriedem stephenfin: tbc i'm waiting on vmware ci results on https://review.opendev.org/#/c/696503/ before moving it forward
15:45:33 mriedem if that ci is busted (let's say we don't get results within the next 24 hours or something) then i'm cool with moving forward since if they can't maintain a working ci then we can't maintain their driver for them
15:45:43 stephenfin coolness
15:55:35 sean-k-mooney ... the release notes tox env takes forever to run
15:59:30 openstackgerrit sean mooney proposed openstack/nova master: add [libvirt]/max_queues config option https://review.opendev.org/695118
15:59:45 sean-k-mooney gibi: stephenfin ^ nits addressed no other changes
15:59:53 gibi sean-k-mooney: looking...
16:01:31 gibi eandersson: the bug is coming from blizzard ^^ could you check the bugfix from your perspective?
16:02:26 sean-k-mooney i will be filing and fixing a related bug for vhost-user seperatly but that should cover the case in the current bug if not let me know.
16:08:32 sean-k-mooney actully thinking about that i wonder if i should also add extra code to handel updating this on live migration?
16:11:12 sean-k-mooney i dont think we have code for the queue lenghts on live migration so i kind of feel a seperate patch to handel both queue lenght and max queues would make sense since that will need object changes and wont be backportable where as this simple fix is

Earlier   Later