Merge pull request #28 from platform9/fix/compute-image-datasource

fix(compute): read back computed instance fields + boot tests from a synced image

Author: PF9-pushkar

CHANGELOG.md

@@ -45,9 +45,10 @@ All notable changes to this project are documented here. The format is based on
   (attach security groups to an unmanaged port, shared or exclusive via `enforce`); and
   `pcd_networking_floatingip_associate` (bind a pre-allocated floating IP to a port).
 - Compute (Nova v2): resources `pcd_compute_keypair`, `pcd_compute_flavor`,
-  `pcd_compute_servergroup` (acceptance-tested); `pcd_compute_instance` (code-complete —
-  boot verification is blocked on a lab image-library issue, see DECISIONS.md); data
+  `pcd_compute_servergroup`, `pcd_compute_instance` (boot, in-place resize, import); data
   sources `pcd_compute_flavor`, `pcd_compute_keypair`, `pcd_compute_availability_zones`.
+  The instance resource reads back all server-computed fields (availability zone,
+  security groups, network) after apply, and only pushes metadata when the user manages it.
 - Compute follow-ups: `pcd_compute_flavor` gains settable `extra_specs` (added/changed/
   removed in place; the flavor's other attributes are now correctly immutable);
   `pcd_compute_instance` supports in-place **resize** on a flavor change and booting by

DECISIONS.md

@@ -42,6 +42,28 @@ real backend fault):**
 - DNS `zone`/`recordset`: Designate returns `500 no_servers_configured` — the pool has no
   nameservers (BIND9 / `pools.yaml` from the runbook's Phase E step 7 not applied).
 
+### Follow-up run (2026-07-13): compute + Cinder unblocked
+
+After the lab operator uploaded a bootable image to the library and fixed the Cinder
+backend, the compute and block-storage suites were re-run. Block-storage volume passed.
+Compute boot passed once pointed at a library-synced image, and surfaced several real
+provider bugs in `pcd_compute_instance` (all fixed) that were latent because the instance
+had never actually booted before:
+- `flatten` did not read back `availability_zone`, `security_groups`, or the `network`
+  block, so they stayed unknown after apply ("invalid result object"). Now populated
+  (with the security-group list deduplicated, since Nova can repeat `default`).
+- The `network` block's inner `uuid`/`port` lacked `UseStateForUnknown`, so an in-place
+  flavor change re-planned them unknown and tripped `RequiresReplace` — the resize
+  replaced the instance instead of resizing. Fixed.
+- `metadata` was pushed on every update even when unset, sending Nova a nil map
+  (`metadata: None` → 400). Fixed with `UseStateForUnknown` + a guarded update.
+- Separately, `pcd_networking_subnet` delete now retries on `409 SubnetInUse` to ride out
+  Nova's asynchronous port release after an instance is deleted.
+
+The compute VM-boot acc tests now resolve an existing image by name via a data source
+(env `PCD_ACC_IMAGE_NAME`); they skip when it is unset, since PCD does not sync
+web-download images to the hypervisor's local Glance.
+
 | Family | Item | Status |
 |---|---|---|
 | Provider core | password auth + project scope, self-signed TLS (`insecure`), `pcd_identity_auth_scope` | **VALIDATED** |
@@ -61,12 +83,12 @@ real backend fault):**
 | Networking | `pcd_networking_floatingip_associate` | **VALIDATED** (2026-07-12) — associates a separately-allocated floating IP to a port; acc test builds the router path and skips unless `PCD_ACC_EXTERNAL_NETWORK` set. |
 | Compute | `pcd_compute_keypair`, `_flavor`, `_servergroup` | **VALIDATED** |
 | Compute (DS) | `pcd_compute_flavor`, `_keypair`, `_availability_zones` | **VALIDATED** |
-| Compute | `pcd_compute_instance` (boot) | **PENDING** — lab image-library gap: images don't reach the onboarded host's local library → nova returns HTTP 204 for image data. Create/schedule/wait/error-report verified; passes with a library-backed image. |
-| Compute | `pcd_compute_flavor` `extra_specs` | **PENDING** — code-complete; in-place add/change/remove; other flavor attrs now correctly force replacement. Acc test written (create + in-place update + import). Not yet run live (credentials unavailable this session). No lab blocker expected — flavors work on the lab. |
-| Compute | `pcd_compute_instance` resize + `image_name` | **PENDING** — code-complete; flavor change → Nova resize/confirm (revert on failure); `image_name` resolved via Glance. Boot-blocked (same as instance boot above). |
-| Compute | `pcd_compute_interface_attach` | **PENDING** — code-complete; needs a booted instance (boot-blocked). Acc test written. |
-| Compute | `pcd_compute_volume_attach` | **PENDING** — code-complete; needs a booted instance **and** a Cinder backend (both lab-blocked). Best-effort volume waiter degrades gracefully without Cinder. |
-| Block storage | `pcd_blockstorage_volume` | **PENDING** — no Cinder storage backend on the lab (`storageBackends={}`); volumes go `creating → error`. Create + waiter + error-detection verified. |
+| Compute | `pcd_compute_instance` (boot) | **VALIDATED** (2026-07-13) — boots against a library-synced image. The acc test resolves an existing image by name via a data source (env `PCD_ACC_IMAGE_NAME`), because PCD does not sync web-download (`copy-from`) images to the hypervisor's local Glance. |
+| Compute | `pcd_compute_flavor` `extra_specs` | **VALIDATED** — in-place add/change/remove; other flavor attrs force replacement. |
+| Compute | `pcd_compute_instance` resize + `image_name` | **VALIDATED** (2026-07-13) — flavor change resizes in place (verified same instance ID); `image_name` resolved via Glance. |
+| Compute | `pcd_compute_interface_attach` | **VALIDATED** (2026-07-13) — hot-attach a second NIC + import. |
+| Compute | `pcd_compute_volume_attach` | **VALIDATED** (2026-07-13) — boot + Cinder volume attach + import. |
+| Block storage | `pcd_blockstorage_volume` | **VALIDATED** (2026-07-13) — create on the `synology-iscsi` backend → available → extend → import. |
 | Block storage (DS) | `pcd_blockstorage_volume`, `_snapshot` | **PENDING** — untestable without volumes on this lab. |
 | Load balancing (Octavia) | `pcd_lb_loadbalancer`, `_listener`, `_pool`, `_member`, `_monitor` + `_loadbalancer` DS | **VALIDATED** (2026-07-12) — full-tree apply on OVN (lb + TCP listener + `SOURCE_IP_PORT` pool + member + TCP monitor + DS + rename + import). **PCD ships the OVN provider only** (`providers=[ovn]`, no amphora), which is L4 — use TCP/UDP/SCTP listeners and OVN pool algorithms; L7 policy/rule resources were removed (see backout entry). `loadbalancer_provider` was added (default `ovn`) because Octavia's server-side default `amphora` is not enabled — without it every create failed. |
 | DNS (Designate) | `pcd_dns_zone`, `pcd_dns_recordset` + `pcd_dns_zone` DS | **PENDING** — Phase 3, code-complete; async create/update/delete → wait-for-`ACTIVE`/404. Acc test (zone + recordset + import) + examples written. Designate is live on the lab (Step 0) and DNS needs no compute/storage backend, so this should pass live — not yet run this session (credentials unavailable). |

internal/services/compute/instance_resource.go

@@ -24,6 +24,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/resource"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/listplanmodifier"
+	"github.com/hashicorp/terraform-plugin-framework/resource/schema/mapplanmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/setplanmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
@@ -98,7 +99,7 @@ func (r *instanceResource) Schema(_ context.Context, _ resource.SchemaRequest, r
 				MarkdownDescription: "Names of security groups to associate. Changing this forces a new resource.",
 				PlanModifiers:       []planmodifier.Set{setplanmodifier.RequiresReplace(), setplanmodifier.UseStateForUnknown()},
 			},
-			"metadata":          schema.MapAttribute{Optional: true, Computed: true, ElementType: types.StringType, MarkdownDescription: "Key-value metadata attached to the instance."},
+			"metadata":          schema.MapAttribute{Optional: true, Computed: true, ElementType: types.StringType, MarkdownDescription: "Key-value metadata attached to the instance.", PlanModifiers: []planmodifier.Map{mapplanmodifier.UseStateForUnknown()}},
 			"user_data":         schema.StringAttribute{Optional: true, MarkdownDescription: "User data (cloud-init) for the instance. Changing this forces a new resource.", PlanModifiers: fn},
 			"availability_zone": schema.StringAttribute{Optional: true, Computed: true, MarkdownDescription: "Availability zone to launch in. Changing this forces a new resource.", PlanModifiers: fnC},
 			"config_drive":      schema.BoolAttribute{Optional: true, MarkdownDescription: "Whether to use a config drive. Changing this forces a new resource.", PlanModifiers: []planmodifier.Bool{}},
@@ -110,9 +111,9 @@ func (r *instanceResource) Schema(_ context.Context, _ resource.SchemaRequest, r
 			"network": schema.ListNestedBlock{
 				MarkdownDescription: "Networks to attach. Changing this forces a new resource.",
 				NestedObject: schema.NestedBlockObject{Attributes: map[string]schema.Attribute{
-					"uuid": schema.StringAttribute{Optional: true, Computed: true, MarkdownDescription: "Network UUID to attach to (required unless port is set)."},
+					"uuid": schema.StringAttribute{Optional: true, Computed: true, MarkdownDescription: "Network UUID to attach to (required unless port is set).", PlanModifiers: stable},
 					"name": schema.StringAttribute{Optional: true, MarkdownDescription: "Network name (informational)."},
-					"port": schema.StringAttribute{Optional: true, Computed: true, MarkdownDescription: "Existing port to attach (required unless uuid is set)."},
+					"port": schema.StringAttribute{Optional: true, Computed: true, MarkdownDescription: "Existing port to attach (required unless uuid is set).", PlanModifiers: stable},
 				}},
 				PlanModifiers: []planmodifier.List{listplanmodifier.RequiresReplace(), listplanmodifier.UseStateForUnknown()},
 			},
@@ -273,13 +274,14 @@ func (r *instanceResource) Update(ctx context.Context, req resource.UpdateReques
 		}
 	}
 
-	if !plan.Metadata.Equal(state.Metadata) {
-		var meta map[string]string
-		if !plan.Metadata.IsNull() && !plan.Metadata.IsUnknown() {
-			resp.Diagnostics.Append(plan.Metadata.ElementsAs(ctx, &meta, false)...)
-			if resp.Diagnostics.HasError() {
-				return
-			}
+	// Only push metadata when the user actually set it and it changed. A null or
+	// unknown plan value means "unmanaged" — Nova rejects a nil metadata map
+	// (metadata: None) with a 400.
+	if !plan.Metadata.IsNull() && !plan.Metadata.IsUnknown() && !plan.Metadata.Equal(state.Metadata) {
+		meta := map[string]string{}
+		resp.Diagnostics.Append(plan.Metadata.ElementsAs(ctx, &meta, false)...)
+		if resp.Diagnostics.HasError() {
+			return
 		}
 		if _, err := servers.UpdateMetadata(ctx, client, plan.ID.ValueString(), servers.MetadataOpts(meta)).Extract(); err != nil {
 			resp.Diagnostics.AddError("compute: updating instance metadata", err.Error())
@@ -390,6 +392,43 @@ func (r *instanceResource) flatten(ctx context.Context, server *servers.Server,
 	diags = append(diags, d...)
 	m.Metadata = metaMap
 
+	// availability_zone and security_groups are Optional+Computed: when the user
+	// omits them the server assigns values, which must be read back or they stay
+	// unknown after apply ("invalid result object").
+	m.AvailabilityZone = types.StringValue(server.AvailabilityZone)
+
+	seenSG := make(map[string]bool, len(server.SecurityGroups))
+	sgs := make([]string, 0, len(server.SecurityGroups))
+	for _, sg := range server.SecurityGroups {
+		// Nova can list the same security group once per attached port; dedupe so
+		// the Set is valid.
+		if name, ok := sg["name"].(string); ok && !seenSG[name] {
+			seenSG[name] = true
+			sgs = append(sgs, name)
+		}
+	}
+	sgSet, d := types.SetValueFrom(ctx, types.StringType, sgs)
+	diags = append(diags, d...)
+	m.SecurityGroups = sgSet
+
+	// The network block's uuid/port are Optional+Computed; resolve any that the
+	// user left unset (unknown) to a concrete value so the block is fully known.
+	if !m.Network.IsNull() && !m.Network.IsUnknown() {
+		var blocks []instanceNetworkModel
+		diags = append(diags, m.Network.ElementsAs(ctx, &blocks, false)...)
+		for i := range blocks {
+			if blocks[i].UUID.IsNull() || blocks[i].UUID.IsUnknown() {
+				blocks[i].UUID = types.StringValue("")
+			}
+			if blocks[i].Port.IsNull() || blocks[i].Port.IsUnknown() {
+				blocks[i].Port = types.StringValue("")
+			}
+		}
+		netList, d := types.ListValueFrom(ctx, m.Network.ElementType(ctx), blocks)
+		diags = append(diags, d...)
+		m.Network = netList
+	}
+
 	if m.Region.IsNull() || m.Region.IsUnknown() {
 		m.Region = types.StringValue(r.config.Region)
 	}

internal/services/compute/instance_test.go

@@ -18,18 +18,24 @@ import (
 	"github.com/platform9/terraform-provider-pcd/internal/acctest"
 )
 
-// defaultTestImageURL is a small bootable qcow2 (CirrOS) fetched by Glance via
-// web-download, which bypasses the ingress upload-size cap. Override with
-// PCD_ACC_IMAGE_URL.
-const defaultTestImageURL = "https://download.cirros-cloud.net/0.6.2/cirros-0.6.2-x86_64-disk.img"
+// testAccBootImageName returns the name of a pre-synced bootable image to boot
+// VMs from, taken from PCD_ACC_IMAGE_NAME. PCD's image library does not propagate
+// web-download (copy-from) images to the hypervisor's local Glance, so the VM
+// boot tests require an image already uploaded to the library (e.g. a CirrOS
+// image). The test skips when the variable is unset.
+func testAccBootImageName(t *testing.T) string {
+	t.Helper()
+	name := os.Getenv("PCD_ACC_IMAGE_NAME")
+	if name == "" {
+		t.Skip("PCD_ACC_IMAGE_NAME not set; skipping VM-boot test (needs a bootable image already in the image library)")
+	}
+	return name
+}
 
 // TestAccComputeInstance_basic boots a real VM through Terraform end to end:
 // network + subnet + Glance image (web-download) + keypair + instance on m1.tiny.
 func TestAccComputeInstance_basic(t *testing.T) {
-	imageURL := os.Getenv("PCD_ACC_IMAGE_URL")
-	if imageURL == "" {
-		imageURL = defaultTestImageURL
-	}
+	imageName := testAccBootImageName(t)
 	const rn = "pcd_compute_instance.test"
 	var instanceID, flavorID string
 
@@ -39,7 +45,7 @@ func TestAccComputeInstance_basic(t *testing.T) {
 		CheckDestroy:             testAccCheckInstanceDestroy(t),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccInstanceConfig(imageURL, "tf-acc-instance", "m1.tiny"),
+				Config: testAccInstanceConfig(imageName, "tf-acc-instance", "m1.tiny"),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccCheckInstanceExists(t, rn),
 					testAccCaptureID(rn, &instanceID),
@@ -51,12 +57,12 @@ func TestAccComputeInstance_basic(t *testing.T) {
 				),
 			},
 			{
-				Config: testAccInstanceConfig(imageURL, "tf-acc-instance-renamed", "m1.tiny"),
+				Config: testAccInstanceConfig(imageName, "tf-acc-instance-renamed", "m1.tiny"),
 				Check:  resource.TestCheckResourceAttr(rn, "name", "tf-acc-instance-renamed"),
 			},
 			{
 				// Resize: change flavor in place (same instance ID, new flavor_id).
-				Config: testAccInstanceConfig(imageURL, "tf-acc-instance-renamed", "m1.small"),
+				Config: testAccInstanceConfig(imageName, "tf-acc-instance-renamed", "m1.small"),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					resource.TestCheckResourceAttrWith(rn, "id", func(v string) error {
 						if v != instanceID {
@@ -79,10 +85,7 @@ func TestAccComputeInstance_basic(t *testing.T) {
 // TestAccComputeInstance_imageName boots an instance referencing its image by
 // name (resolved via Glance) instead of image_id.
 func TestAccComputeInstance_imageName(t *testing.T) {
-	imageURL := os.Getenv("PCD_ACC_IMAGE_URL")
-	if imageURL == "" {
-		imageURL = defaultTestImageURL
-	}
+	imageName := testAccBootImageName(t)
 	const rn = "pcd_compute_instance.test"
 
 	resource.Test(t, resource.TestCase{
@@ -91,20 +94,24 @@ func TestAccComputeInstance_imageName(t *testing.T) {
 		CheckDestroy:             testAccCheckInstanceDestroy(t),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccInstanceImageNameConfig(imageURL),
+				Config: testAccInstanceImageNameConfig(imageName),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccCheckInstanceExists(t, rn),
 					resource.TestCheckResourceAttr(rn, "status", "ACTIVE"),
 					// image_id is Computed and gets populated from the resolved image_name.
-					resource.TestCheckResourceAttrPair(rn, "image_id", "pcd_images_image.test", "id"),
+					resource.TestCheckResourceAttrPair(rn, "image_id", "data.pcd_images_image.boot", "id"),
 				),
 			},
 		},
 	})
 }
 
-func testAccInstanceConfig(imageURL, name, flavorName string) string {
+func testAccInstanceConfig(imageName, name, flavorName string) string {
 	return fmt.Sprintf(`
+data "pcd_images_image" "boot" {
+  name = %q
+}
+
 resource "pcd_networking_network" "test" {
   name = "tf-acc-inst-net"
 }
@@ -114,20 +121,13 @@ resource "pcd_networking_subnet" "test" {
   cidr       = "10.103.0.0/24"
 }
 
-resource "pcd_images_image" "test" {
-  name             = "tf-acc-inst-img"
-  container_format = "bare"
-  disk_format      = "qcow2"
-  image_source_url = %q
-}
-
 resource "pcd_compute_keypair" "test" {
   name = "tf-acc-inst-key"
 }
 
 resource "pcd_compute_instance" "test" {
   name        = %q
-  image_id    = pcd_images_image.test.id
+  image_id    = data.pcd_images_image.boot.id
   flavor_name = %q
   key_pair    = pcd_compute_keypair.test.name
 
@@ -137,11 +137,15 @@ resource "pcd_compute_instance" "test" {
 
   depends_on = [pcd_networking_subnet.test]
 }
-`, imageURL, name, flavorName)
+`, imageName, name, flavorName)
 }
 
-func testAccInstanceImageNameConfig(imageURL string) string {
+func testAccInstanceImageNameConfig(imageName string) string {
 	return fmt.Sprintf(`
+data "pcd_images_image" "boot" {
+  name = %[1]q
+}
+
 resource "pcd_networking_network" "test" {
   name = "tf-acc-instn-net"
 }
@@ -151,16 +155,9 @@ resource "pcd_networking_subnet" "test" {
   cidr       = "10.113.0.0/24"
 }
 
-resource "pcd_images_image" "test" {
-  name             = "tf-acc-instn-img"
-  container_format = "bare"
-  disk_format      = "qcow2"
-  image_source_url = %q
-}
-
 resource "pcd_compute_instance" "test" {
   name        = "tf-acc-instn"
-  image_name  = pcd_images_image.test.name
+  image_name  = %[1]q
   flavor_name = "m1.tiny"
 
   network {
@@ -169,7 +166,7 @@ resource "pcd_compute_instance" "test" {
 
   depends_on = [pcd_networking_subnet.test]
 }
-`, imageURL)
+`, imageName)
 }
 
 // testAccCaptureAttr records a resource attribute value for later comparison.