Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature detect diff-based bandwidth-optimized PATCH protocol for saving checkpoints #11421

Merged
merged 13 commits into from Nov 28, 2022
Merged

Feature detect diff-based bandwidth-optimized PATCH protocol for saving checkpoints #11421

merged 13 commits into from Nov 28, 2022

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Nov 21, 2022 •
edited

Description

Starts using diff-based PATCH protocol for saving checkpoints for backends that indicate that they support it via GET /api/capabilities. Removes flags that opted into the feature. Specifically:

  • call GET /api/capabilities in parallel with GetStack call so as not to add to critical path latency
  • interpret 404 Not Found as "no capabilities" for backwards compatibility and not forcing a simultaneous backend update
  • let the response from /api/capabilities configure a minimal stack size cutoff for opting into the diff-based protocol
  • opens the possibility to tune the optimal cutoff with service experimentation

Fixes https://github.com/pulumi/home/issues/2425

I've tested the change against the mainline backend that does not support capabilities yet (404 path) and the experimental backend that does, including both sides of the size cutoff, and it seems to work as expected.

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@t0yv0 t0yv0 requested a review from caseyyh November 21, 2022 18:43
@pulumi-bot
Copy link
Contributor

pulumi-bot commented Nov 21, 2022 •
edited

Changelog

[uncommitted] (2022-11-22)

Features

  • [backend/service] Allows the service to opt into a bandwidth-optimized DIFF protocol for storing checkpoints. Previously this required setting the PULUMI_OPTIMIZED_CHECKPOINT_PATCH env variable on the client. This env variable is now deprecated.
    #11421

@t0yv0 t0yv0 marked this pull request as ready for review November 21, 2022 21:37
@t0yv0 t0yv0 changed the title Feature detect delta checkpoints Feature detect diff-based bandwidth-optimized PATCH protocol for saving checkpoints Nov 21, 2022
@t0yv0 t0yv0 requested a review from a team November 21, 2022 22:13
justinvp
justinvp approved these changes Nov 21, 2022
View reviewed changes
Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

pkg/backend/httpstate/backend.go Outdated Show resolved Hide resolved
pkg/backend/httpstate/client/client.go
func (pc *Client) GetCapabilities(ctx context.Context) (*apitype.CapabilitiesResponse, error) {
var resp apitype.CapabilitiesResponse
err := pc.restCall(ctx, http.MethodGet, "/api/capabilities", nil, nil, &resp)
if is404(err) {
Copy link
Member

Choose a reason for hiding this comment

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

I suspect a customer maintaining a fork of the CLI may not even want the client to call a nonexistent /api/capabilities endpoint. We could ask.

I wonder if we should have a disableCapabilities bool field (or better name) on the Client struct, that a customer could set to true from their own implementation of newClient, that when set would avoid making the request to /api/capabilities at all, and have this method return early with &apitype.CapabilitiesResponse{} like the 404 case.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right I was also worried about the 404s tripping some alarms, I thought we cleared it already but I'll double-check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added DisableCapabilityProbing on Client struct.

pkg/backend/httpstate/backend.go Outdated Show resolved Hide resolved
iwahbe
iwahbe approved these changes Nov 22, 2022
View reviewed changes
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

I have 1 nit, otherwise it looks good.

pkg/backend/httpstate/client/client.go Outdated Show resolved Hide resolved
@t0yv0 t0yv0 force-pushed the t0yv0/feature-detect-delta-checkpoints branch from 6bec243 to 134621f Compare November 22, 2022 16:40
@t0yv0 t0yv0 force-pushed the t0yv0/feature-detect-delta-checkpoints branch from afb91d6 to 1e4aa31 Compare November 22, 2022 23:17
@t0yv0
Copy link
Member Author

t0yv0 commented Nov 28, 2022

bors merge

@bors
Copy link
Contributor

bors bot commented Nov 28, 2022

Build succeeded:

@bors bors bot merged commit 377982b into master Nov 28, 2022
@pulumi-bot pulumi-bot deleted the t0yv0/feature-detect-delta-checkpoints branch November 28, 2022 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinvp justinvp justinvp approved these changes

@iwahbe iwahbe iwahbe approved these changes

@caseyyh caseyyh caseyyh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@t0yv0 @pulumi-bot @justinvp @iwahbe @caseyyh