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

Add instance_max_run_duration_seconds config #242

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

Conversation

phoenixuprising
Copy link

This pr adds a new instance_max_run_duration_seconds config field
that gets piped down into the builder instance's maxRunDuration
schedule. This will allow for an instance to automatically be stopped
or deleted, even if the packer build process is killed.

It also adds an instance_termination_action which must be set if
instance_max_run_duration_seconds config is set. This will tell GCP
to either STOP or DELETE the instance once the max duration has
been reached.

This pr adds a new `instance_max_run_duration_seconds` config field
that gets piped down into the builder instance's `maxRunDuration`
schedule. This will allow for an instance to automatically be stopped
or deleted, even if the packer build process is killed.

It also adds an `instance_termination_action` which must be set if
`instance_max_run_duration_seconds` config is set. This will tell GCP
to either `STOP` or `DELETE` the instance once the max duration has
been reached.
@phoenixuprising phoenixuprising requested a review from a team as a code owner December 11, 2024 05:53
Copy link

hashicorp-cla-app bot commented Dec 11, 2024

CLA assistant check
All committers have signed the CLA.

@phoenixuprising
Copy link
Author

@nywilken - I saw you commenting on another PR. Is this something you can look at or pass off to someone that can?

My team is running packer in a github action and people will cancel the action sometimes while it is running, which doesn't give packer the opportunity to clean up instances correctly.

@nywilken
Copy link
Contributor

@nywilken - I saw you commenting on another PR. Is this something you can look at or pass off to someone that can?

My team is running packer in a github action and people will cancel the action sometimes while it is running, which doesn't give packer the opportunity to clean up instances correctly.

@phoenixuprising I’m no longer working on this project. But I’m cc’ing @lbajolet-hashicorp to help get eyes on the pull-request. Please be mindful that the team’s priority might be elsewhere at the moment. So he made need time to review and respond.

@@ -632,6 +637,18 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) {
c.WindowsPasswordTimeout = 3 * time.Minute
}

if c.InstanceMaxRunDurationSeconds < 0 {
Copy link
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?

Copy link
Author

@phoenixuprising phoenixuprising Dec 17, 2024

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.

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") {
Copy link
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.

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

This is a great idea and is a nice feature add. I left a few comments but Im no longer on the team so Packeristas may have other comments.

@phoenixuprising
Copy link
Author

@lbajolet-hashicorp - Just wanted to check in and see if you could look over this PR. Thanks.

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