ibpsa / project1-boptest

Building Optimization Performance Tests
Other
104 stars 69 forks source link

Add a quit command #541

Closed Enderdead closed 1 year ago

Enderdead commented 1 year ago

I'm using BOPTEST for my personal research and would like to structure my experiments with only ONE script to execute. The simulator architecture requires us to launch the server on one side and the client on the other side. These two processes can be merged into one script thanks to the subprocess library. However, as the Flask server is designed to function as a persistent server, it won't terminate itself if the client ends its tasks on this instance. This means that my merged script cannot finish and requires a keyboard interrupt.

To solve this issue, I propose the addition of a quit command to shut down the server remotely. Adding this command wouldn't alter the user experience but it would offer a new way to manage the server. I have already implemented the REST command, but before making a pull request, I would appreciate your feedback on this.

Thank you in advance for your attention.

dhblum commented 1 year ago

Thank you for reporting and suggesting. I understand the need. In boptest-service we have the endpoint /stop. See the API readme there. I'd propose using the same API call here to shutdown the running test container, which is comparable to the same request in Service. @kbenne @JavierArroyoBastida Thoughts?

javiarrobas commented 1 year ago

@Enderdead what would be the difference between the existing /stop endpoint and the command you propose? it'd be maybe good idea to send a draft pull request to better understand.

dhblum commented 1 year ago

@JavierArroyoBastida The /stop endpoint only exists when using Service right now. This would add it to the core API when a test case is deployed locally.

javiarrobas commented 1 year ago

I see. Thanks for clarifying! is there any backward incompatibility this could bring? otherwise I think it's an added value to have it.

Enderdead commented 1 year ago

From my standpoint, there should be no compatibility issues, as the new feature introduces an independent command. I have already incorporated this command into my fork of BOPTEST (see here: 2070737). I invite you to review my implementation if needed. I will await your approval before submitting a merge request.

dhblum commented 1 year ago

@Enderdead I thought about this a little more and wondered how you start a test case with your script, even after quitting one? Test cases are started with docker-compose up and can be similarly shut down with docker-compose down, which is actually preferred over the keyboard interrupt because it removes the network as well. I ask because, I wonder if your script uses docker-compose up to start a test, it could similarly use docker-compose down to stop it?

Enderdead commented 1 year ago

I'm using the docker command directly instead of docker-compose to update the port, so yes, your approach works. The docker stop command is able to stop the server, but it takes quite long, about 10 seconds. The docker kill command works as intended but it's not really clean.

I will proceed with the docker kill command.

I'm not sure if you still want to pursue the idea of a quit command, but as it stands, it's fine.

I close the issue for now.

dhblum commented 1 year ago

Ok thanks a lot @Enderdead. The initial value I thought the quit command would have is for the most part nullified I think due to the need for a docker command to start up a test case anyway. I suppose there's an argument for separating the starting of the docker container and then starting up of a specific test case, but that's a more intrusive change which I don't think is a priority right now. I'm inclined not to pursue this, nor the quit command, at the moment.