| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-04-14 | |||
| 20:32:47 | melwitt | bauzas: oh, I see. I don't know of a way around that. you would probably need the ghost of mriedem. or maybe gibi might have ideas | |
| 20:33:12 | bauzas | melwitt: so, for example, if I create an instance for host1, resize it to host2 and then confirm the resize, it won't remove the guest in host1 because instance.host (host2) == CONF.host (host2) even I checked that it was calling host1 | |
| 20:33:38 | bauzas | melwitt: no worries, I'll just then modify the CONF before calling confirm_resize | |
| 20:36:04 | mriedem | isn't there another handle within the libvirt driver to the host value used when the service was started? | |
| 20:36:06 | mriedem | the compute service i mean | |
| 20:36:16 | mriedem | if so, you can change that code to use that and not deal with global config problems | |
| 20:38:11 | mriedem | the compute driver has a handle to the virtapi which has a handle to the compute manager which has a self.host value | |
| 20:39:09 | bauzas | mriedem: yeah, no worries, I can provide a change for it | |
| 20:39:25 | mriedem | so i think it's just `self.virtapi._compute.host` | |
| 20:39:40 | melwitt | oh yeah, self.host, that will be different even in func tests. good call | |
| 20:40:06 | mriedem | you could add a "host" property getter method to the virtapi so that the driver doesn't need to know about self._compute | |
| 20:40:23 | mriedem | self.virtapi.host | |
| 20:40:25 | mriedem | badabing | |
| 20:41:09 | bauzas | thanks | |
| 20:41:28 | bauzas | fwiw, we have a lot of CONF.host values in libvirt | |
| 20:41:54 | bauzas | so I'll just create a change for all of them | |
| 20:45:03 | artom | bauzas, yeah, there's not much you can do with the global CONF.host... | |
| 20:45:19 | artom | Although the computes should keep their hostnames... | |
| 20:45:24 | artom | So maybe there's a bug somewhere? | |
| 20:45:48 | bauzas | artom: that's not really a bug | |
| 20:45:51 | artom | Oh, wait, melwitt linked the "wrong" helper | |
| 20:45:55 | bauzas | artom: it's just a problem for tests | |
| 20:46:15 | artom | bauzas, are you using the fake driver, or the libvirt driver? | |
| 20:46:22 | bauzas | the latter | |
| 20:46:31 | bauzas | hence the problem | |
| 20:46:58 | bauzas | anyway, I found the solution | |
| 20:47:00 | melwitt | artom: what do you mean "wrong" helper? | |
| 20:47:12 | artom | bauzas, https://github.com/openstack/nova/blob/9001d7e3459d97f507e8ce638d1fc3935401252d/nova/tests/functional/libvirt/base.py#L117 | |
| 20:47:32 | bauzas | I all know about it | |
| 20:47:58 | bauzas | but again, CONF.host will be overrided by the last value | |
| 20:48:16 | bauzas | given it's a global variable | |
| 20:48:21 | artom | bauzas, no way around it, then | |
| 20:48:26 | artom | CONF is global per process | |
| 20:48:37 | bauzas | we only run by a single process fwiw | |
| 20:48:43 | artom | Yep | |
| 20:48:44 | bauzas | (for testing) | |
| 20:48:59 | artom | OK, my kids are impatiently calling me to literally go fly a kite | |
| 20:49:04 | artom | So I'm off again | |
| 20:49:06 | mriedem | if the driver code uses the host set on the compute manager (via virtapi) then you get whatever host name was used when the service was started | |
| 20:49:12 | mriedem | so, ween off global conf | |
| 20:49:16 | bauzas | anyway, I got the workaround (changing the value before calling the post api), and I'll create a new change for libvirt | |
| 20:49:54 | bauzas | I mean, I'll propose the functional test first, and then another change for just using virtapi in libvirt | |
| 20:49:58 | sean-k-mooney | i think you could mock it to have different values in different greenthread/coroutiens but ya the currnet constution makes having different values chalanging | |
| 20:50:02 | bauzas | done. | |
| 20:50:48 | bauzas | sean-k-mooney: oh yeah of course, we could start the services by each greenthread... and then we would see problems :p | |
| 20:50:56 | bauzas | or I dunno | |
| 20:51:02 | bauzas | anyway | |
| 20:51:06 | bauzas | I'm done | |
| 21:51:13 | sean-k-mooney | bauzas: mriedem melwitt this is how we can mock the globals i think http://paste.openstack.org/show/792118/ | |
| 21:51:41 | melwitt | sean-k-mooney: ahhh mine eyes! | |
| 21:52:43 | sean-k-mooney | hehe i have to create an event loop to get it to interleave and emulate two compute services so there is a bit of boiler plate | |
| 21:53:03 | mriedem | or just stop using globals | |
| 21:53:05 | bauzas | sean-k-mooney: lol | |
| 21:53:11 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: Pass allocations to virt drivers when resizing https://review.opendev.org/589085 | |
| 21:53:12 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: Pass allocations to virt drivers when reverting resize https://review.opendev.org/712118 | |
| 21:53:12 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: Allocate mdevs when resizing or reverting resize https://review.opendev.org/712741 | |
| 21:53:19 | bauzas | sean-k-mooney: I'm just uploading the series ^ | |
| 21:53:41 | bauzas | aaaaaand I'm done today (well, tomorrow is in 7 mins) | |
| 21:53:51 | sean-k-mooney | well stop using gloabals means deviate form how oslo conf is used everywhere else in openstack | |
| 21:53:51 | melwitt | time to start again in 7 minutes! | |
| 21:53:52 | artom | mriedem, well, a global per-process CONF makes sense | |
| 21:54:07 | artom | It's not like different bits of a single nova process can have different configs | |
| 21:54:08 | sean-k-mooney | not that im saying we should not stop using globals | |
| 21:54:43 | bauzas | sean-k-mooney: artom: melwitt: honestly, let's discuss it on the change I'll provide tomorrow morning my time (ie. using virtapi.host field instead of CONF.host) | |
| 21:54:45 | mriedem | sean-k-mooney: it's not used globally in placement, intentionally | |
| 21:54:45 | artom | Though I suppose single global config != single global variable | |
| 21:54:48 | sean-k-mooney | i just wanted to point out we cauld create muliple patchers for the config an activate them as needed | |
| 21:54:49 | mriedem | cdent burned all of that out | |
| 21:55:12 | bnemec | Ugh, really? | |
| 21:55:12 | artom | Smart man | |
| 21:55:14 | sean-k-mooney | mriedem: ya well that was because they had the same proablem and bit the bullet | |
| 21:55:16 | bnemec | Please don't do that. | |
| 21:55:16 | mriedem | and it's not meant to be used globally, the oslo docs even say that i think, but that's what's happened | |
| 21:55:46 | artom | Anyways, I have to make supper | |
| 21:55:53 | sean-k-mooney | bnemec: stop using global or my fun with mock | |
| 21:55:55 | artom | I do enjoy these drive-by discussions :P | |
| 21:56:25 | mriedem | https://docs.openstack.org/oslo.config/latest/reference/faq.html#why-does-oslo-config-have-a-conf-object-global-objects-suck | |
| 21:56:44 | mriedem | artom: i was summoned earlier | |
| 21:57:11 | artom | mriedem, summoned eh? You're an Eldrich God? | |
| 21:57:18 | bauzas | again, I agree with mriedem, I think the simpliest would be to just stopping to use CONF.host everytime in libvirt | |
| 21:57:20 | artom | At least you don't talk of yourself in the third person ;) | |
| 21:57:21 | bnemec | mriedem: That's saying you _should_ use the global object because it's what everyone else is doing. | |
| 21:57:27 | mriedem | more like a pit fiend | |
| 21:57:43 | mriedem | bnemec: it concedes | |
| 21:57:50 | mriedem | "yes it sucks but everyone is doing it so sure" | |
| 21:58:19 | artom | Ah, the old jump off a bridge debate | |
| 21:58:19 | bnemec | I can tell you it would break parts of the policy generator if you dropped it. | |
| 21:58:29 | artom | mriedem, dammit, jinx! | |
| 21:59:09 | mriedem | bnemec: tbc, the suggestion was to stop using the global config to access one variable in one very specific place in the libvirt driver so that functional tests weren't mocking or hacking global config to workaround its global-ness | |
| 21:59:28 | mriedem | bnemec: nova is never going to fully move off global config usage, it's just that one spot i was talking about | |
| 22:00:07 | bnemec | That sounds perfectly reasonable. | |
| 22:00:33 | bauzas | again, I think the easiest is to just stop using global objects as much as we can | |
| 22:00:46 | bauzas | so, bye CONF.host and just check what we need once | |
| 22:01:05 | bnemec | The one danger of not looking it up from the conf object each time is that mutable config won't work correctly, unless you write a hook to update it. | |
| 22:01:27 | bnemec | But that's both solvable and not an issue if that config opt isn't marked mutable. | |
| 22:05:51 | sean-k-mooney | this is a cleaner verion of the same thing by the way http://paste.openstack.org/show/792119/ | |
| 22:08:11 | sean-k-mooney | compute-1 and comptue-2 have there own view of the global object that they can modify without affecting each other. | |
| 22:09:26 | sean-k-mooney | this is still rather archane so im not really sure i would want to do this but if i coudl make it into a nice decorator to hide this i could be an option. anyway i just wanted to play with mocking and see what i could do | |
| 22:12:32 | sean-k-mooney | bnemec: we dont really support mutable config for many config options today in nova | |
| 22:13:00 | sean-k-mooney | bnemec: some stuff yes but you cant change the virt dirver while its runing or many of the virt driver options | |