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

command: Make provider installation interruptible #26405

Merged
merged 2 commits into from Sep 29, 2020

Conversation

apparentlymart
Copy link
Member

@apparentlymart apparentlymart commented Sep 29, 2020

In earlier commits we started to make the installation codepath context-aware so that it could be canceled in the event of a SIGINT, but we didn't complete wiring that through the API of the getproviders package.

Here we make the getproviders.Source interface methods, along with some other functions that can make network requests, take a context.Context argument and act appropriately if that context is canceled.

The main providercache.Installer.EnsureProviderVersions method now also has some context-awareness so that it can abort its work early if its context reports any sort of error. That avoids waiting for the process to wind through all of the remaining iterations of the various loops, logging each request failure separately, and instead returns just a single aggregate "canceled" error.

We can then set things up in the terraform init and terraform providers mirror commands so that the context will be
cancelled if we get an interrupt signal, allowing provider installation to abort early while still atomically completing any local side-effects that may have started.

This fixes #18474, and partially addresses #23647 (because the other operations in terraform init are still not interruptible yet.)


This PR also bundles in the removal of the terraform 0.13upgrade command, which was due to happen before Terraform 0.14 anyway, primarily because it includes calls into the getproviders package that I would otherwise have need to update to pass in a context. Reviewing the two commits separately might be easier.

We only preserve these major upgrade versions for one major version after
they are added, because our upgrade path assumes moving forward only one
major version at a time. Now that our main branch is tracking towards
Terraform 0.14, we no longer need the 0.13upgrade subcommand.

This also includes some minor adjustments to the 0.12upgrade command to
align the terminology used in the output of both commands. We usually
use the word "deprecated" to mean that something is still available but
no longer recommended, but neither of these commands is actually available
so "removed" is clearer.

We could in principle have removed even the removal notice for 0.12upgrade
here, but it's relatively little code and not a big deal to keep around
to help prompt those who might try to upgrade directly from 0.11 to 0.14.
We may still remove the historical configuration upgrade commands prior to
releasing Terraform 1.0, though.
In earlier commits we started to make the installation codepath
context-aware so that it could be canceled in the event of a SIGINT, but
we didn't complete wiring that through the API of the getproviders
package.

Here we make the getproviders.Source interface methods, along with some
other functions that can make network requests, take a context.Context
argument and act appropriately if that context is cancelled.

The main providercache.Installer.EnsureProviderVersions method now also
has some context-awareness so that it can abort its work early if its
context reports any sort of error. That avoids waiting for the process
to wind through all of the remaining iterations of the various loops,
logging each request failure separately, and instead returns just
a single aggregate "canceled" error.

We can then set things up in the "terraform init" and
"terraform providers mirror" commands so that the context will be
cancelled if we get an interrupt signal, allowing provider installation
to abort early while still atomically completing any local-side effects
that may have started.
@codecov
Copy link

codecov bot commented Sep 29, 2020

Codecov Report

Merging #26405 into master will decrease coverage by 0.12%.
The diff coverage is 64.78%.

Impacted Files Coverage Δ
command/providers_mirror.go 0.00% <0.00%> (ø)
internal/getproviders/errors.go 35.59% <0.00%> (-0.62%) ⬇️
internal/getproviders/filesystem_mirror_source.go 66.66% <ø> (ø)
internal/getproviders/mock_source.go 52.23% <ø> (ø)
internal/providercache/package_install.go 20.25% <0.00%> (-0.53%) ⬇️
internal/providercache/installer.go 27.83% <33.33%> (-0.54%) ⬇️
command/013_config_upgrade.go 38.46% <50.00%> (-31.21%) ⬇️
internal/getproviders/registry_client.go 63.42% <57.14%> (-0.06%) ⬇️
internal/getproviders/http_mirror_source.go 68.33% <60.00%> (-0.60%) ⬇️
command/012_config_upgrade.go 38.46% <75.00%> (ø)
... and 8 more

@mildwonkey
Copy link
Contributor

Screen Shot 2020-09-29 at 9 16 54 AM

Love it!

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

I especially like the custom messages you've put in for the 0.12 and 0.13 commands (specifying that one upgrades syntax and the other provider requirements)!

@apparentlymart apparentlymart merged commit 0b734a2 into master Sep 29, 2020
@apparentlymart apparentlymart deleted the f-init-interrupt-providers branch September 29, 2020 17:00
@ghost
Copy link

ghost commented Oct 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@hashicorp hashicorp locked as resolved and limited conversation to collaborators Oct 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform does not respect SIGINT when downloading providers
2 participants