Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-06-13
15:37:41 mriedem i see that id and host are special cased
15:37:50 mriedem i was just doing this:
15:37:51 mriedem return key in self._obj
15:37:51 mriedem def __contains__(self, key):
15:37:56 mriedem but do i need to special case id and host?
15:38:11 mriedem i don't think so, but wanted to ask
15:38:37 bauzas mriedem: I need to reload the context in mind because it's a 2-yr blame :p
15:39:11 mriedem i think we special case id and host because they are namespaced
15:39:12 mriedem to the cell
15:39:28 mriedem whether they are IN the proxied object is the same either way i think
15:41:00 bauzas mriedem: oh right I remember
15:41:01 bauzas mriedem: now
15:41:30 bauzas AFAIR, we're amending the host value by the cell path
15:41:43 bauzas the same goes with the ID
15:41:55 bauzas something like 'foo' becomes cell1!foo AFAIR
15:42:13 bauzas which was a problem for our object layers
15:42:20 bauzas at least the id
15:42:48 bauzas so, we wrote those ugly patterns to make sure we can just modify both the ID and the host fields without touching the object
15:43:48 mriedem yeah, so i think the answer is my simple __contains__ impl is fine
15:43:50 mriedem for what i need
15:43:57 mriedem which is just checking if a field is set in the wrapped object
15:44:08 mriedem it's a proxy to the ovo base __contains__
15:44:26 bauzas I'm thinking of your question
15:44:33 bauzas whether we should special case id and host
15:44:44 bauzas I think no
15:44:50 bauzas because we have the properties
15:46:59 bauzas mriedem: ^
15:49:31 bauzas mriedem: the thing is, I'm thiking of the precedence between __getitem__ and __contains__
15:50:33 bauzas mriedem: but if you ask : 'id' in mycellproxy
15:50:39 mriedem bauzas: __contains__ is used before __getitem__
15:50:43 bauzas it should provide True anyway
15:50:45 mriedem which is why i needed to add it
15:50:59 mriedem see http://logs.openstack.org/99/471899/5/check/gate-nova-python27-ubuntu-xenial/7be94a3/testr_results.html.gz
15:51:12 mriedem that was calling __getitem__ with key=0
15:51:17 bauzas FWIW, the doc is https://docs.python.org/3/reference/datamodel.html
15:51:21 mriedem treating it as a list rather than a map (dict)
15:51:37 mriedem and getattr blows up with TypeError if you're passing a non-string
15:51:41 mriedem __contains__ fixes it
15:53:05 bauzas mriedem: not sure I get why you land with key=0 but okay
15:53:37 mriedem me neither
15:53:47 mriedem anyway, i think it's resolved,
15:53:51 mriedem forget i ever asked about this
15:54:16 bauzas well, okay :)
15:55:22 sfinucan sdague: Could you take a look at this and see if it makes sense? https://review.openstack.org/#/c/473224/
15:55:54 sdague sfinucan: with the version bump it should be fine
15:56:31 sfinucan sdague: so they should go in together?
15:57:00 sfinucan *in the same patch
15:57:06 sdague yeh
15:57:24 sdague I think there is going to need to be a unit test change at the same time
15:57:31 sdague I can pull them together if you like
15:57:45 cfriesen mriedem: andreykurilin: Still hitting the problem. http://paste.openstack.org/show/612439/ has my setUp() code, the test itself, and the pdb output while running the test.
15:58:45 cfriesen mriedem: andreykurilin: for some reason I can't do pdb in novaclient/v2/shell.py, it just hangs. wonder if that's related to the issue
15:59:31 andreykurilin it is strange, because I fetched your patch, applied changes in setUp method and everything works for me(at least test_default_endpoint_type passes)
15:59:55 cfriesen andreykurilin:did you change it from "flavor-list" to "list"?
16:00:12 andreykurilin ok, let me try
16:00:18 cfriesen in test_default_endpoint_type9)
16:02:10 mriedem alex_xu: sdague: remember how we wanted to do the os-services and os-hypervisors id->uuid API changes in the same microversion, plus a bunch of other cleanups for weird PUT and GET APIs?
16:02:26 mriedem this one https://specs.openstack.org/openstack/nova-specs/specs/pike/approved/service-hyper-uuid-in-api.html
16:02:28 openstackgerrit Sean Dague proposed openstack/python-novaclient master: Fix setting 'global_request_id' in SessionClient https://review.openstack.org/473224
16:03:03 mriedem turns out that might be a monster change if we're doing it on the same microversion, like the os-services one is pushing 1000 LOC
16:03:10 andreykurilin cfriesen: `self.mock_client.return_value.api_version = api_versions.APIVersion("2.1")`
16:03:16 andreykurilin you need to setup api_version like this
16:04:07 andreykurilin cfriesen: because self.mock_client is just a mock of novaclient.client.Client method and CLI obtains api_version property from the object which that novaclient.client.Client returns
16:04:22 sdague mriedem: because of the tests and friends?
16:04:31 mriedem sdague: and docs yeah
16:04:32 mriedem plus,
16:04:38 mriedem os-hypervisors is going to require changes for paging across cells,
16:04:43 mriedem which i didn't originally anticipate in the spec
16:04:46 sdague ok
16:05:02 sdague is there any way to structure that code for land better?
16:05:29 mriedem i've tried to split out some of it by layers, e.g. https://review.openstack.org/#/c/461532/
16:05:37 mriedem is the compute API stuff, before the REST API controller pieces
16:05:54 mriedem can't really do that once i get to https://review.openstack.org/#/c/464280/ though
16:05:58 sfinucan sdague: Yeah, go for it
16:05:59 mriedem granted that one is mostly tests and docs
16:06:14 sfinucan (pulling them together)
16:06:17 sdague honestly, I have wondered if we should stack our API changes a little differently so the functional is in one patch, samples tests in next patch, api-ref in 3rd? Then just -2 the bottom and do a flush
16:06:37 sdague because I do agree they get complicated to mentally context switch in the reviews
16:06:38 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Handle uuids in os-hypervisors API https://review.openstack.org/471899
16:06:39 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Use uuid for id in Services API https://review.openstack.org/464280
16:06:48 mriedem ^ is the latest before i have the api-ref working
16:06:54 mriedem per my os_api_ref problem this morning
16:07:12 mriedem my plan here was to get reviews on each separately, -2 on os-services until +2 on os-hypervisors, then squash them
16:07:37 mriedem there is going to be db api/object layer paging changes for the hypervisors one that will be separate
16:07:40 mriedem but things still get pretty big
16:07:46 cfriesen andreykurilin: you're my hero. :)
16:07:59 mriedem https://review.openstack.org/#/c/464280/11/nova/api/openstack/compute/services.py is the main functional piece in the os-services change, and isn't too bad
16:08:18 mriedem i chose not to try and re-using the weird little private methods for the service action apis
16:08:22 mriedem like _enable and _disable
16:08:36 mriedem so it's a single update() method for the new microversion that handles all of the things in the request
16:12:08 openstackgerrit Sean Dague proposed openstack/python-novaclient master: Fix setting 'global_request_id' in SessionClient https://review.openstack.org/473224
16:12:16 sdague sfinucan: it can actualy be a little smaller
16:13:30 sfinucan sdague: And even smaller? REQ_ID_HEADER looks unused
16:14:41 sdague sfinucan: oh, it might be now
16:14:49 bauzas mriedem: I did remember how cutting the relationship between services and hypervisors was a big thing, so I trust you when you say it's becoming a monster change
16:14:52 sdague I can swing around in a sec
16:15:08 sfinucan no rush
16:15:35 bauzas touching services and hypervisor seems like the most impactful things to do, because we have a shit ton of tests asserting things, plus a lot of logic around those
16:15:46 openstackgerrit Sean Dague proposed openstack/python-novaclient master: Fix setting 'global_request_id' in SessionClient https://review.openstack.org/473224
16:15:50 mriedem it's definitely not fun

Earlier   Later