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 source_file to incus_instance #172

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
62 changes: 49 additions & 13 deletions docs/resources/instance.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,49 @@ 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`. Additionally the property `path = "/"` is required.

```hcl
resource "incus_instance" "instance1" {
project = "default"
name = "instance1"
source_file = "/path/to/backup.tar.gz"

device = {
name = "storage"
type = "disk"
properties = {
path = "/"
pool = "pool-name"
}
}
}
```

## Argument Reference

* `name` - **Required** - Name of the instance.

* `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.
Expand All @@ -110,20 +146,20 @@ 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.

* `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.

Expand All @@ -140,21 +176,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.

Expand All @@ -163,7 +199,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

Expand Down Expand Up @@ -240,5 +276,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.*`
162 changes: 159 additions & 3 deletions internal/instance/resource_instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package instance
import (
"context"
"fmt"
"os"
"sort"
"strings"
"time"
Expand All @@ -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"
Expand All @@ -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"`
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -371,13 +384,76 @@ 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()) {
Copy link
Member

Choose a reason for hiding this comment

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

I consider this harder to read than before, could we make this more readable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason for this is, that we now have 3 different options, from which at most one (0 or 1) may be used.

I see the following ways to change this code:

Suggested change
if !atMostOne(!config.Image.IsNull(), !config.SourceFile.IsNull(), !config.SourceInstance.IsNull()) {
if moreThanOne(!config.Image.IsNull(), !config.SourceFile.IsNull(), !config.SourceInstance.IsNull()) {

This removes one negation !, which might make it a little bit more readable. On the other side, it is pretty common in Go to have negative condition to enter an error path (basically stating the condition positively and then negating it for error path).

Suggested change
if !atMostOne(!config.Image.IsNull(), !config.SourceFile.IsNull(), !config.SourceInstance.IsNull()) {
if maxOneEnabled(config.Image.IsNull(), config.SourceFile.IsNull(), config.SourceInstance.IsNull()) {

This removes all the negations. The name could also be maxOneNonNull.

Suggested change
if !atMostOne(!config.Image.IsNull(), !config.SourceFile.IsNull(), !config.SourceInstance.IsNull()) {
if (!config.Image.IsNull() && (!config.SourceFile.IsNull() || !config.SourceInstance.IsNull())) || (!config.SourceFile.IsNull() && !config.SourceInstance.IsNull()) {

As list of boolean conditions. Might work for now, but if we need to extend it to a fourth case, I expect this solution to become pretty unreadable.

Which of those do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

The second and in other programming languages this is method is usually called any(…). Where it returns true if one of the values passed are true, otherwise false.

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 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(
"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()) != 2 {
resp.Diagnostics.AddError(
"Invalid Configuration",
`Exactly two device properties named "path" and "pool" need to be provided with source_file.`,
)
return
}

properties := make(map[string]string, 2)
diags = deviceList[0].Properties.ElementsAs(ctx, &properties, false)
if diags.HasError() {
resp.Diagnostics.Append(diags...)
return
}

_, poolOK := properties["pool"]
path, pathOK := properties["path"]

if !poolOK || !pathOK || path != "/" {
resp.Diagnostics.AddError(
"Invalid Configuration",
`Exactly two device properties named "path" and "pool" need to be provided with source_file. For "path", the only accepted value is "/".`,
)
return
}
}
}
}

func (r InstanceResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
Expand All @@ -401,6 +477,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...)
Expand Down Expand Up @@ -795,6 +874,13 @@ func (r InstanceResource) SyncState(ctx context.Context, tfState *tfsdk.State, s
return respDiags
}

if !m.SourceFile.IsNull() && !m.Devices.IsNull() {
maveonair marked this conversation as resolved.
Show resolved Hide resolved
// 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
}

m.Name = types.StringValue(instance.Name)
m.Type = types.StringValue(instance.Type)
m.Description = types.StringValue(instance.Description)
Expand Down Expand Up @@ -889,6 +975,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
Copy link
Member

Choose a reason for hiding this comment

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

What happens when poolName is not set if len(plan.devices.Elements()) == 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, poolName is the zero value, which is the empty string.

Judging from https://github.com/lxc/incus/blob/21eee07711f872aaf6873f0a5f74094dcc5d85c0/client/incus_instances.go#L534, this is fine. If the extension container_backup_override_pool is present, this is handled automatically. If not, an error is returned from incus client.


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 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
}

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
Expand Down Expand Up @@ -1416,3 +1562,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
}
Loading
Loading