[Spot] Make spot_state use safe_cursor().

[concretevitamin]

When developing a basic Flask app that eventually calls into spot_state.get_spot_jobs(), SQLite complained about threading errors. With safe_cursor() this error was fixed. This PR changes the entire spot_state module to use it.

Tested (run the relevant ones):

• Code formatting: bash format.sh
• Any manual or new tests for this PR (please specify below)
• All smoke tests: pytest tests/test_smoke.py --managed-spot
  • running
• Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
• Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

[Michaelvll]

Thanks for identifying this issue @concretevitamin! Left a question about the consistency across the modules. : )

[Michaelvll]

It seems we have this in the sky.skylet.configs as well. Do we want to refactor it as a common utility function in sky.utils.db_utils?

Also, we have a db_utils.SQLiteConn which creates a thread-level connection for the database and serves for the same purpose in job_lib and global_user_state. Can we have choose one of them for all the modules using sqlite?

[concretevitamin]

Good point - moved it into db_utils.safe_cursor(). SQLiteConn doesn't come with auto-commit as a context manager so I didn't use it.

[Michaelvll]

Thanks @concretevitamin! LGTM.

One general question: will adding the commit after read-only queries degrade the performance, as we have quite many SELECT queries that do no modify the database?

[concretevitamin]

Thanks @concretevitamin! LGTM.

One general question: will adding the commit after read-only queries degrade the performance, as we have quite many SELECT queries that do no modify the database?

On a controller,

%timeit spot_state.get_spot_jobs()
513 µs ± 5.94 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

which is fast enough.