AWS Capacity Reservation support #1977
Conversation
…nto issue_1155_aws_capacity_block
|
One addition: from what it seems, Capacity Reservations and Capacity Blocks share the same API with negligible differences, so one can replace the other for testing, at least in this scenario. |
There was a problem hiding this comment.
Hi @solovyevt, thanks for this draft, everything looks good so far.
explicit validation on CLI client, such as verifying that
spot_policy: spotandreservation: <id>are not provided simultaneously
I think validation in CLI is optional. The server will still need to filter out spot offers if the configuration specifies spot_policy: auto.
filter out
spot: trueofferings during the offer acquisition, but could not find a way to do it in the least intrusive way
As an option, this filtering can be done in AWSCompute.get_offers if you'll make this method aware of whether a capacity block is used or not.
Is it okay if I expand the base class Compute with additional methods for reservation validation?
Yes, feel free to add new methods or update the signatures of existing ones as you see fit. I think the validation could be done in AWSCompute.get_offers if you make it aware of capacity blocks, but that choice is up to you.
|
@jvstme Ready for review |
solovyevt
changed the title
| @@ -51,6 +53,7 @@ def get_fleets_table(fleets: List[Fleet], verbose: bool = False) -> Table: | |||
|
|
|||
| row = [ | |||
| fleet.name if i == 0 else "", | |||
| fleet.spec.configuration.reservation if i == 0 else "", | |||
There was a problem hiding this comment.
Only include reservation value if verbose. Otherwise it shifts all the values without verbose:
✗ dstack fleet
FLEET INSTANCE BACKEND RESOUR… PRICE STATUS CREATED
popular-rattlesnake-1 0 aws 2xCPU, $0.107 provis… now
(eu-we… 8GB,
100.0GB
(disk)
| instance_count: Optional[int] = 0, | ||
| instance_types: Optional[List[str]] = None, | ||
| is_capacity_block: Optional[bool] = False, | ||
| ) -> Optional[str]: |
There was a problem hiding this comment.
I believe it's not an str that's returned?
| instance_count: Optional[int] = 0, | ||
| instance_types: Optional[List[str]] = None, | ||
| is_capacity_block: Optional[bool] = False, |
There was a problem hiding this comment.
I don't think there is a need to make instance_count or is_capacity_block optional – just having default values would do.
|
Pointed out some issues, but LGTM overall. Leaving it to @jvstme for a more thorough review. |
| reservation: Annotated[ | ||
| Optional[str], | ||
| Field(description="The existing reservation for the instances"), | ||
| ] = None |
There was a problem hiding this comment.
Introducing a new field breaks client backward compatibility since a new client will send a value to an old server that will reject it. The CLI can explicitly handle that (some examples https://github.com/dstackai/dstack/blob/a7410ac605a37bc4342c9b4f5a1a079c882674f9/src/dstack/api/server/_runs.py#L115-L127)
Probably @jvstme can help with that.
There was a problem hiding this comment.
I've addressed backward compatibility in 2b93408, but would be great to have it validated. Tested myself against the latest server version, i.e. 0.18.25.
| reservation: Annotated[ | ||
| Optional[str], | ||
| Field(description="The existing reservation for the instances"), | ||
| ] = None |
There was a problem hiding this comment.
Just pointing out another client backward incompatibility that we need to address.
|
@Solovyev, fix the tests please. |
|
@r4victor Python tests are fixed. For the future, is it possible to run them locally? Couldn't find any info in contribution docs. |
|
@solovyevt, we somehow missed the section on tests in contributing. I updated it: https://github.com/dstackai/dstack/blob/master/CONTRIBUTING.md#run-tests |
|
Please wait with the merge. Suddenly realised that there might be a nasty bug related to resource quota filtering in To validate the fix more thoroughly, I will test it on one of our upcoming Capacity Blocks next Monday. Sorry for the trouble. |
Closes #1155.
Implemented
reservationprofile parameter. It is not a final version wanted to check a few things first:Wasn't sure if I should include any explicit validation on CLI client, such as verifying that
spot_policy: spotandreservation: <id>are not provided simultaneously. It makes more sense to implicitly filter outspot: trueofferings during the offer acquisition, but could not find a way to do it in the least intrusive way.Couldn't squeeze in server-side capacity block validations inside
AWSComputewithout modifying the signatures of existing functions. For example, capacity blocks should certainly be validated in regards to their available capacity and instance type, and this is something that cannot extracted just fromrequirementsalone. Is it okay if I expand the base classComputewith additional methods for reservation validation?This is only a fleet implementation in the moment, some minor changes for dev environments are left out on purpose to avoid intrusive plumbing without checking up with you first. Thanks!