From 886f3914258a9d1762901ae767e09080378b26b6 Mon Sep 17 00:00:00 2001 From: Lucas Bremgartner Date: Thu, 21 Nov 2024 07:11:23 +0100 Subject: [PATCH 1/5] chore: formatting Signed-off-by: Lucas Bremgartner --- docs/resources/instance.md | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/docs/resources/instance.md b/docs/resources/instance.md index b537386..4a3008c 100644 --- a/docs/resources/instance.md +++ b/docs/resources/instance.md @@ -110,7 +110,7 @@ resource "incus_instance" "instance2" { If `running` is set to false or instance is already running (on update), this value has no effect. Defaults to `true`. * `profiles` - *Optional* - List of Incus config profiles to apply to the new - instance. Profile `default` will be applied if profiles are not set (are `null`). + instance. Profile `default` will be applied if profiles are not set (are `null`). However, if an empty array (`[]`) is set as a value, no profiles will be applied. * `device` - *Optional* - Device definition. See reference below. @@ -118,12 +118,12 @@ resource "incus_instance" "instance2" { * `file` - *Optional* - File to upload to the instance. See reference below. * `config` - *Optional* - Map of key/value pairs of - [instance config settings](https://linuxcontainers.org/incus/docs/main/reference/instance_options/). + [instance config settings](https://linuxcontainers.org/incus/docs/main/reference/instance_options/). * `project` - *Optional* - Name of the project where the instance will be spawned. * `remote` - *Optional* - The remote in which the resource will be created. If - not provided, the provider's default remote will be used. + not provided, the provider's default remote will be used. * `target` - *Optional* - Specify a target node in a cluster. @@ -140,21 +140,21 @@ The `device` block supports: * `name` - **Required** - Name of the device. * `type` - **Required** - Type of the device Must be one of none, disk, nic, - unix-char, unix-block, usb, gpu, infiniband, proxy, unix-hotplug, tpm, pci. + unix-char, unix-block, usb, gpu, infiniband, proxy, unix-hotplug, tpm, pci. * `properties`- **Required** - Map of key/value pairs of - [device properties](https://linuxcontainers.org/incus/docs/main/reference/devices/). + [device properties](https://linuxcontainers.org/incus/docs/main/reference/devices/). The `file` block supports: -* `content` - *__Required__ unless source_path is used* - The _contents_ of the file. - Use the `file()` function to read in the content of a file from disk. +* `content` - **Required** unless source_path is used* - The _contents_ of the file. + Use the `file()` function to read in the content of a file from disk. -* `source_path` - *__Required__ unless content is used* - The source path to a file to - copy to the instance. +* `source_path` - **Required** unless content is used* - The source path to a file to + copy to the instance. * `target_path` - **Required** - The absolute path of the file on the instance, - including the filename. + including the filename. * `uid` - *Optional* - The UID of the file. Must be an unquoted integer. @@ -163,7 +163,7 @@ The `file` block supports: * `mode` - *Optional* - The octal permissions of the file, must be quoted. Defaults to `0755`. * `create_directories` - *Optional* - Whether to create the directories leading - to the target if they do not exist. + to the target if they do not exist. ## Attribute Reference @@ -240,5 +240,5 @@ import { * The instance resource `config` includes some keys that can be automatically generated by the Incus. If these keys are not explicitly defined by the user, they will be omitted from the Terraform state and treated as computed values. - - `image.*` - - `volatile.*` + * `image.*` + * `volatile.*` From a886be0e74ab5b225b3dcbb959192cf007813ddd Mon Sep 17 00:00:00 2001 From: Lucas Bremgartner Date: Thu, 21 Nov 2024 07:16:48 +0100 Subject: [PATCH 2/5] docs: Add source_file to incus_instance Signed-off-by: Lucas Bremgartner --- docs/resources/instance.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/docs/resources/instance.md b/docs/resources/instance.md index 4a3008c..cc9585d 100644 --- a/docs/resources/instance.md +++ b/docs/resources/instance.md @@ -89,6 +89,39 @@ resource "incus_instance" "instance2" { } ``` +## Example to create a new instance from an instance backup + +```hcl +resource "incus_instance" "instance1" { + project = "default" + name = "instance1" + source_file = "/path/to/backup.tar.gz" +} +``` + +## Example to create a new instance from an instance backup with storage + +In order to provide the storage pool name for an instance, which is created +from a backup exactly one `device` configuration of `type = "disk"` might be +provided. The name of the pool is given as the `pool` attribute in +`properties`, which is the only property accepted in this case. + +```hcl +resource "incus_instance" "instance1" { + project = "default" + name = "instance1" + source_file = "/path/to/backup.tar.gz" + + device = { + name = "storage" + type = "disk" + properties = { + pool = "pool-name" + } + } +} +``` + ## Argument Reference * `name` - **Required** - Name of the instance. @@ -96,6 +129,8 @@ resource "incus_instance" "instance2" { * `image` - *Optional* - Base image from which the instance will be created. Must specify [an image accessible from the provider remote](https://linuxcontainers.org/incus/docs/main/reference/image_servers/). +* `source_file` - *Optional* - The souce backup file from which the instance should be restored. For handling of storage pool, see examples. + * `source_instance` - *Optional* - The source instance from which the instance will be created. See reference below. * `description` - *Optional* - Description of the instance. From 636ecbe07c87f0bc4356f04eafeef1f177526c10 Mon Sep 17 00:00:00 2001 From: Lucas Bremgartner Date: Thu, 21 Nov 2024 17:18:26 +0100 Subject: [PATCH 3/5] config: Add source_file to incus_instance Signed-off-by: Lucas Bremgartner --- internal/instance/resource_instance.go | 159 ++++++++++++++++++++++++- 1 file changed, 156 insertions(+), 3 deletions(-) diff --git a/internal/instance/resource_instance.go b/internal/instance/resource_instance.go index 33e9ce3..e29b58b 100644 --- a/internal/instance/resource_instance.go +++ b/internal/instance/resource_instance.go @@ -3,6 +3,7 @@ package instance import ( "context" "fmt" + "os" "sort" "strings" "time" @@ -27,8 +28,9 @@ import ( "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" - "github.com/lxc/incus/v6/client" + incus "github.com/lxc/incus/v6/client" "github.com/lxc/incus/v6/shared/api" + "github.com/mitchellh/go-homedir" "github.com/lxc/terraform-provider-incus/internal/common" "github.com/lxc/terraform-provider-incus/internal/errors" @@ -53,6 +55,7 @@ type InstanceModel struct { Remote types.String `tfsdk:"remote"` Target types.String `tfsdk:"target"` SourceInstance types.Object `tfsdk:"source_instance"` + SourceFile types.String `tfsdk:"source_file"` // Computed. IPv4 types.String `tfsdk:"ipv4_address"` @@ -205,6 +208,16 @@ func (r InstanceResource) Schema(_ context.Context, _ resource.SchemaRequest, re }, }, + "source_file": schema.StringAttribute{ + Optional: true, + PlanModifiers: []planmodifier.String{ + stringplanmodifier.RequiresReplace(), + }, + Validators: []validator.String{ + stringvalidator.LengthAtLeast(1), + }, + }, + // Computed. "ipv4_address": schema.StringAttribute{ @@ -371,13 +384,73 @@ func (r InstanceResource) ValidateConfig(ctx context.Context, req resource.Valid ) } - if !config.Image.IsNull() && !config.SourceInstance.IsNull() { + if !atMostOne(!config.Image.IsNull(), !config.SourceFile.IsNull(), !config.SourceInstance.IsNull()) { resp.Diagnostics.AddError( "Invalid Configuration", - "Only image or source_instance can be set.", + "At most one of image, source_file and source_instance can be set.", ) return } + + if !config.SourceFile.IsNull() { + // Instances from source_file are mutually exclusive with a series of other attributes. + if !config.Description.IsNull() || + !config.Type.IsNull() || + !config.Ephemeral.IsNull() || + !config.Profiles.IsNull() || + !config.Files.IsNull() || + !config.Config.IsNull() { + resp.Diagnostics.AddError( + "Invalid Configuration", + "Attribute source_file is mutually exclusive with description, type, ephemeral, profiles, file and config.", + ) + return + } + + // With `incus import`, a storage pool can be provided optionally. + // In order to support the same behavior with source_file, + // a single device entry of type `disk` is allowed with a single property + // `pool` set to the respective pool name. + if len(config.Devices.Elements()) > 0 { + if len(config.Devices.Elements()) > 1 { + resp.Diagnostics.AddError( + "Invalid Configuration", + "Only one device entry is supported with source_file.", + ) + return + } + + deviceList := make([]common.DeviceModel, 0, 1) + diags = config.Devices.ElementsAs(ctx, &deviceList, false) + if diags.HasError() { + resp.Diagnostics.Append(diags...) + return + } + + if len(deviceList[0].Properties.Elements()) != 1 { + resp.Diagnostics.AddError( + "Invalid Configuration", + `Exactly one device property named "pool" needs to be provided with source_file.`, + ) + return + } + + properties := make(map[string]string, 1) + diags = deviceList[0].Properties.ElementsAs(ctx, &properties, false) + if diags.HasError() { + resp.Diagnostics.Append(diags...) + return + } + + if _, ok := properties["pool"]; !ok { + resp.Diagnostics.AddError( + "Invalid Configuration", + `Exactly one device property named "pool" needs to be provided with source_file.`, + ) + return + } + } + } } func (r InstanceResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) { @@ -401,6 +474,9 @@ func (r InstanceResource) Create(ctx context.Context, req resource.CreateRequest if !plan.Image.IsNull() { diags = r.createInstanceFromImage(ctx, server, plan) resp.Diagnostics.Append(diags...) + } else if !plan.SourceFile.IsNull() { + diags = r.createInstanceFromSourceFile(ctx, server, plan) + resp.Diagnostics.Append(diags...) } else if !plan.SourceInstance.IsNull() { diags = r.createInstanceFromSourceInstance(ctx, server, plan) resp.Diagnostics.Append(diags...) @@ -795,6 +871,13 @@ func (r InstanceResource) SyncState(ctx context.Context, tfState *tfsdk.State, s return respDiags } + if !m.SourceFile.IsNull() && !m.Devices.IsNull() { + // Using device to signal the storage poll is a special case, which is not + // reflected on the instance state and therefore we need to compensate here + // in order to prevent inconsistent provider results. + devices = m.Devices + } + m.Name = types.StringValue(instance.Name) m.Type = types.StringValue(instance.Type) m.Description = types.StringValue(instance.Description) @@ -889,6 +972,66 @@ func (r InstanceResource) createInstanceFromImage(ctx context.Context, server in return diags } +func (r InstanceResource) createInstanceFromSourceFile(ctx context.Context, server incus.InstanceServer, plan InstanceModel) diag.Diagnostics { + var diags diag.Diagnostics + + name := plan.Name.ValueString() + + var poolName string + + if len(plan.Devices.Elements()) > 0 { + // Only one device is expected, this is ensured by ValidateConfig. + deviceList := make([]common.DeviceModel, 0, 1) + diags = plan.Devices.ElementsAs(ctx, &deviceList, false) + if diags.HasError() { + return diags + } + + // Exactly one property named "pool" is expected, this is ensured by ValidateConfig. + properties := make(map[string]string, 1) + diags = deviceList[0].Properties.ElementsAs(ctx, &properties, false) + if diags.HasError() { + return diags + } + + poolName = properties["pool"] + } + + srcFile := plan.SourceFile.ValueString() + + path, err := homedir.Expand(srcFile) + if err != nil { + diags.AddError(fmt.Sprintf("Failed to determine source_file: %q", srcFile), err.Error()) + return diags + } + + file, err := os.Open(path) + if err != nil { + diags.AddError(fmt.Sprintf("Failed to open source_file: %q", path), err.Error()) + return diags + } + + defer func() { _ = file.Close() }() + + createArgs := incus.InstanceBackupArgs{ + BackupFile: file, + PoolName: poolName, + Name: name, + } + + op, err := server.CreateInstanceFromBackup(createArgs) + if err == nil { + err = op.Wait() + } + + if err != nil { + diags.AddError(fmt.Sprintf("Failed to create instance: %q", name), err.Error()) + return diags + } + + return diags +} + func (r InstanceResource) createInstanceFromSourceInstance(ctx context.Context, destServer incus.InstanceServer, plan InstanceModel) diag.Diagnostics { var diags diag.Diagnostics var sourceInstanceModel SourceInstanceModel @@ -1416,3 +1559,13 @@ func getAddresses(name string, entry api.InstanceStateNetwork) (string, string, return ipv4, ipv6, entry.Hwaddr, name, true } + +func atMostOne(in ...bool) bool { + var count int + for _, b := range in { + if b { + count++ + } + } + return count <= 1 +} From 6f6050edf4bd3cafa40b7af4d3347c43eec1eea9 Mon Sep 17 00:00:00 2001 From: Lucas Bremgartner Date: Thu, 21 Nov 2024 17:18:37 +0100 Subject: [PATCH 4/5] test: Add source_file to incus_instance Signed-off-by: Lucas Bremgartner --- internal/instance/resource_instance_test.go | 134 +++++++++++++++++++- 1 file changed, 133 insertions(+), 1 deletion(-) diff --git a/internal/instance/resource_instance_test.go b/internal/instance/resource_instance_test.go index 11ca373..e445b39 100644 --- a/internal/instance/resource_instance_test.go +++ b/internal/instance/resource_instance_test.go @@ -2,10 +2,11 @@ package instance_test import ( "fmt" + "path/filepath" "regexp" "testing" - "github.com/dustinkirkland/golang-petname" + petname "github.com/dustinkirkland/golang-petname" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/lxc/terraform-provider-incus/internal/acctest" @@ -857,6 +858,90 @@ func TestAccInstance_sourceInstanceWithSnapshot(t *testing.T) { }) } +func TestAccInstance_sourceFile(t *testing.T) { + tmpDir := t.TempDir() + backupFile := filepath.Join(tmpDir, "backup.tar.gz") + + sourceInstanceName := petname.Generate(2, "-") + instanceName := petname.Generate(2, "-") + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ProtoV6ProviderFactories: acctest.ProtoV6ProviderFactories, + ExternalProviders: map[string]resource.ExternalProvider{ + "null": { + Source: "null", + VersionConstraint: ">= 3.0.0", + }, + }, + Steps: []resource.TestStep{ + { + Config: testAccInstance_sourceFileExportInstance(sourceInstanceName, backupFile), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("incus_instance.instance1", "name", sourceInstanceName), + resource.TestCheckResourceAttr("incus_instance.instance1", "image", acctest.TestImage), + resource.TestCheckResourceAttr("incus_instance.instance1", "status", "Stopped"), + ), + }, + { + Config: `#`, // Empty config to remove instance. Comment is required, since empty string is seen as zero value. + }, + { + Config: testAccInstance_sourceFile(instanceName, backupFile), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("incus_instance.instance1", "name", instanceName), + resource.TestCheckResourceAttr("incus_instance.instance1", "source_file", backupFile), + resource.TestCheckResourceAttr("incus_instance.instance1", "status", "Stopped"), + ), + }, + }, + }) +} + +func TestAccInstance_sourceFileWithStorage(t *testing.T) { + tmpDir := t.TempDir() + backupFile := filepath.Join(tmpDir, "backup.tar.gz") + + sourceInstanceName := petname.Generate(2, "-") + instanceName := petname.Generate(2, "-") + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ProtoV6ProviderFactories: acctest.ProtoV6ProviderFactories, + ExternalProviders: map[string]resource.ExternalProvider{ + "null": { + Source: "null", + VersionConstraint: ">= 3.0.0", + }, + }, + Steps: []resource.TestStep{ + { + Config: testAccInstance_sourceFileExportInstance(sourceInstanceName, backupFile), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("incus_instance.instance1", "name", sourceInstanceName), + resource.TestCheckResourceAttr("incus_instance.instance1", "image", acctest.TestImage), + resource.TestCheckResourceAttr("incus_instance.instance1", "status", "Stopped"), + ), + }, + { + Config: `#`, // Empty config to remove instance. Comment is required, since empty string is seen as zero value. + }, + { + Config: testAccInstance_sourceFileWithStorage(instanceName, backupFile), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("incus_instance.instance1", "name", instanceName), + resource.TestCheckResourceAttr("incus_instance.instance1", "source_file", backupFile), + resource.TestCheckResourceAttr("incus_instance.instance1", "status", "Running"), + resource.TestCheckResourceAttr("incus_instance.instance1", "device.#", "1"), + resource.TestCheckResourceAttr("incus_instance.instance1", "device.0.name", "storage"), + resource.TestCheckResourceAttr("incus_instance.instance1", "device.0.type", "disk"), + resource.TestCheckResourceAttr("incus_instance.instance1", "device.0.properties.pool", "default"), + ), + }, + }, + }) +} + func testAccInstance_basic(name string, image string) string { return fmt.Sprintf(` resource "incus_instance" "instance1" { @@ -1479,3 +1564,50 @@ resource "incus_instance" "instance2" { } `, projectName, sourceInstanceName, snapshotName, instanceName, acctest.TestImage) } + +func testAccInstance_sourceFileExportInstance(sourceInstanceName, backupFile string) string { + return fmt.Sprintf(` +resource "incus_instance" "instance1" { + name = "%[1]s" + image = "%[2]s" + + running = false +} + +resource "null_resource" "export_instance1" { + provisioner "local-exec" { + command = "incus export ${incus_instance.instance1.name} %[3]s" + } +} +`, sourceInstanceName, acctest.TestImage, backupFile) +} + +func testAccInstance_sourceFile(instanceName, backupFile string) string { + return fmt.Sprintf(` +resource "incus_instance" "instance1" { + name = "%[1]s" + source_file = "%[2]s" + + running = false +} +`, instanceName, backupFile) +} + +func testAccInstance_sourceFileWithStorage(instanceName, backupFile string) string { + return fmt.Sprintf(` +resource "incus_instance" "instance1" { + name = "%[1]s" + source_file = "%[2]s" + + device { + name = "storage" + type = "disk" + properties = { + "pool" = "default" + } + } + + running = true +} +`, instanceName, backupFile) +} From df320a9ac9cfb4afaf28da8b209d1381c18a199d Mon Sep 17 00:00:00 2001 From: Lucas Bremgartner Date: Mon, 25 Nov 2024 15:46:10 +0100 Subject: [PATCH 5/5] review: make path property required for source_file with storage Signed-off-by: Lucas Bremgartner --- docs/resources/instance.md | 3 ++- internal/instance/resource_instance.go | 23 ++++++++++++--------- internal/instance/resource_instance_test.go | 2 ++ 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/docs/resources/instance.md b/docs/resources/instance.md index cc9585d..1d1f79e 100644 --- a/docs/resources/instance.md +++ b/docs/resources/instance.md @@ -104,7 +104,7 @@ resource "incus_instance" "instance1" { In order to provide the storage pool name for an instance, which is created from a backup exactly one `device` configuration of `type = "disk"` might be provided. The name of the pool is given as the `pool` attribute in -`properties`, which is the only property accepted in this case. +`properties`. Additionally the property `path = "/"` is required. ```hcl resource "incus_instance" "instance1" { @@ -116,6 +116,7 @@ resource "incus_instance" "instance1" { name = "storage" type = "disk" properties = { + path = "/" pool = "pool-name" } } diff --git a/internal/instance/resource_instance.go b/internal/instance/resource_instance.go index e29b58b..2828019 100644 --- a/internal/instance/resource_instance.go +++ b/internal/instance/resource_instance.go @@ -409,8 +409,8 @@ func (r InstanceResource) ValidateConfig(ctx context.Context, req resource.Valid // With `incus import`, a storage pool can be provided optionally. // In order to support the same behavior with source_file, - // a single device entry of type `disk` is allowed with a single property - // `pool` set to the respective pool name. + // a single device entry of type `disk` is allowed with exactly two properties + // `path` and `pool` being set. For `path`, the only accepted value is `/`. if len(config.Devices.Elements()) > 0 { if len(config.Devices.Elements()) > 1 { resp.Diagnostics.AddError( @@ -427,25 +427,28 @@ func (r InstanceResource) ValidateConfig(ctx context.Context, req resource.Valid return } - if len(deviceList[0].Properties.Elements()) != 1 { + if len(deviceList[0].Properties.Elements()) != 2 { resp.Diagnostics.AddError( "Invalid Configuration", - `Exactly one device property named "pool" needs to be provided with source_file.`, + `Exactly two device properties named "path" and "pool" need to be provided with source_file.`, ) return } - properties := make(map[string]string, 1) + properties := make(map[string]string, 2) diags = deviceList[0].Properties.ElementsAs(ctx, &properties, false) if diags.HasError() { resp.Diagnostics.Append(diags...) return } - if _, ok := properties["pool"]; !ok { + _, poolOK := properties["pool"] + path, pathOK := properties["path"] + + if !poolOK || !pathOK || path != "/" { resp.Diagnostics.AddError( "Invalid Configuration", - `Exactly one device property named "pool" needs to be provided with source_file.`, + `Exactly two device properties named "path" and "pool" need to be provided with source_file. For "path", the only accepted value is "/".`, ) return } @@ -872,7 +875,7 @@ func (r InstanceResource) SyncState(ctx context.Context, tfState *tfsdk.State, s } if !m.SourceFile.IsNull() && !m.Devices.IsNull() { - // Using device to signal the storage poll is a special case, which is not + // Using device to signal the storage pool is a special case, which is not // reflected on the instance state and therefore we need to compensate here // in order to prevent inconsistent provider results. devices = m.Devices @@ -987,8 +990,8 @@ func (r InstanceResource) createInstanceFromSourceFile(ctx context.Context, serv return diags } - // Exactly one property named "pool" is expected, this is ensured by ValidateConfig. - properties := make(map[string]string, 1) + // Exactly two properties named "path" and "pool" are expected, this is ensured by ValidateConfig. + properties := make(map[string]string, 2) diags = deviceList[0].Properties.ElementsAs(ctx, &properties, false) if diags.HasError() { return diags diff --git a/internal/instance/resource_instance_test.go b/internal/instance/resource_instance_test.go index e445b39..25a944c 100644 --- a/internal/instance/resource_instance_test.go +++ b/internal/instance/resource_instance_test.go @@ -935,6 +935,7 @@ func TestAccInstance_sourceFileWithStorage(t *testing.T) { resource.TestCheckResourceAttr("incus_instance.instance1", "device.#", "1"), resource.TestCheckResourceAttr("incus_instance.instance1", "device.0.name", "storage"), resource.TestCheckResourceAttr("incus_instance.instance1", "device.0.type", "disk"), + resource.TestCheckResourceAttr("incus_instance.instance1", "device.0.properties.path", "/"), resource.TestCheckResourceAttr("incus_instance.instance1", "device.0.properties.pool", "default"), ), }, @@ -1603,6 +1604,7 @@ resource "incus_instance" "instance1" { name = "storage" type = "disk" properties = { + "path" = "/" "pool" = "default" } }