Skip to content

Add instance_max_run_duration_seconds config #242

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.

Sign up for GitHub

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

Open
phoenixuprising wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add instance_max_run_duration_seconds config #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d96b1d
Bump google.golang.org/api to 0.211.0
phoenixuprising Dec 10, 2024
f898092
Add instance_max_run_duration_seconds config
phoenixuprising Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .web-docs/components/builder/googlecompute/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,11 @@ builder.

- `preemptible` (bool) - If true, launch a preemptible instance.

- `instance_max_run_duration_seconds` (int64) - If set, the instance will be automatically terminated after the specified amount of seconds.
instance_termination_action must be set if this property is set.

- `instance_termination_action` (string) - The termination action for this VM, which can be either STOP or DELETE.

- `node_affinity` ([]common.NodeAffinity) - Sets a node affinity label for the launched instance (eg. for sole tenancy).
Please see [Provisioning VMs on
sole-tenant nodes](https://cloud.google.com/compute/docs/nodes/provisioning-sole-tenant-vms)
Expand Down
17 changes: 17 additions & 0 deletions builder/googlecompute/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,11 @@ type Config struct {
OnHostMaintenance string `mapstructure:"on_host_maintenance" required:"false"`
// If true, launch a preemptible instance.
Preemptible bool `mapstructure:"preemptible" required:"false"`
// If set, the instance will be automatically terminated after the specified amount of seconds.
// instance_termination_action must be set if this property is set.
InstanceMaxRunDurationSeconds int64 `mapstructure:"instance_max_run_duration_seconds" required:"false"`
// The termination action for this VM, which can be either STOP or DELETE.
InstanceTerminationAction string `mapstructure:"instance_termination_action" required:"false"`
// Sets a node affinity label for the launched instance (eg. for sole tenancy).
// Please see [Provisioning VMs on
// sole-tenant nodes](https://cloud.google.com/compute/docs/nodes/provisioning-sole-tenant-vms)
Expand Down Expand Up @@ -632,6 +637,18 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) {
c.WindowsPasswordTimeout = 3 * time.Minute
}

if c.InstanceMaxRunDurationSeconds < 0 {

@nywilken nywilken Dec 14, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My suggestion here would be to include a high default value and possibly validate the the provided value is not lower than the default timeouts set for SSH or other Packer processes. To avoid terminating the machine before establishing a connection. It might not be so straightforward to know all the timeouts so I would suggest documenting it as well. Within the configuration option that at minimum the value should be set to X number of seconds. Possibly even hours or days.

I’ll leave this question for @lbajolet-hashicorp Should this be an unsigned int?

@phoenixuprising phoenixuprising Dec 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It seems that if it is not straightforward to know all the timeouts, setting a high default without checking the other config values would be the best path forward to me. I could set the default to something like 2 days and add documentation to the config values. I also like the idea of setting the default termination option to STOP so I'll add that.

Should this be an unsigned int?

Probably. I don't believe that maxRunDuration can be negative.

@lbajolet-hashicorp lbajolet-hashicorp Feb 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah regarding the timeout situation I tend to agree with @phoenixuprising, it will be tricky to infer a good value here, especially since things can be set from other attributes, and having something that is coherent and long-term viable is troublesome.
I do agree that this could be a uint, but I assume the rationale for it being an int64 is because it is the type expected by the SDK/API? I don't mind this being either personally, but if we're dealing with a signed number, I would suggest adding a sanity check during Prepare to make sure it is always positive, as is the case in the current state of the code.
Note: an argument could still be made in favour of a uint here, as then the type explicitly prevents negative values, so later changes in the codebase bear less risk of unintentional breakage, ultimately the choice is yours @phoenixuprising.

I like the idea of having a high enough default value, you suggest 2 days, I would even go lower and suggest something like 1 day, I would think this is reasonable enough for the vast majority of our users, and will fix that problem automagically for everyone.
Regarding the default termination option, I wonder if STOP is the right default here though, my read on this is that we want to reduce cost if a build gets stuck for too long, therefore I would maybe go with DELETE as the default option, so the instance is wiped if this happens, as to not incur extra cost.

@lbajolet-hashicorp lbajolet-hashicorp Feb 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note: if we pick a non-zero default value here, we should probably amend the documentation for the attribute so the default value is clearly stated, along with a way to disable that behaviour.

errs = packersdk.MultiErrorAppend(fmt.Errorf("'instance_max_run_duration_seconds' must be positive"))
}

if c.InstanceMaxRunDurationSeconds > 0 && c.InstanceTerminationAction == "" {
errs = packersdk.MultiErrorAppend(fmt.Errorf("'instance_termination_action' must be set if 'instance_max_run_duration_seconds' is set"))
}

if c.InstanceTerminationAction != "" && !(c.InstanceTerminationAction == "STOP" || c.InstanceTerminationAction == "DELETE") {

@nywilken nywilken Dec 14, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might be worth adding constants for these values.

that aside would it make sense to defaulting to STOP after x days.

errs = packersdk.MultiErrorAppend(fmt.Errorf("'instance_termination_action' must be 'STOP' or 'DELETE'"))
}

return warnings, errs
}

Expand Down
488 changes: 246 additions & 242 deletions builder/googlecompute/config.hcl2spec.go
View file
Open in desktop

Large diffs are not rendered by default.

66 changes: 66 additions & 0 deletions builder/googlecompute/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,72 @@ func TestConfigPrepareImageArchitecture(t *testing.T) {
}
}

func TestConfigInstanceMaxRunDurationSeconds(t *testing.T) {
cases := []struct {
Keys []string
Values []interface{}
Err bool

@lbajolet-hashicorp lbajolet-hashicorp Feb 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very nitpicky but in terms of self-documentation, it would imho be better if this field was called ExpectErr instead of just Err to convey the intent that this is the expected outcome of the test here.

}{
{
[]string{"instance_max_run_duration_seconds", "instance_termination_action"},
[]interface{}{5, "STOP"},
false,
},
{
[]string{"instance_max_run_duration_seconds", "instance_termination_action"},
[]interface{}{5, "DELETE"},
false,
},
{
[]string{"instance_max_run_duration_seconds"},
[]interface{}{1},
true,
},
{
[]string{"instance_max_run_duration_seconds"},
[]interface{}{-1},
false,
},
{
[]string{"instance_max_run_duration_seconds", "instance_termination_action"},
[]interface{}{5, "BAD_VALUE"},
true,
},
{
[]string{"instance_max_run_duration_seconds", "instance_termination_action"},
[]interface{}{5, ""},
true,
},
}

for _, tc := range cases {
raw, tempfile := testConfig(t)
defer os.Remove(tempfile)

errStr := ""
for k := range tc.Keys {

// Create the string for error reporting
// convert value to string if it can be converted
errStr += fmt.Sprintf("%s:%v, ", tc.Keys[k], tc.Values[k])
if tc.Values[k] == nil {
delete(raw, tc.Keys[k])
} else {
raw[tc.Keys[k]] = tc.Values[k]
}
}

var c Config
warns, errs := c.Prepare(raw)

if tc.Err {
testConfigErr(t, warns, errs, strings.TrimRight(errStr, ", "))
} else {
testConfigOk(t, warns, errs)
}
}
}

// Helper stuff below

func testConfig(t *testing.T) (config map[string]interface{}, tempAccountFile string) {
Expand Down
2 changes: 2 additions & 0 deletions builder/googlecompute/step_create_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ func (s *StepCreateInstance) Run(ctx context.Context, state multistep.StateBag)
OmitExternalIP: c.OmitExternalIP,
OnHostMaintenance: c.OnHostMaintenance,
Preemptible: c.Preemptible,
MaxRunDurationSeconds: c.InstanceMaxRunDurationSeconds,
TerminationAction: c.InstanceTerminationAction,
NodeAffinities: c.NodeAffinities,
Region: c.Region,
ServiceAccountEmail: c.ServiceAccountEmail,
Expand Down
5 changes: 5 additions & 0 deletions docs-partials/builder/googlecompute/Config-not-required.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,11 @@

- `preemptible` (bool) - If true, launch a preemptible instance.

- `instance_max_run_duration_seconds` (int64) - If set, the instance will be automatically terminated after the specified amount of seconds.
instance_termination_action must be set if this property is set.

- `instance_termination_action` (string) - The termination action for this VM, which can be either STOP or DELETE.

- `node_affinity` ([]common.NodeAffinity) - Sets a node affinity label for the launched instance (eg. for sole tenancy).
Please see [Provisioning VMs on
sole-tenant nodes](https://cloud.google.com/compute/docs/nodes/provisioning-sole-tenant-vms)
Expand Down
60 changes: 33 additions & 27 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,25 @@ module github.com/hashicorp/packer-plugin-googlecompute
go 1.21.0

require (
cloud.google.com/go/compute/metadata v0.2.3
cloud.google.com/go/compute/metadata v0.5.2
github.com/gofrs/uuid v4.0.0+incompatible
github.com/google/go-cmp v0.6.0
github.com/hashicorp/hcl/v2 v2.19.1
github.com/hashicorp/packer-plugin-sdk v0.5.4
github.com/hashicorp/vault/api v1.14.0
github.com/mitchellh/mapstructure v1.5.0
github.com/stretchr/testify v1.8.4
github.com/stretchr/testify v1.9.0
github.com/zclconf/go-cty v1.13.3
golang.org/x/oauth2 v0.13.0
google.golang.org/api v0.150.0
golang.org/x/oauth2 v0.24.0
google.golang.org/api v0.211.0
)

require (
cloud.google.com/go v0.110.8 // indirect
cloud.google.com/go/compute v1.23.1 // indirect
cloud.google.com/go/iam v1.1.3 // indirect
cloud.google.com/go/storage v1.35.1 // indirect
cloud.google.com/go v0.112.2 // indirect
cloud.google.com/go/auth v0.12.1 // indirect
cloud.google.com/go/auth/oauth2adapt v0.2.6 // indirect
cloud.google.com/go/iam v1.1.6 // indirect
cloud.google.com/go/storage v1.39.1 // indirect
github.com/Azure/go-ntlmssp v0.0.0-20200615164410-66371956d46c // indirect
github.com/ChrisTrenkamp/goxpath v0.0.0-20210404020558-97928f7e12b6 // indirect
github.com/agext/levenshtein v1.2.3 // indirect
Expand All @@ -33,14 +34,16 @@ require (
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
github.com/dylanmei/iso8601 v0.1.0 // indirect
github.com/fatih/color v1.16.0 // indirect
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/go-jose/go-jose/v4 v4.0.1 // indirect
github.com/go-logr/logr v1.4.2 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
github.com/gofrs/flock v0.8.1 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/golang/protobuf v1.5.3 // indirect
github.com/google/s2a-go v0.1.7 // indirect
github.com/google/uuid v1.4.0 // indirect
github.com/googleapis/enterprise-certificate-proxy v0.3.2 // indirect
github.com/googleapis/gax-go/v2 v2.12.0 // indirect
github.com/google/s2a-go v0.1.8 // indirect
github.com/google/uuid v1.6.0 // indirect
github.com/googleapis/enterprise-certificate-proxy v0.3.4 // indirect
github.com/googleapis/gax-go/v2 v2.14.0 // indirect
github.com/hashicorp/consul/api v1.25.1 // indirect
github.com/hashicorp/errwrap v1.1.0 // indirect
github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
Expand Down Expand Up @@ -83,21 +86,24 @@ require (
github.com/ugorji/go/codec v1.2.6 // indirect
github.com/ulikunitz/xz v0.5.10 // indirect
go.opencensus.io v0.24.0 // indirect
golang.org/x/crypto v0.23.0 // indirect
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.54.0 // indirect
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.54.0 // indirect
go.opentelemetry.io/otel v1.29.0 // indirect
go.opentelemetry.io/otel/metric v1.29.0 // indirect
go.opentelemetry.io/otel/trace v1.29.0 // indirect
golang.org/x/crypto v0.30.0 // indirect
golang.org/x/exp v0.0.0-20230321023759-10a507213a29 // indirect
golang.org/x/net v0.25.0 // indirect
golang.org/x/sync v0.5.0 // indirect
golang.org/x/sys v0.20.0 // indirect
golang.org/x/term v0.20.0 // indirect
golang.org/x/text v0.15.0 // indirect
golang.org/x/time v0.3.0 // indirect
golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect
google.golang.org/appengine v1.6.7 // indirect
google.golang.org/genproto v0.0.0-20231016165738-49dd2c1f3d0b // indirect
google.golang.org/genproto/googleapis/api v0.0.0-20231016165738-49dd2c1f3d0b // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20231030173426-d783a09b4405 // indirect
google.golang.org/grpc v1.59.0 // indirect
google.golang.org/protobuf v1.33.0 // indirect
golang.org/x/net v0.32.0 // indirect
golang.org/x/sync v0.10.0 // indirect
golang.org/x/sys v0.28.0 // indirect
golang.org/x/term v0.27.0 // indirect
golang.org/x/text v0.21.0 // indirect
golang.org/x/time v0.8.0 // indirect
google.golang.org/genproto v0.0.0-20240213162025-012b6fc9bca9 // indirect
google.golang.org/genproto/googleapis/api v0.0.0-20241118233622-e639e219e697 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20241206012308-a4fef0638583 // indirect
google.golang.org/grpc v1.67.1 // indirect
google.golang.org/protobuf v1.35.2 // indirect
gopkg.in/yaml.v2 v2.3.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)
Expand Down
Loading