Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-02-27
15:32:51 sean-k-mooney and then it never gets updated again
15:34:35 sean-k-mooney mordred: ^ that is also relevent to you i guess
15:37:34 mordred yeah. that - if we're using the api request token, then when we get the singleton sdk connection object, we need to call conn.connect_as on it to get a new connection with the credentials we've been given, otherwise we're making all of the requests with the creds of the original user :)
15:38:09 sean-k-mooney ya we are not doing that
15:38:19 sean-k-mooney so that is likely the bug
15:38:35 mordred conn.connect_as lets us keep discovery cache appropriately and all of that - but will construct a new underlying Auth construct and a new Adapter
15:38:59 mordred sean-k-mooney: computers are hard
15:39:18 sean-k-mooney mordred: we would do that here right https://review.opendev.org/#/c/631242/51/nova/accelerator/cyborg.py@32 just before the retrun?
15:39:32 sean-k-mooney although im not sure that would be safe
15:39:47 sean-k-mooney we could context switch at any point so we would have to take the lock
15:39:58 sean-k-mooney and do it before making any call
15:41:49 mordred sean-k-mooney: sdk is threadsafe
15:41:55 mordred you shouldn't need a lock
15:42:12 sean-k-mooney is it eventlet safe
15:42:22 mordred _should_ be
15:42:25 mordred if it's not it's a bug
15:42:38 sean-k-mooney if the con.connect_as returns a new object that we then make new calls on it should work
15:42:42 mordred I mean - but that said - with eventlet who the hell knows
15:42:53 mordred sean-k-mooney: yes - connect_as returns a new object
15:43:29 mordred it returns a whole new Connection with all the settings of the old one but with just the auth bits replaced
15:43:32 sean-k-mooney and that object basicaly poxies the requeqes on it to the adapter it was created form but with the update creds
15:44:21 mordred it has to create a new adapter - because an adapter is a container of session+auth+endpoint-config - you can't use the old adapter if you have new auth, you need a new adapter
15:44:36 sean-k-mooney is so then we just need to change "return _CYBORG_CLIENT" to "return _CYBORG_CLIENT.connect_as(context)
15:44:36 mordred but it does keep/share the things that are safe to keep/share
15:44:43 mordred yeah
15:45:13 mordred with a little hand wavey, because we'll want to make sure we're doing the right things with context ... but pseudo-code-wide yes
15:45:36 sean-k-mooney ok so ill swap to the sdk adapter and add the connect_as
15:46:49 sean-k-mooney mordred: ya, this isnt my code and i havent realy review it im just testing it but i can relyably repoduce that the current code is borked so tring to fine the minimal correct fix and test that in a follow up patch
15:47:09 sean-k-mooney if it fixes the issue then we can squash it in and move on to the next thing
15:47:41 mordred ++
15:48:00 mordred let me know if you have issues and want me to dive in further
15:48:11 mordred I'm sure I could find some lovely new ways to curse
15:56:00 efried If we have to build a new adapter anyway, then I suppose we might as well just do as dansmith suggests and un-singleton for now. We'll want to do the SDK cutover later, but there's no way we'll forget to do that.
15:56:14 dansmith ++
15:57:48 sean-k-mooney im not sure if the connect_as is cheaper then building the adapter from scratch but ya i just want to see this working and then iterate on it later to not loose the momentem we have while people are around to ask questions
15:58:15 sean-k-mooney *answer
15:58:49 openstackgerrit Stephen Finucane proposed openstack/os-resource-classes master: Switch to hacking 2.x https://review.opendev.org/710276
16:34:55 openstackgerrit Merged openstack/os-traits master: setup.cfg: Configure 'python-requires' https://review.opendev.org/704534
16:34:56 openstackgerrit Merged openstack/os-traits master: setup.cfg: Remove unused 'build_sphinx' configuration https://review.opendev.org/704535
16:40:46 gibi sean-k-mooney: re: provider config. I have to take tomorrow off so I will look at your update earliest on Monday
16:48:04 Sundar dansmith, gibi, sean-k-mooney, efried: On the rescheduling front, I don't have a multi-node env to test it. It is not going to be easy for me to get the lab resources, even without FPGAs, to set that up. Plus, I have other things I need to focus on. Any ideas on bringing this to a timely closure?
16:48:27 dansmith Sundar: functional tests?
16:48:42 efried Sundar: Also, sean-k-mooney set up a multinode CI job.
16:49:21 dansmith multinode ci might be hard to trigger a reschedule though
16:49:54 dansmith Sundar: tangential to this part, did you see my comment about moving the event to the front of the queue, below the -2'd ksa patch?
16:50:52 Sundar dansmith: Yes, I saw that. I have moved it before the ksa patch in my local env. The next commit should have it.
16:50:59 dansmith Sundar: cool
16:51:14 Sundar dansmith: With functional tests, I can check if the logs show rescheduling messages, and that the tests pass. Is there anything more you are looking for?
16:51:49 efried You can do better than that to prove a reschedule occurred: use the fixture that makes the initial host predictable and prove that it lands on a different one.
16:52:11 dansmith Sundar: "tests passing" isn't a bar to meet, "tests validating something useful" is
16:52:11 gibi efried ++
16:52:12 efried there should be some good examples
16:52:15 dansmith yep, what efried said
16:52:48 Sundar efried: Thanks, will look for that. If you happen to remember the(rough) name of the fixture, off the top of your head, I'll start with that.
16:53:00 dansmith Sundar: what specifically are you stuck on? there's lots of feedback to work on without much response from you yet that I can see.. is there something specific that you're blocked on?
16:53:46 Sundar dansmith: Not sure that any of my changes will actually work in the real world, without a multinode env. I tried to get a 2nd server but it will take time, if at all.
16:53:53 efried Sundar: HostNameWeigherFixture I think
16:53:58 gibi Sundar: HostNameWeigherFixture makes the host selection peridctable
16:54:11 dansmith Sundar: functional tests man :)
16:54:12 Sundar efried, gibi: Thanks, got it
16:54:50 dansmith Sundar: if you have reasonable functional tests that don't mock out insane levels of things and validate reasonable conditions, then any real world failure we find later is understandable
16:55:53 dansmith Sundar: test_servers.py has a bunch of reschedule-specific tests
17:00:36 Sundar dansmith: Ok, let's just focus on functional tests, then. BTW, https://review.opendev.org/gitweb?p=openstack/nova.git;f=nova/conductor/manager.py;hb=refs/changes/44/631244/63#l602 is specific to the resheduling path AFAICS. is there any other path, like rebuild/resize/whatever, that invokes this?
17:01:46 dansmith Sundar: only if one of those things needs a reschedule, AFAIK
17:04:14 Sundar Ok. BTW, I will be travelling over the next couple of days. I will respond with the comments I accepted by today, and will post the rest in a few days.
17:04:36 dansmith okay
17:05:55 Sundar dansmith: In https://review.opendev.org/#/c/631244/63/nova/conductor/manager.py@848, is there any resource to be cleaned up in the exception path?
17:07:29 dansmith Sundar: seems like there's network and block devices to clean up, but I'm not sure why the error paths above aren't doing that
17:07:37 dansmith Sundar: should you be deleting arqs?
17:07:55 dansmith or are you assuming those get deleted when the error'd instance gets deleted?
17:08:19 dansmith Sundar: there is a _cleanup_after_reschedule_fails() method in there...
17:08:23 sean-k-mooney gibi: ack that is fine. i likely will not be ready until then anyway
17:09:02 dansmith er, cleanup_when_reschedule_fails
17:09:10 dansmith which cleans up allocated network resources at least
17:09:25 Sundar Well, not in this patch for sure, because delete ARQ functionality comes later. However, we should do that in the delete patch because Create ARQs may succeed but bind may throw an exception potentially. Then, Cyborg should be informed to delete the created ARQs.
17:09:54 dansmith Sundar: okay your existing code won't even leave the instance in error state, which is bad.. but that cleanup method will do that for you
17:10:44 Sundar dansmith: Yes, I see it. Good. Thanks.
17:11:39 Sundar dansmith: I could tie the delete ARQs to _cleanup_when_reschedule_fails(). I'll check to see what other impact it has.
17:12:18 dansmith Sundar: if deleting the instance in un-scheduled state will clean those up, then it's probably okay to punt to that.. the only problem might be if the ARQs are holding some resource for the dead instance
17:14:47 Sundar dansmith: If the bind succeeds, then ARQs are holding real resources. So, if some other task fails during rescheduling, then Cyborg should be told to delete the ARQs. I think the changes we already have should take care of that.
17:17:27 dansmith Sundar: right but the bind is right before the call back to compute, so if that fails, no bind was done, and if it has failed before that, we're already unbound from the original host, so not holding resource... is that right?
17:21:02 Sundar fail. It is this second path that I am not terribly sure about.
17:21:02 Sundar dansmith: bind is async. The call here just kicks off the process, and the actual bind may succeed or fail after that. If the bind fails, either Cyborg sends a failure notification or Nova gets a timeout (in corner cases). That is already handled. if the bind succeeds, Cyborg has allocated and prepared resources. But something else in Nova may
17:21:36 dansmith I think you're missing my point
17:21:56 dansmith or maybe you're talking about the reschedule racing with the *original* bind?
17:24:51 Sundar dansmith: I am talking of the bind during the rescheduling in https://review.opendev.org/#/c/631244/63/nova/conductor/manager.py@842 . Yes, if there is a failure in _create_and_bind_arqs( ) or earlier, no resources are allocated.
17:25:33 dansmith Sundar: so you're concerned about what happens if the bind during reschedule fails and not knowing whether things are actually bound or not?
17:25:48 dansmith if so that's... pretty scary
17:25:51 Sundar I was talking of the scenario where _create_and_bind_arqs() succeeds, Nova goes ahead with the reschedule, CYborg prepares devices in the new host, and then things go south.
17:26:00 Sundar Things go south in Nova
17:26:20 Sundar So, every failure path in reschedule must call Cyborg to delete ARQs
17:26:29 dansmith depends on where it goes south, but there's not much we can do about it
17:26:45 dansmith try to delete the arqs when we can in error paths,
17:27:02 dansmith but as long as the instance delete will clean things up gracefully, that's the safety net
17:27:49 dansmith after your bind in the reschedule path, the rest of it is identical to the regular build process
17:27:50 Sundar I am trying to see if I am missing any error paths. AFAICS, we haven't missed any in the conductor. In the compute manager, if the reschedule takes the same code paths as the priginal schedule, we should be good.
17:27:58 dansmith right
17:28:18 Sundar Ok, then we are good
17:28:25 dansmith if your bind in reschedule in conductor succeeds, then it's a regular build after that
17:45:13 openstackgerrit Merged openstack/os-traits master: Remove use of six https://review.opendev.org/710274

Earlier   Later