Add a utility to perform a gradual split #116

stephen-soltesz · 2023-02-14T01:43:14Z

This change adds a deployment utility to gradually split traffic to a new version. This is meant to address:

App Engine deployment of Locate with --promote causes brief outage #114

This change is

coveralls · 2023-02-14T01:49:31Z

Pull Request Test Coverage Report for Build 895

69 of 69 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.08%) to 98.053%

Totals
Change from base Build 892:	0.08%
Covered Lines:	1712
Relevant Lines:	1746

💛 - Coveralls

cristinaleonr

This is a nice solution. Thank you!

I can't help but feel like we are implementing functionality that should already come out of the box :/.

As a potential alternative, from the description, it seems like the initial_delay_sec setting can give new instances more time to initialize before they are ready to serve traffic.

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

cloudbuild/cloudbuild.yaml line 18 at r2 (raw file):

  - go test -v ./...

# Run unit tests for environment.

Perhaps this should say build gradual-split command?

cmd/gradual-split/main.go line 52 at r2 (raw file):

}

func (a *AppAPI) ServiceUpdate(ctx context.Context, serviceID string, service *appengine.Service, mask string) (*appengine.Operation, error) {

Should this have a docstring?

cmd/gradual-split/internal/split.go line 43 at r2 (raw file):

				continue
			}
			if verFrom >= version.Id {

Is this if statement needed? The scenario seems to be covered by the one below.

cmd/gradual-split/internal/split.go line 82 at r2 (raw file):

		}
	case len(service.Split.Allocations) == 2:
		// Assume the split is already in progress.

This seems to be called once from main. Under what circumstances would the split already be in progress?

cmd/gradual-split/internal/split.go line 98 at r2 (raw file):

// each step. PerformSplit can resume a split in progress.
func PerformSplit(ctx context.Context, api AppWrapper, service *appengine.Service, opt *SplitOptions) error {
	// delay time.Duration, vfrom, vto string) error {

Is this needed?

cmd/gradual-split/internal/split_test.go line 33 at r2 (raw file):

		return nil, a.serviceErr
	}
	return nil, nil // a.apis.Apps.Services.Patch(project, serviceID, service).UpdateMask(mask).Do()

Is this commented out code needed?

stephen-soltesz

I agree it feels strange that Flexible doesn't support gradual traffic migration between versions as Standard env does. https://cloud.google.com/appengine/docs/flexible/migrating-traffic

Do you suspect that initial_delay_sec would solve the --promote issue? The outage from #114 latest at least 5min at most 10min. Is the root cause that the VMs were either "not live" or "not ready" before the promote takes effect? Either way, it coul be time for explicit live and ready handlers.

Reviewable status: 0 of 1 approvals obtained (waiting on @cristinaleonr)

cloudbuild/cloudbuild.yaml line 18 at r2 (raw file):