moves callbacks to reactor in PCU agent

jlashner commented 4 months ago

Fixes race condition from callbacks in PCU agent

Description

During HWP testing today Kyohei and I saw some strange behavior in when calling PCU agent operations through a client, which I am pretty sure are due to how this race condition interacts with the OCS timeout logic. The behavior we were seeing was that anytime we called an operation through an OCS client with a timeout that wasn't None, it would return a TIMEOUT error even when successful.

For testing, we changed the supervisor agent to not set any timeouts when interacting with the PCU agent, but I'm pretty sure this can be fixed by fixing that race condition in the agent re-structure.

Motivation and Context

Important for HWP supervisor

How Has This Been Tested?

This has not been tested.

Types of changes

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

[X] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.

jlashner commented 4 months ago

Testing before merging might be difficult with how things are setup on satp3 (see this issue), and we'd need to find system time.... so if we don't need to I'd rather not, this seems simple enough.

Ya I think that OCS PR would also help. I'm not sure how urgent this is because we were able to work around it in recent supervisor changes and HWP spin-up, spin-down and brake ops seem to be working consistently. But I think when that new OCS version is ready to release we should get agents running on it.

BrianJKoopman commented 4 months ago

Testing before merging might be difficult with how things are setup on satp3 (see this issue), and we'd need to find system time.... so if we don't need to I'd rather not, this seems simple enough.

I'm fine with going ahead on this, since it seemed to fix the problem when you tested initially fixing that race condition.

I saw that infrastructure issue go by, but haven't had time to look at the system really. Limited system time makes debugging that difficult too.

Ya I think that OCS PR would also help. I'm not sure how urgent this is because we were able to work around it in recent supervisor changes and HWP spin-up, spin-down and brake ops seem to be working consistently. But I think when that new OCS version is ready to release we should get agents running on it.

Cool, we could release whenever. I'll move it up my list, but not to the top if this isn't urgent.

simonsobs / socs