[Serve] Fix adding initial version and cleanup service versions

[cblmemo]

Originally, in our serve state,

• We create the initial insertion to version_specs in the __init__ function of ReplicaManager, which is not attached to insertion to the services table. This could sometimes lead to a misalign of the two tables;
• We do not cleanup the version information after its termination. Although we clean all outdated version in our controller process, this will left one final valid version at termination.

This PR fixed the two problems.

TODO: test.

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
• Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
• Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

[Michaelvll]

Thanks for the fix @cblmemo! Left two quesetions.

[Michaelvll]

Can we write down in what case the two tables will be misaligned if we have always handled the two database at the same time?

[cblmemo]

Previously, the system added an initial version in the __init__ function of replica managers and added the service record here

skypilot/sky/serve/service.py

Lines 146 to 151 in 9e27a81

	success = serve_state.add_service(
	service_name,
	controller_job_id=job_id,
	policy=service_spec.autoscaling_policy_str(),
	requested_resources_str=backend_utils.get_task_resources_str(task),
	status=serve_state.ServiceStatus.CONTROLLER_INIT)

If the code is aborted before adding the version and after adding the service, it will cause a misalignment. In this PR we move the adding of initial version closer to the service table, to reduce the possibility of causing such misalignment, though arguably it is still 'possible' to happed, so I added a safe guard here.

[cblmemo]

An example of abortion would be shutil lib call to fail, e.g. permission issues

[Michaelvll]

Was the previous issue mainly because the clean up not deleting the final version from the version_spec table, which is fixed by the delete_all_versions now?

If this safe guard is only for rare case, it would be great to mention that this is rare in the comment here to make sure we still know whether this is a common case in the future. : )

[cblmemo]

Oh, sorry for not making it clear. The previous issue is the misalignment of service and version_spec, which causes the _get_service_status to return a None (due to the joining of the two tables), and causes the assertion to abort the deletion of the service.

skypilot/sky/serve/serve_utils.py

Lines 415 to 417 in 9e27a81

	service_status = _get_service_status(service_name,
	with_replica_info=False)
	assert service_status is not None, service_name

I found that we did not clean up the remaining version when I debugged the original issue, and fix that in the same PR along the way.

[cblmemo]

If this safe guard is only for rare case, it would be great to mention that this is rare in the comment here to make sure we still know whether this is a common case in the future. : )

Good point! Added a comment for this. Thanks!

[Michaelvll]

Just curious, is this movement related to the bug?

[cblmemo]

Yes, please see the comment above 🫡

[Michaelvll]

Thanks for the fix @cblmemo! LGTM.