Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-02-18
15:26:13 gibi mriedem: I'm looking at the heal_allocation code and I'm wondering how to handle when more than on thing is missing
15:26:16 gibi (e.g. project_id and some port allocation) from the instance allocation:
15:26:22 gibi a) do a separate PUT /allocations/{instance.uuid} for each step
15:26:24 kashyap mriedem: When you get a moment, on this: https://specs.openstack.org/openstack/nova-specs/specs/stein/approved/cpu-model-selection.html
15:26:29 gibi b) generate all the missing allocation of the instance and
15:26:44 gibi do a single PUT /allocations/{instance.uuid} at the end
15:26:45 kashyap mriedem: Do you know if Jack Ding / cfriesen et al are working on it?
15:27:00 gibi #a) has the benefit to allow only healing project_id when there is no bandwidth inventory to heal the port allocations
15:27:08 gibi #b) has the benefit of only calling PUT /allocations once
15:28:18 kashyap gibi: Sorry for breaking your chat, didn't realize it for a moment.
15:28:41 gibi kashyap: no worries I think we break each other chat :D
15:28:55 kashyap (I know there is no 'mutex' on IRC, but still :-))
15:28:59 kashyap Matt: I ask because, there is a related blueprint I filed the other day: https://blueprints.launchpad.net/nova/+spec/cpu-selection-with-hypervisor-consideration
15:32:02 kashyap Anyway, it can be worked out; as no one posted any code for it.
15:32:52 mriedem kashyap: no https://blueprints.launchpad.net/nova/+spec/cpu-model-selection
15:32:58 mriedem no patches proposed
15:33:07 kashyap mriedem: Right, I saw that.
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: Add requested_networks to RequestSpec https://review.openstack.org/570201
16:46:59 openstackgerrit Theodoros Tsioutsias proposed openstack/nova master: Enable rebuild for instances in cell0 https://review.openstack.org/570203
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.

Earlier   Later