Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-02-18
15:33:34 kashyap But most important thing is to rip out existing two CPU-related APIs and use the more sensible APIs that take into account the hypervisor capabilities.
15:33:57 mriedem gibi: i'm assuming (a) is less change in the heal_allocations code
15:34:26 mriedem gibi: for healing missing bw allocations i would hope most of that can be a separate private method that does the check and calculates the allocations to PUT
15:34:52 gibi mriedem: (a) seems a bit less intrusive to the existing code yes
15:35:10 gibi mriedem: yes, I will definitly separate out the bw healing to methos
15:35:13 gibi methods
15:35:37 mriedem gibi: so we already do at most 2 PUTs per instance right?
15:35:40 mriedem first here https://github.com/openstack/nova/blob/master/nova/cmd/manage.py#L1892
15:35:49 mriedem second here https://github.com/openstack/nova/blob/master/nova/cmd/manage.py#L1912
15:35:57 mriedem the second is getting allocations from the flavor
15:36:25 mriedem gibi: so i think for bw allocations, you add another private method and merge the results of scheduler_utils.resources_from_flavor before calling the 2nd PUT
15:37:08 gibi mriedem: if the code needs to create allocation then it is a single PUT
15:37:29 gibi mriedem: also if there is allocation but no project_id then it is also a single PUT
15:37:48 gibi mriedem: there is a return here https://github.com/openstack/nova/blob/e3c24da89aa3e6462f1b07e00659c87f252ba4ba/nova/cmd/manage.py#L1898
15:38:34 mriedem yeah that complicates things...
15:39:09 gibi mriedem: so adding bw to both places means calling my new bw calculation function in both places
15:39:42 mriedem or, get bw allocations first, then if allocations exist but don't have bw resources, skip that return statement and merge for the 2nd PUT
15:40:15 mriedem well,
15:40:32 mriedem i guess you don't want to have to do the work of calculating the bw allocations if you can determine the existing allocations have the bw resource classes
15:41:14 gibi mriedem: I have to check each neutron port attached to the instance to see if there is resoruce_request in the port but no rp uuid in the binding
15:41:37 mriedem yeah i guess the lack of bw in the existing allocatoins does'nt tell us if you need to heal them
15:41:41 mriedem because not all instances will have bw allocations
15:41:51 gibi mriedem: yes
15:42:05 mriedem so it sounds like you need to do that calculation up front anyway
15:42:40 gibi mriedem: yeah, I can do the check and the calculation upfront and merge it to whatwhever PUT it is needed
15:43:05 mriedem we may want an option on the command itself to skip that check
15:43:06 gibi and introduce a new PUT branch to update the bw only, if other data doesn't triggered the PUT
15:43:12 mriedem --skip-bw-allocations
15:43:18 gibi mriedem: ack
15:43:39 mriedem because if my cloud doesn't enable any of that via policy, i don't want this command to waste time spamming the neutron API to figure out that information
15:44:01 mriedem note that this is another argument for stashing the port resource request information in the info_cache :)
15:44:27 mriedem if it was in the info cache we wouldn't need to hit the neutron api for each instance here
15:44:39 gibi mriedem: that cache could be stale when you turn on the policy :)
15:44:57 mriedem that's what we have _heal_instance_info_cache in the compute for
15:45:05 mriedem but sure, you could miss it
15:45:23 mriedem you'd likely know if it's not cached based on some key missing from the cache
15:45:36 gibi mriedem: I'm happy to add a flag to the CLI instead of a cache
15:45:46 mriedem i.e. if 'resource_request' not in VIF - stale cache, vs vif['resource_request'] == None means not stale, but no resource request
15:46:02 mriedem yeah i know :)
15:46:03 mriedem just saying
15:46:44 gibi I hear you. and I still afaraid of cache for this
15:48:38 gibi anyhow I will try to precreate the bw if needed and merge it in to the existing PUTs
16:24:05 openstackgerrit Matt Riedemann proposed openstack/nova master: Add microversion to expose virtual device tags https://review.openstack.org/631948
16:27:21 openstackgerrit Kashyap Chamarthy proposed openstack/nova master: libvirt: Omit needless check on 'CONF.serial_console' https://review.openstack.org/637578
16:44:01 openstackgerrit Matt Riedemann proposed openstack/python-novaclient master: Add support for microversion 2.70 - expose device tags https://review.openstack.org/636779
16:46:59 openstackgerrit Theodoros Tsioutsias proposed openstack/nova master: Enable rebuild for instances in cell0 https://review.openstack.org/570203
16:46:59 openstackgerrit Theodoros Tsioutsias proposed openstack/nova master: Add requested_networks to RequestSpec https://review.openstack.org/570201
16:47:41 openstackgerrit Theodoros Tsioutsias proposed openstack/nova master: Introduce the PENDING instance state https://review.openstack.org/566473
16:47:42 openstackgerrit Theodoros Tsioutsias proposed openstack/nova master: Allow rebuild for instances in PENDING state https://review.openstack.org/637585
16:49:53 zzzeek jaypipes: https://stackoverflow.com/questions/54751671/can-two-innodb-update-statements-against-a-pk-index-deadlock-if-they-are-given-a
16:54:20 jaypipes zzzeek: that's what we include the `WHERE id = ? AND version_id = <our expected version id>` in the WHERE statement.
16:54:27 jaypipes *why* we include... sorry
16:54:45 zzzeek jaypipes: yes, that's my versioning code so I know that part :)
16:55:04 zzzeek jaypipes: in neutron I'm helping them with a case where they want to bump version ids without checking
16:55:23 zzzeek jaypipes: and the approach of just emitting the UPDATE agaisnt PK with the versino_id=version_id+1 works.
16:55:45 zzzeek jaypipes: I think that it will *never* fail, however even if it did produce a deadlock sometimes, it would be a lot less failure than it has now
16:56:03 jaypipes zzzeek: they want to bump version codes without including the "AND version_id = <expected>" in the WHERE statement you mean?
16:56:14 zzzeek jaypipes: yes, they want the number to increment and that's it
16:56:21 zzzeek they dont care what it was previosuly
16:56:25 jaypipes yeah, that's a recipe for disaster.
16:56:28 zzzeek jaypipes: why
16:56:43 jaypipes because it will produce excessive deadlocks, as you are showing here.
16:57:25 zzzeek jaypipes: if you UPDATE against the PKs in the same order each time, it will not. also, this code previously used the "where verison_Id=<>" thing and it failed lots under stress test. now it does not fail at all
16:57:51 zzzeek jaypipes: so, my assertion is, even if it does deadlock, MySQLs deadlocks are immediate and there should be a lot less of them compared to failed compare-and-swaps right now
16:58:16 zzzeek jaypipes: obviosuly for galera multi master everything goes out the window :)
16:58:46 jaypipes not really.
16:58:50 zzzeek jaypipes: but this is mostly a question of, when MySQL emits UPDATE and we dont use ORDER BY (which is goofy), is it going to scan the rows from the index in the same way each time
16:59:37 jaypipes zzzeek: are you asking if innodb is going to lock record 1, *then* record 2, regardless of if the WHERE statement says IN(1, 2) or IN(2, 1)?
16:59:45 zzzeek jaypipes: yes
16:59:49 jaypipes neither.
17:00:17 zzzeek jaypipes: not exactly , the Q is, will it lock 1 and 2 in the *same* order as another UPDATE statement that refers to the same keys
17:00:29 zzzeek i dont care what the order is, just that it is deterministic
17:00:39 jaypipes there will always be a single lock (a gap lock of (inclusive_low=1, inclusive_high=2)
17:00:53 jaypipes it's not 2 locks.
17:01:10 jaypipes it's one lock, evaluated atomically.
17:01:20 zzzeek jaypipes: OK then what does https://dev.mysql.com/doc/refman/8.0/en/innodb-locking.html#innodb-gap-locks say: "Gap locking is not needed for statements that lock rows using a unique index to search for a unique row. "
17:01:31 zzzeek jaypipes: I tried this without using the PK, and it gap locks
17:01:36 zzzeek jaypipes: add the PK, and it row locks
17:02:48 zzzeek b.c. yes w a gap lock, its crazytown time if you have lots of rows
17:05:59 jaypipes zzzeek: can we back up a sec? I'm a little lost on what you're trying to do in Neutron land. could you link me to the code you are doing/reviewing?
17:06:37 zzzeek jaypipes: sure it's https://review.openstack.org/#/c/635671/ didnt want to pull you into neutron
17:08:14 zzzeek jaypipes: they were ready to add a whole new table and rewrite their whole versioning thing just so they can have incrementing version ids without checking for conflicts
17:08:43 zzzeek jaypipes: as always, if this fails sometimes, it retries. it's not the end of the world. the current implementation fails and retries lots.
17:13:41 jaypipes zzzeek: retries are natural and expected, yes
17:13:52 jaypipes zzzeek: lemme read this patch first :)
17:14:01 zzzeek jaypipes: im going to try the sleep() trick to see if i can get the two updates to interleave
17:22:30 openstackgerrit Jim Rollenhagen proposed openstack/nova master: ironic: check fresh data when sync_power_state doesn't line up https://review.openstack.org/636699
17:28:29 openstackgerrit Maciej Kucia proposed openstack/nova master: SR-IOV passthrough: Check PF only if VF is enabled https://review.openstack.org/476642
17:28:50 openstackgerrit Merged openstack/nova master: create_veth_pair is unused, remove it. https://review.openstack.org/624226
17:32:43 openstackgerrit Elod Illes proposed openstack/nova stable/queens: Make service all-cells min version helper use scatter-gather https://review.openstack.org/637594
17:59:49 jaypipes zzzeek: hmm, neutron patch is a rabbithole for me... abstraction on top of abstraction. I'm afraid that I'm having a difficult time following what SQL statements would/will be produced at which events with this...
18:00:13 zzzeek jaypipes: I know, neutron is a PITA
18:00:26 zzzeek jaypipes: i never work on it because it is nonsensical
18:00:35 zzzeek jaypipes: but i figured out enough what this does :)
18:00:37 openstackgerrit Elod Illes proposed openstack/nova stable/queens: Make host_manager use scatter-gather and ignore down cells https://review.openstack.org/637599
18:02:46 jaypipes zzzeek: this patch definitely fixes a number of deadlocks you are seeing, yes?
18:03:14 jaypipes zzzeek: where specifically are those deadlocks being hit? (in the neutron code, that is...)
18:04:06 zzzeek jaypipes: it fixes SQLAlchemy version id misses, not "deadlocks" in the literal sense
18:04:16 zzzeek jaypipes: SQLAlchemy StaleDataErrors
18:04:55 zzzeek jaypipes: they have this test case where they create 300 security group rules

Earlier   Later