Skip to content

Remove --free, implement shared CPU/memory and automatic defaults#55

Merged
nathanjcochran merged 10 commits intomainfrom
nathan/remove-free-flag
Oct 14, 2025
Merged

Remove --free, implement shared CPU/memory and automatic defaults#55
nathanjcochran merged 10 commits intomainfrom
nathan/remove-free-flag

Conversation

@nathanjcochran
Copy link
Member

@nathanjcochran nathanjcochran commented Oct 13, 2025

See Slack thread and AGE-208 for background.

This implements the changes in AGE-208, with one relatively minor difference: I did not implement explicit "auto" options for each flag. Instead, I just removed the default value for each flag, and am interpreting the zero value (i.e. when the flag is omitted) to mean "auto". This was just a little easier to implement as a first pass, as it meant that I didn't need to update as much of the --help text to include references to "auto" or explain what it means. I think it would be easy to add explicit "auto" options as a follow-up, if we're sure we want it.

This PR:

  • Gets rid of the --free flag.
  • Adds support for specifying shared for --cpu and --memory (this is how you create a free services now).
  • Gets rid of the explicit defaults for --addons, --region, --cpu, and --memory
    • These parameters are no longer sent in the API request if they aren't specified.
    • The default values are now determined in the API layer.
    • The --cpu and --memory defaults now depend on what type of plan you're on (the free plan defaults to shared CPU/memory, whereas paid plans default to 0.5 CPU/ 2 GB.
    • The --addons and --region defaults depend on what type of service you're creating.
      • If you're creating a free service (shared CPU/memory), --addons defaults to time-series,ai, whereas if you're creating a paid service, it defaults to time-series.
      • --region currently always defaults to us-east-1, but we intend to have it intelligently choose the best region for paid services in the future.
  • Makes corresponding changes to the service_create MCP tool.

How to test:

  • Check out https://github.com/timescale/savannah-public/pull/51, build and run it: go install ./... && server
  • Check out this branch and build it: go install ./...
  • Configure your CLI to point to your local savannah-public server: tiger config set api_url http://localhost:8080/public/api/v1
  • Create a service: tiger service create (default service type depends on your plan)
  • Create a free service: tiger service create --cpu=shared

Related PRs:

@nathanjcochran nathanjcochran self-assigned this Oct 13, 2025
@nathanjcochran nathanjcochran marked this pull request as ready for review October 14, 2025 13:20
Copy link
Contributor

@Askir Askir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally looks good to me but could use a bit better explanation I think

Comment on lines +431 to +432
cmd.Flags().StringVar(&createCpuMillis, "cpu", "", "CPU allocation in millicores or 'shared'")
cmd.Flags().StringVar(&createMemoryGBs, "memory", "", "Memory allocation in gigabytes or 'shared'")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bit confusing with the default behavior not being mentioned?

Copy link
Member Author

@nathanjcochran nathanjcochran Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I think it's going to be hard to fit an explanation of the default behavior into the flag descriptions (whereas previously, it was easy to just list a default value). We can list auto as the default in a follow-up PR if we think that's clearer (I think that change should be backwards-compatible), but I still don't think it would really explain much.

So I think it needs to go in the main --help text. How does this look for now (at least as a first pass - can definitely improve later)? 346aae9

8 CPU (8000m) / 32GB | 16 CPU (16000m) / 64GB | 32 CPU (32000m) / 128GB
Allowed CPU/Memory Configurations:
shared / shared | 0.5 CPU (500m) / 2GB | 1 CPU (1000m) / 4GB | 2 CPU (2000m) / 8GB
4 CPU (4000m) / 16GB | 8 CPU (8000m) / 32GB | 16 CPU (16000m) / 64GB | 32 CPU (32000m) / 128GB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at least here we should document the default behavior. I'd actually prefer to have auto/auto as a specific option but even if we don't have it we should explain what not providing anything does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agree. See my other comment above.

nathanjcochran and others added 2 commits October 14, 2025 10:43
Co-authored-by: Jascha Beste <bestejascha@gmail.com>
Signed-off-by: Nathan Cochran <nathanjcochran@gmail.com>
@nathanjcochran nathanjcochran merged commit bdc046e into main Oct 14, 2025
2 checks passed
@nathanjcochran nathanjcochran deleted the nathan/remove-free-flag branch October 14, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants