From 88beca90abd1fe1f0d65713bb9b71daf1232f6a4 Mon Sep 17 00:00:00 2001 From: Din Music Date: Mon, 11 Dec 2023 17:10:32 +0100 Subject: [PATCH 1/4] internal/resource: Remove initial delay when waiting for a state Signed-off-by: Din Music --- internal/instance/resource_instance.go | 64 +++++++++++--------------- 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/internal/instance/resource_instance.go b/internal/instance/resource_instance.go index 77a96bd..72b11bf 100644 --- a/internal/instance/resource_instance.go +++ b/internal/instance/resource_instance.go @@ -489,7 +489,7 @@ func (r InstanceResource) Create(ctx context.Context, req resource.CreateRequest if plan.Running.ValueBool() { // Start the instance. - diag := startInstance(ctx, server, instance.Name, r.provider.RefreshInterval()) + diag := startInstance(ctx, server, instance.Name) if diag != nil { resp.Diagnostics.Append(diag) return @@ -498,7 +498,7 @@ func (r InstanceResource) Create(ctx context.Context, req resource.CreateRequest // Wait for the instance to obtain an IP address if network // availability is requested by the user. if plan.WaitForNetwork.ValueBool() { - diag := waitInstanceNetwork(ctx, server, instance.Name, r.provider.RefreshInterval()) + diag := waitInstanceNetwork(ctx, server, instance.Name) if diag != nil { resp.Diagnostics.Append(diag) return @@ -573,7 +573,7 @@ func (r InstanceResource) Update(ctx context.Context, req resource.UpdateRequest // First ensure the desired state of the instance (stopped/running). // This ensures we fail fast if instance runs into an issue. if plan.Running.ValueBool() && !isInstanceOperational(*instanceState) { - diag := startInstance(ctx, server, instanceName, r.provider.RefreshInterval()) + diag := startInstance(ctx, server, instanceName) if diag != nil { resp.Diagnostics.Append(diag) return @@ -582,7 +582,7 @@ func (r InstanceResource) Update(ctx context.Context, req resource.UpdateRequest // If instance is freshly started, we should also wait for // network (if user requested that). if plan.WaitForNetwork.ValueBool() { - diag := waitInstanceNetwork(ctx, server, instanceName, r.provider.RefreshInterval()) + diag := waitInstanceNetwork(ctx, server, instanceName) if diag != nil { resp.Diagnostics.Append(diag) return @@ -590,7 +590,7 @@ func (r InstanceResource) Update(ctx context.Context, req resource.UpdateRequest } } else if !plan.Running.ValueBool() && !isInstanceStopped(*instanceState) { // Stop the instance gracefully. - _, diag := stopInstance(ctx, server, instanceName, r.provider.RefreshInterval(), false) + _, diag := stopInstance(ctx, server, instanceName, false) if diag != nil { resp.Diagnostics.Append(diag) return @@ -719,7 +719,7 @@ func (r InstanceResource) Delete(ctx context.Context, req resource.DeleteRequest instanceName := state.Name.ValueString() // Force stop the instance, because we are deleting it anyway. - isFound, diag := stopInstance(ctx, server, instanceName, r.provider.RefreshInterval(), true) + isFound, diag := stopInstance(ctx, server, instanceName, true) if diag != nil { // Ephemeral instances will be removed when stopped. if !isFound { @@ -922,7 +922,7 @@ func ToProfileListType(ctx context.Context, profiles []string) (types.List, diag // startInstance starts an instance with the given name. It also waits // for it to become fully operational. -func startInstance(ctx context.Context, server lxd.InstanceServer, instanceName string, refInterval time.Duration) diag.Diagnostic { +func startInstance(ctx context.Context, server lxd.InstanceServer, instanceName string) diag.Diagnostic { st, etag, err := server.GetInstanceState(instanceName) if err != nil { return diag.NewErrorDiagnostic(fmt.Sprintf("Failed to retrieve state of instance %q", instanceName), err.Error()) @@ -966,15 +966,7 @@ func startInstance(ctx context.Context, server lxd.InstanceServer, instanceName // Even though op.Wait has completed, wait until we can see // the instance is fully started via a new API call. - stateConf := &retry.StateChangeConf{ - Target: []string{api.Running.String()}, - Timeout: 3 * time.Minute, - MinTimeout: 3 * time.Second, - Delay: refInterval, - Refresh: instanceStartedCheck, - } - - _, err = stateConf.WaitForStateContext(ctx) + _, err = waitForState(ctx, instanceStartedCheck, api.Running.String()) if err != nil { return diag.NewErrorDiagnostic(fmt.Sprintf("Failed to wait for instance %q to start", instanceName), err.Error()) } @@ -986,7 +978,7 @@ func startInstance(ctx context.Context, server lxd.InstanceServer, instanceName // status to become Stopped or the instance to be removed (not found) in // case of an ephemeral instance. In the latter case, false is returned // along an error. -func stopInstance(ctx context.Context, server lxd.InstanceServer, instanceName string, refInterval time.Duration, force bool) (bool, diag.Diagnostic) { +func stopInstance(ctx context.Context, server lxd.InstanceServer, instanceName string, force bool) (bool, diag.Diagnostic) { st, etag, err := server.GetInstanceState(instanceName) if err != nil { return true, diag.NewErrorDiagnostic(fmt.Sprintf("Failed to retrieve state of instance %q", instanceName), err.Error()) @@ -1022,17 +1014,9 @@ func stopInstance(ctx context.Context, server lxd.InstanceServer, instanceName s return st, st.Status, nil } - stateConf := &retry.StateChangeConf{ - Target: []string{api.Stopped.String()}, - Timeout: 3 * time.Minute, - MinTimeout: 3 * time.Second, - Delay: refInterval, - Refresh: instanceStoppedCheck, - } - // Even though op.Wait has completed, wait until we can see // the instance is stopped via a new API call. - _, err = stateConf.WaitForStateContext(ctx) + _, err = waitForState(ctx, instanceStoppedCheck, api.Stopped.String()) if err != nil { found := !errors.IsNotFoundError(err) return found, diag.NewErrorDiagnostic(fmt.Sprintf("Failed to wait for instance %q to stop", instanceName), err.Error()) @@ -1044,7 +1028,7 @@ func stopInstance(ctx context.Context, server lxd.InstanceServer, instanceName s // waitInstanceNetwork waits for an instance with the given name to receive // an IPv4 address on any interface (excluding loopback). This should be // called only if the instance is running. -func waitInstanceNetwork(ctx context.Context, server lxd.InstanceServer, instanceName string, refInterval time.Duration) diag.Diagnostic { +func waitInstanceNetwork(ctx context.Context, server lxd.InstanceServer, instanceName string) diag.Diagnostic { // instanceNetworkCheck function checks whether instance has // received an IP address. instanceNetworkCheck := func() (any, string, error) { @@ -1068,17 +1052,7 @@ func waitInstanceNetwork(ctx context.Context, server lxd.InstanceServer, instanc return st, "Waiting for network", nil } - // LXD will return "Running" even if "inet" has not yet - // been set. Therefore, wait until we see an "inet" IP. - networkConf := &retry.StateChangeConf{ - Target: []string{"OK"}, - Timeout: 3 * time.Minute, - MinTimeout: 3 * time.Second, - Delay: refInterval, - Refresh: instanceNetworkCheck, - } - - _, err := networkConf.WaitForStateContext(ctx) + _, err := waitForState(ctx, instanceNetworkCheck, "OK") if err != nil { return diag.NewErrorDiagnostic(fmt.Sprintf("Failed to wait for instance %q to get an IP address", instanceName), err.Error()) } @@ -1086,6 +1060,20 @@ func waitInstanceNetwork(ctx context.Context, server lxd.InstanceServer, instanc return nil } +// waitForState waits until the provided function reports one of the target +// states. It returns either the resulting state or an error. +func waitForState(ctx context.Context, refreshFunc retry.StateRefreshFunc, targets ...string) (any, error) { + stateRefreshConf := &retry.StateChangeConf{ + Refresh: refreshFunc, + Target: targets, + Timeout: 3 * time.Minute, + MinTimeout: 2 * time.Second, // Timeout increases: 2, 4, 8, 10, 10, ... + Delay: 2 * time.Second, // Delay before the first check/refresh. + } + + return stateRefreshConf.WaitForStateContext(ctx) +} + // isInstanceOperational determines if an instance is fully operational based // on its state. It returns true if the instance is running and the reported // process count is positive. Checking for a positive process count is esential From 7fab75f2fef703cfaa63da275d8e5281cf0eccda Mon Sep 17 00:00:00 2001 From: Din Music Date: Mon, 11 Dec 2023 17:16:59 +0100 Subject: [PATCH 2/4] internal/provider*: Remove refresh_interval attribute Signed-off-by: Din Music --- internal/provider-config/config.go | 14 +----------- internal/provider/provider.go | 34 ++++-------------------------- main.go | 4 ++-- 3 files changed, 7 insertions(+), 45 deletions(-) diff --git a/internal/provider-config/config.go b/internal/provider-config/config.go index 88698b0..137b484 100644 --- a/internal/provider-config/config.go +++ b/internal/provider-config/config.go @@ -6,7 +6,6 @@ import ( "os" "path/filepath" "sync" - "time" lxd "github.com/canonical/lxd/client" lxd_config "github.com/canonical/lxd/lxc/config" @@ -39,10 +38,6 @@ type LxdProviderConfig struct { // should be accepted. acceptServerCertificate bool - // refreshInterval is a custom interval for communicating with remote - // LXD servers. - refreshInterval time.Duration - // LXDConfig is the converted form of terraformLXDConfig // in LXD's native data structure. This is lazy-loaded / created // only when a connection to an LXD remote/server happens. @@ -69,10 +64,9 @@ type LxdProviderConfig struct { // NewLxdProvider returns initialized LXD provider structure. This struct is // used to store information about this Terraform provider's configuration for // reference throughout the lifecycle. -func NewLxdProvider(lxdConfig *lxd_config.Config, refreshInterval time.Duration, acceptServerCert bool) *LxdProviderConfig { +func NewLxdProvider(lxdConfig *lxd_config.Config, acceptServerCert bool) *LxdProviderConfig { return &LxdProviderConfig{ acceptServerCertificate: acceptServerCert, - refreshInterval: refreshInterval, lxdConfig: lxdConfig, remotes: make(map[string]LxdProviderRemoteConfig), servers: make(map[string]lxd.Server), @@ -496,9 +490,3 @@ func (p *LxdProviderConfig) getLxdConfigImageServer(remoteName string) (lxd.Imag defer p.mux.RUnlock() return p.lxdConfig.GetImageServer(remoteName) } - -// RefreshInterval returns a time interval on which provider should -// retry to communicate with the LXD server. -func (p *LxdProviderConfig) RefreshInterval() time.Duration { - return p.refreshInterval -} diff --git a/internal/provider/provider.go b/internal/provider/provider.go index 4d67aa2..3e86a1f 100644 --- a/internal/provider/provider.go +++ b/internal/provider/provider.go @@ -5,13 +5,11 @@ import ( "log" "os" "path/filepath" - "time" lxd_config "github.com/canonical/lxd/lxc/config" lxd_shared "github.com/canonical/lxd/shared" "github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator" "github.com/hashicorp/terraform-plugin-framework/datasource" - "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/provider" "github.com/hashicorp/terraform-plugin-framework/provider/schema" "github.com/hashicorp/terraform-plugin-framework/resource" @@ -41,27 +39,20 @@ type LxdProviderModel struct { Remotes []LxdProviderRemoteModel `tfsdk:"remote"` ConfigDir types.String `tfsdk:"config_dir"` Project types.String `tfsdk:"project"` - RefreshInterval types.String `tfsdk:"refresh_interval"` AcceptRemoteCertificate types.Bool `tfsdk:"accept_remote_certificate"` GenerateClientCertificates types.Bool `tfsdk:"generate_client_certificates"` } // LxdProvider ... type LxdProvider struct { - version string - refreshInterval string + version string } // New returns LXD provider with the given version set. -func NewLxdProvider(version string, refreshInterval string) func() provider.Provider { - if refreshInterval == "" { - refreshInterval = "10s" - } - +func NewLxdProvider(version string) func() provider.Provider { return func() provider.Provider { return &LxdProvider{ - version: version, - refreshInterval: refreshInterval, + version: version, } } } @@ -89,11 +80,6 @@ func (p *LxdProvider) Schema(_ context.Context, _ provider.SchemaRequest, resp * Description: "Accept the server certificate.", }, - "refresh_interval": schema.StringAttribute{ - Optional: true, - Description: "How often to poll during state changes. (default = 10s)", - }, - "project": schema.StringAttribute{ Optional: true, Description: "The project where project-scoped resources will be created. Can be overridden in individual resources. (default = default)", @@ -176,18 +162,6 @@ func (p *LxdProvider) Configure(ctx context.Context, req provider.ConfigureReque log.Printf("[DEBUG] LXD Config: %#v", config) - // Determine custom refresh interval. Default is 10 seconds. - refreshIntervalString := data.RefreshInterval.ValueString() - if refreshIntervalString == "" { - refreshIntervalString = p.refreshInterval - } - - refreshInterval, err := time.ParseDuration(refreshIntervalString) - if err != nil { - resp.Diagnostics.AddAttributeError(path.Root("refresh_interval"), "Invalid refresh interval", err.Error()) - return - } - // Determine if the LXD server's SSL certificates should be // accepted. If this is set to false and if the remote's // certificates haven't already been accepted, the user will @@ -227,7 +201,7 @@ func (p *LxdProvider) Configure(ctx context.Context, req provider.ConfigureReque // Initialize global LxdProvider struct. // This struct is used to store information about this Terraform // provider's configuration for reference throughout the lifecycle. - lxdProvider := provider_config.NewLxdProvider(config, refreshInterval, acceptServerCertificate) + lxdProvider := provider_config.NewLxdProvider(config, acceptServerCertificate) // Create LXD remote from environment variables (if defined). // This emulates the Terraform provider "remote" config: diff --git a/main.go b/main.go index 6717626..0f0fa20 100644 --- a/main.go +++ b/main.go @@ -10,7 +10,7 @@ import ( ) // version indicates provider's version. The appropriate value -// for the compile binary will be set by the goreleaser. +// for the compiled binary will be set by the goreleaser. // See: https://goreleaser.com/cookbooks/using-main.version/ var version = "dev" @@ -34,7 +34,7 @@ func main() { ProtocolVersion: 6, } - err := providerserver.Serve(context.Background(), provider.NewLxdProvider(version, "10s"), opts) + err := providerserver.Serve(context.Background(), provider.NewLxdProvider(version), opts) if err != nil { log.Fatal(err.Error()) } From 046af22a70695edce2f0105421d596d12dc26d3d Mon Sep 17 00:00:00 2001 From: Din Music Date: Mon, 11 Dec 2023 17:19:59 +0100 Subject: [PATCH 3/4] internal/acctest: Remove refresh_interval from test provider --- internal/acctest/provider_factory.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/acctest/provider_factory.go b/internal/acctest/provider_factory.go index 04aeb6b..7264c83 100644 --- a/internal/acctest/provider_factory.go +++ b/internal/acctest/provider_factory.go @@ -2,7 +2,6 @@ package acctest import ( "sync" - "time" lxd_config "github.com/canonical/lxd/lxc/config" "github.com/hashicorp/terraform-plugin-framework/providerserver" @@ -29,9 +28,8 @@ func testProvider() *provider_config.LxdProviderConfig { if testProviderConfig == nil { config := lxd_config.DefaultConfig() - refreshInterval := time.Duration(2 * time.Second) acceptClientCert := true - testProviderConfig = provider_config.NewLxdProvider(config, refreshInterval, acceptClientCert) + testProviderConfig = provider_config.NewLxdProvider(config, acceptClientCert) } return testProviderConfig @@ -42,5 +40,5 @@ func testProvider() *provider_config.LxdProviderConfig { // CLI command executed to create a provider server to which the CLI can // reattach. var ProtoV6ProviderFactories = map[string]func() (tfprotov6.ProviderServer, error){ - "lxd": providerserver.NewProtocol6WithError(provider.NewLxdProvider("test", "2s")()), + "lxd": providerserver.NewProtocol6WithError(provider.NewLxdProvider("test")()), } From 9434a9d1bef57239a907164b848d8d9209991f5a Mon Sep 17 00:00:00 2001 From: Din Music Date: Mon, 11 Dec 2023 17:20:51 +0100 Subject: [PATCH 4/4] docs: Remove docs for refresh_interval Signed-off-by: Din Music --- docs/index.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docs/index.md b/docs/index.md index 56c503f..2325f92 100644 --- a/docs/index.md +++ b/docs/index.md @@ -75,10 +75,6 @@ The following arguments are supported: also be set with the `LXD_ACCEPT_SERVER_CERTIFICATE` environment variable. Defaults to `false` -* `refresh_interval` - *Optional* - How often to poll during state change. - Defaults to "10s", or 10 seconds. Valid values are a Go-style parsable time - duration (`10s`, `1m`, `5h`). - The `remote` block supports: * `address` - *Optional* - The address of the LXD remote.