Switch to CFT region and add more robust tracking and cleanup of stacks. #3701
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,8 +11,6 @@ import ( | |
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/elastic/elastic-agent/pkg/testing/runner" | ||
| ) | ||
|
|
||
|
|
@@ -62,89 +60,77 @@ func (p *provisioner) SetLogger(l runner.Logger) { | |
| p.logger = l | ||
| } | ||
|
|
||
| // Create creates a stack. | ||
| func (p *provisioner) Create(ctx context.Context, request runner.StackRequest) (runner.Stack, error) { | ||
| // allow up to 2 minutes for request | ||
| createCtx, createCancel := context.WithTimeout(ctx, 2*time.Minute) | ||
| defer createCancel() | ||
| resp, err := p.createDeployment(createCtx, request, | ||
| map[string]string{ | ||
| "division": "engineering", | ||
| "org": "ingest", | ||
| "team": "elastic-agent", | ||
| "project": "elastic-agent", | ||
| "integration-tests": "true", | ||
| }) | ||
| if err != nil { | ||
| return runner.Stack{}, err | ||
blakerouse marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| return runner.Stack{ | ||
| ID: request.ID, | ||
| Version: request.Version, | ||
| Elasticsearch: resp.ElasticsearchEndpoint, | ||
| Kibana: resp.KibanaEndpoint, | ||
| Username: resp.Username, | ||
| Password: resp.Password, | ||
| Internal: map[string]interface{}{ | ||
| "deployment_id": resp.ID, | ||
| }, | ||
| Ready: false, | ||
| }, nil | ||
| } | ||
|
|
||
| // WaitForReady should block until the stack is ready or the context is cancelled. | ||
| func (p *provisioner) WaitForReady(ctx context.Context, stack runner.Stack) (runner.Stack, error) { | ||
| deploymentID, err := p.getDeploymentID(stack) | ||
| if err != nil { | ||
| return stack, fmt.Errorf("failed to get deployment ID from the stack: %w", err) | ||
| } | ||
| // allow up to 10 minutes for it to become ready | ||
| ctx, cancel := context.WithTimeout(ctx, 10*time.Minute) | ||
|
There was a problem hiding this comment. We can make the timeout a parameter passed from the outside maybe ? (there are other instances throughout the code, all with slightly different timeouts, I would really prefer if we could configure them elsewhere and pass them in to the provisioners functions) There was a problem hiding this comment. I made it this way allowing the provisioner code to make the decision on what is the best timeout to be used based on what is being used. That way a timeout from a setting or default value doesn't effect the needed time. I believe its the provisioners that should be determine the appropriate amount of time. There was a problem hiding this comment. Well it seems to me that we have at least one occurrence where these timeouts are related or is it just coincidence? The timeout should be how long the caller is willing to wait, so why can't we pass that in ? In general I don't see the downside of putting the timeouts as parameters (even if at first we just pass constants in it) rather than hardcoding different timeouts in different functions There was a problem hiding this comment. That timeout places a hard limit just for caution. I could remove it, but if the provisioner doesn't place a timeout on its own execution then it would result in it blocking forever. If you prefer I can remove it here. There was a problem hiding this comment. I like it as it is, having only the context, if the caller wants a timeout, it can set the timeout in the context. There was a problem hiding this comment. I agree with the provisioner setting a timeout in case its caller does not, so we can avoid getting stuck wiating for ever, but the caller should have the flexibility to choose how long it is willing to wait. I like the idea of just using the context, the provisioner can check whether there is a deadline set in the context, if there isn't one, then it can set a hardcoded default one. There was a problem hiding this comment. I like how it is. There was a problem hiding this comment. Updated comment with a correct example: https://go.dev/play/p/KBEtIjKFho_b package main
import (
"context"
"fmt"
"time"
)
func main() {
ctx1, cancel1 := context.WithTimeout(context.Background(), 1*time.Millisecond)
defer cancel1()
ctx5, cancel5 := context.WithTimeout(ctx1, 5*time.Second)
defer cancel5()
start := time.Now()
select {
case <-ctx5.Done():
fmt.Printf("Child context is expired after %s\n", time.Since(start))
}
}This prints So with the code today the maximum and default timeout is 10 minutes, and callers can set a shorter timeout via the context. I think this is a perfectly reasonable way for this to work. The only debate should be if 10 minutes is a long enough maximum timeout. |
||
| defer cancel() | ||
| p.logger.Logf("Waiting for cloud stack %s to be ready [stack_id: %s, deployment_id: %s]", stack.Version, stack.ID, deploymentID) | ||
| ready, err := p.client.DeploymentIsReady(ctx, deploymentID, 30*time.Second) | ||
| if err != nil { | ||
| return stack, fmt.Errorf("failed to check for cloud %s [stack_id: %s, deployment_id: %s] to be ready: %w", stack.Version, stack.ID, deploymentID, err) | ||
| } | ||
| if !ready { | ||
| return stack, fmt.Errorf("cloud %s [stack_id: %s, deployment_id: %s] never became ready: %w", stack.Version, stack.ID, deploymentID, err) | ||
| } | ||
| stack.Ready = true | ||
| return stack, nil | ||
| } | ||
|
|
||
| // Delete deletes a stack. | ||
| func (p *provisioner) Delete(ctx context.Context, stack runner.Stack) error { | ||
| deploymentID, err := p.getDeploymentID(stack) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // allow up to 1 minute for request | ||
| ctx, cancel := context.WithTimeout(ctx, 1*time.Minute) | ||
| defer cancel() | ||
|
|
||
| p.logger.Logf("Destroying cloud stack %s [stack_id: %s, deployment_id: %s]", stack.Version, stack.ID, deploymentID) | ||
| return p.client.ShutdownDeployment(ctx, deploymentID) | ||
| } | ||
|
|
||
| func (p *provisioner) createDeployment(ctx context.Context, r runner.StackRequest, tags map[string]string) (*CreateDeploymentResponse, error) { | ||
| ctx, cancel := context.WithTimeout(ctx, 1*time.Minute) | ||
| defer cancel() | ||
|
|
||
| p.logger.Logf("Creating cloud stack %s [stack_id: %s]", r.Version, r.ID) | ||
| name := fmt.Sprintf("%s-%s", strings.Replace(p.cfg.Identifier, ".", "-", -1), r.ID) | ||
|
|
||
| // prepare tags | ||
|
|
@@ -168,26 +154,21 @@ func (p *provisioner) createDeployment(ctx context.Context, r runner.StackReques | |
| p.logger.Logf("Failed to create ESS cloud %s: %s", r.Version, err) | ||
| return nil, fmt.Errorf("failed to create ESS cloud for version %s: %w", r.Version, err) | ||
| } | ||
| p.logger.Logf("Created cloud stack %s [stack_id: %s, deployment_id: %s]", r.Version, r.ID, resp.ID) | ||
| return resp, nil | ||
| } | ||
|
|
||
| func (p *provisioner) getDeploymentID(stack runner.Stack) (string, error) { | ||
| if stack.Internal == nil { | ||
| return "", fmt.Errorf("missing internal information") | ||
| } | ||
| deploymentIDRaw, ok := stack.Internal["deployment_id"] | ||
| if !ok { | ||
| return "", fmt.Errorf("missing internal deployment_id") | ||
| } | ||
| deploymentID, ok := deploymentIDRaw.(string) | ||
| if !ok { | ||
| return "", fmt.Errorf("internal deployment_id not a string") | ||
| } | ||
| return deploymentID, nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can pass timeout as a parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above on why I didn't do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
I didn't get why not have the timeout configurable.
I read up to the top of the file and didn't get the reasoning to have the timeout hardcoded...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think he meant the comment chain in https://github.com/elastic/elastic-agent/pull/3701/files#r1383088811 which might have been above at one point but isn't necessarily now.