Skip to content

Commit 1f59c15

Browse files
committed
fix: correct volume allocation semantics
Resolves: #1240
1 parent bde8afb commit 1f59c15

7 files changed

Lines changed: 235 additions & 22 deletions

File tree

docs/resources/volume.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ resource "libvirt_volume" "from_local" {
9898

9999
### Optional
100100

101+
- `allocation` (Number) Initial volume allocation in bytes. Omit to use libvirt's default full allocation; set below capacity to request sparse allocation where supported.
101102
- `allocation_unit` (String) Specifies the units for the allocated space in the storage volume.
102103
- `backing_store` (Attributes) Backing store configuration for copy-on-write volumes (see [below for nested schema](#nestedatt--backing_store))
103104
- `capacity` (Number) Volume capacity in bytes (required unless using create.content)
@@ -109,7 +110,6 @@ resource "libvirt_volume" "from_local" {
109110

110111
### Read-Only
111112

112-
- `allocation` (Number) Configures the total amount of space allocated for the storage volume.
113113
- `id` (String) Volume identifier (same as key)
114114
- `key` (String) Defines a unique key identifier for the storage volume.
115115
- `path` (String) Volume path on the host filesystem (same as target.path)

internal/codegen/policy/field_policy.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ var fieldPolicies = map[string][]fieldPolicy{
2323
},
2424
"StorageVolume.allocation": {
2525
policyComputedReportedField,
26-
policyDisablePreserveUserIntent,
2726
},
2827
"StorageVolume.physical": {
2928
policyComputedReportedField,

internal/codegen/policy/field_policy_test.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ func TestApplyFieldPoliciesMarksReportedFieldOverrides(t *testing.T) {
109109
Fields: []*generator.FieldIR{
110110
{TFName: "capacity", IsOptional: true, IsRequired: true},
111111
{TFName: "physical", IsOptional: true, IsRequired: true, PreserveUserIntent: true},
112+
{TFName: "allocation", IsOptional: true, IsRequired: true, PreserveUserIntent: true},
112113
},
113114
},
114115
}
@@ -146,4 +147,12 @@ func TestApplyFieldPoliciesMarksReportedFieldOverrides(t *testing.T) {
146147
if volumePhysical.PreserveUserIntent {
147148
t.Fatal("expected StorageVolume.physical to disable PreserveUserIntent")
148149
}
150+
151+
volumeAllocation := structs[1].Fields[2]
152+
if !volumeAllocation.IsComputed || volumeAllocation.IsOptional || volumeAllocation.IsRequired {
153+
t.Fatal("expected StorageVolume.allocation to be computed in the IR after override")
154+
}
155+
if !volumeAllocation.PreserveUserIntent {
156+
t.Fatal("expected StorageVolume.allocation to preserve user intent")
157+
}
149158
}

internal/codegen/templates/convert.go.tmpl

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -636,7 +636,7 @@ func {{ .Name }}ToXML(ctx context.Context, model *{{ .Name }}Model) (*libvirtxml
636636
{{- define "toXMLField" }}
637637
{{- if .IsFlattenedValue }}
638638
// Flattened chardata+attributes field: {{ .GoName }}
639-
if !model.{{ .GoName }}.IsNull() {
639+
if !model.{{ .GoName }}.IsNull() && !model.{{ .GoName }}.IsUnknown() {
640640
{{- $valueType := .NestedStruct.ValueWithUnitPattern.ValueGoType }}
641641
{{- $valueField := .NestedStruct.ValueWithUnitPattern.ValueFieldName }}
642642
{{- if .IsPointer }}
@@ -653,7 +653,7 @@ func {{ .Name }}ToXML(ctx context.Context, model *{{ .Name }}Model) (*libvirtxml
653653
}
654654
// Set attributes if user specified them
655655
{{- range .NestedStruct.ValueWithUnitPattern.AttributeFields }}
656-
if !model.{{ $.GoName }}{{ .GoName }}.IsNull() {
656+
if !model.{{ $.GoName }}{{ .GoName }}.IsNull() && !model.{{ $.GoName }}{{ .GoName }}.IsUnknown() {
657657
{{- if eq .GoType "string" }}
658658
xml.{{ $.GoName }}.{{ .GoName }} = model.{{ $.GoName }}{{ .GoName }}.ValueString()
659659
{{- else if eq .GoType "uint" }}
@@ -679,7 +679,7 @@ func {{ .Name }}ToXML(ctx context.Context, model *{{ .Name }}Model) (*libvirtxml
679679
}
680680
// Set attributes if user specified them
681681
{{- range .NestedStruct.ValueWithUnitPattern.AttributeFields }}
682-
if !model.{{ $.GoName }}{{ .GoName }}.IsNull() {
682+
if !model.{{ $.GoName }}{{ .GoName }}.IsNull() && !model.{{ $.GoName }}{{ .GoName }}.IsUnknown() {
683683
{{- if eq .GoType "string" }}
684684
xml.{{ $.GoName }}.{{ .GoName }} = model.{{ $.GoName }}{{ .GoName }}.ValueString()
685685
{{- else if eq .GoType "uint" }}

internal/generated/conversion_test.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -987,3 +987,66 @@ func TestDomainConsoleFromXMLPreservesPlannedAliasWhenXMLAliasIsOmitted(t *testi
987987
t.Fatalf("expected alias name serial0, got %v", got.Name)
988988
}
989989
}
990+
991+
func TestStorageVolumeToXMLAllocationSemantics(t *testing.T) {
992+
ctx := context.Background()
993+
994+
tests := []struct {
995+
name string
996+
allocation types.Int64
997+
wantNil bool
998+
wantValue uint64
999+
}{
1000+
{
1001+
name: "null is omitted",
1002+
allocation: types.Int64Null(),
1003+
wantNil: true,
1004+
},
1005+
{
1006+
name: "unknown is omitted",
1007+
allocation: types.Int64Unknown(),
1008+
wantNil: true,
1009+
},
1010+
{
1011+
name: "explicit zero requests sparse allocation",
1012+
allocation: types.Int64Value(0),
1013+
wantValue: 0,
1014+
},
1015+
{
1016+
name: "explicit capacity requests full allocation",
1017+
allocation: types.Int64Value(1024 * 1024),
1018+
wantValue: 1024 * 1024,
1019+
},
1020+
}
1021+
1022+
for _, tt := range tests {
1023+
t.Run(tt.name, func(t *testing.T) {
1024+
model := &StorageVolumeModel{
1025+
Name: types.StringValue("test-volume"),
1026+
Allocation: tt.allocation,
1027+
AllocationUnit: types.StringNull(),
1028+
Capacity: types.Int64Value(1024 * 1024),
1029+
CapacityUnit: types.StringNull(),
1030+
}
1031+
1032+
xml, err := StorageVolumeToXML(ctx, model)
1033+
if err != nil {
1034+
t.Fatalf("StorageVolumeToXML failed: %v", err)
1035+
}
1036+
1037+
if tt.wantNil {
1038+
if xml.Allocation != nil {
1039+
t.Fatalf("expected allocation to be omitted, got %+v", xml.Allocation)
1040+
}
1041+
return
1042+
}
1043+
1044+
if xml.Allocation == nil {
1045+
t.Fatal("expected allocation to be present")
1046+
}
1047+
if xml.Allocation.Value != tt.wantValue {
1048+
t.Fatalf("expected allocation %d, got %d", tt.wantValue, xml.Allocation.Value)
1049+
}
1050+
})
1051+
}
1052+
}

internal/provider/volume_resource.go

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,11 @@ func (r *VolumeResource) Schema(ctx context.Context, req resource.SchemaRequest,
110110
Optional: true,
111111
Computed: true,
112112
},
113+
"allocation": schema.Int64Attribute{
114+
Description: "Initial volume allocation in bytes. Omit to use libvirt's default full allocation; set below capacity to request sparse allocation where supported.",
115+
Optional: true,
116+
Computed: true,
117+
},
113118
"target": schema.SingleNestedAttribute{
114119
Optional: true,
115120
Attributes: targetAttrs,
@@ -225,30 +230,32 @@ func (r *VolumeResource) Create(ctx context.Context, req resource.CreateRequest,
225230
}
226231
}
227232

228-
// Determine capacity: prefer the upload stream size when available,
229-
// otherwise fall back to user-provided capacity.
233+
// Determine volume capacity independently from upload length. A configured
234+
// capacity may intentionally be larger than the source image.
230235
var volumeCapacity int64
231-
if uploadStream != nil {
232-
switch {
233-
case uploadCapacity != nil:
234-
volumeCapacity = *uploadCapacity
235-
case !model.Capacity.IsNull() && !model.Capacity.IsUnknown():
236-
volumeCapacity = model.Capacity.ValueInt64()
237-
default:
236+
if !model.Capacity.IsNull() && !model.Capacity.IsUnknown() {
237+
volumeCapacity = model.Capacity.ValueInt64()
238+
if uploadCapacity != nil && volumeCapacity < *uploadCapacity {
238239
resp.Diagnostics.AddError(
239-
"Missing Capacity",
240-
"Volume capacity is required when the upload source does not provide Content-Length",
240+
"Insufficient Capacity",
241+
fmt.Sprintf("Configured volume capacity %d is smaller than upload content length %d", volumeCapacity, *uploadCapacity),
241242
)
242243
return
243244
}
244-
} else if model.Capacity.IsNull() || model.Capacity.IsUnknown() {
245+
} else if uploadCapacity != nil {
246+
volumeCapacity = *uploadCapacity
247+
} else if uploadStream != nil {
245248
resp.Diagnostics.AddError(
246249
"Missing Capacity",
247-
"Volume capacity is required when not uploading from a URL",
250+
"Volume capacity is required when the upload source does not provide Content-Length",
248251
)
249252
return
250253
} else {
251-
volumeCapacity = model.Capacity.ValueInt64()
254+
resp.Diagnostics.AddError(
255+
"Missing Capacity",
256+
"Volume capacity is required when not uploading from a URL",
257+
)
258+
return
252259
}
253260

254261
// Create a model for XML conversion with computed capacity
@@ -306,13 +313,18 @@ func (r *VolumeResource) Create(ctx context.Context, req resource.CreateRequest,
306313
}
307314
}()
308315

316+
uploadLength := volumeCapacity
317+
if uploadCapacity != nil {
318+
uploadLength = *uploadCapacity
319+
}
320+
309321
tflog.Info(ctx, "Uploading content to volume", map[string]any{
310-
"size": volumeCapacity,
322+
"size": uploadLength,
311323
})
312324

313325
// Upload the content using StorageVolUpload
314-
// The 0 flag means start at offset 0, volumeCapacity is the length.
315-
err = r.client.Libvirt().StorageVolUpload(volume, uploadStream.Reader, 0, uint64(volumeCapacity), 0)
326+
// The 0 flag means start at offset 0; length is the source byte count.
327+
err = r.client.Libvirt().StorageVolUpload(volume, uploadStream.Reader, 0, uint64(uploadLength), 0)
316328
if err != nil {
317329
// Upload failed, try to clean up the volume (ignore cleanup errors to preserve original error)
318330
if delErr := r.client.Libvirt().StorageVolDelete(volume, 0); delErr != nil {

internal/provider/volume_resource_test.go

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,59 @@ func TestAccVolumeResource_basic(t *testing.T) {
7777
resource.TestCheckResourceAttrSet("libvirt_volume.test", "allocation"),
7878
),
7979
},
80+
{
81+
Config: testAccVolumeResourceConfigBasic("test-volume", poolPath),
82+
PlanOnly: true,
83+
},
8084
// Delete testing automatically occurs in TestCase
8185
},
8286
})
8387
}
8488

89+
func TestAccVolumeResource_explicitSparseAllocation(t *testing.T) {
90+
poolPath := t.TempDir()
91+
92+
resource.Test(t, resource.TestCase{
93+
PreCheck: func() { testAccPreCheck(t) },
94+
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
95+
Steps: []resource.TestStep{
96+
{
97+
Config: testAccVolumeResourceConfigAllocation("test-volume-sparse", poolPath, 0),
98+
Check: resource.ComposeAggregateTestCheckFunc(
99+
resource.TestCheckResourceAttr("libvirt_volume.test", "capacity", "1048576"),
100+
resource.TestCheckResourceAttr("libvirt_volume.test", "allocation", "0"),
101+
),
102+
},
103+
{
104+
Config: testAccVolumeResourceConfigAllocation("test-volume-sparse", poolPath, 0),
105+
PlanOnly: true,
106+
},
107+
},
108+
})
109+
}
110+
111+
func TestAccVolumeResource_explicitFullAllocation(t *testing.T) {
112+
poolPath := t.TempDir()
113+
114+
resource.Test(t, resource.TestCase{
115+
PreCheck: func() { testAccPreCheck(t) },
116+
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
117+
Steps: []resource.TestStep{
118+
{
119+
Config: testAccVolumeResourceConfigAllocation("test-volume-full", poolPath, 1024*1024),
120+
Check: resource.ComposeAggregateTestCheckFunc(
121+
resource.TestCheckResourceAttr("libvirt_volume.test", "capacity", "1048576"),
122+
resource.TestCheckResourceAttr("libvirt_volume.test", "allocation", "1048576"),
123+
),
124+
},
125+
{
126+
Config: testAccVolumeResourceConfigAllocation("test-volume-full", poolPath, 1024*1024),
127+
PlanOnly: true,
128+
},
129+
},
130+
})
131+
}
132+
85133
func TestAccVolumeResource_importByKey(t *testing.T) {
86134
poolPath := t.TempDir()
87135

@@ -497,6 +545,41 @@ func TestAccVolumeResource_uploadFromHTTPWithContentLength(t *testing.T) {
497545
})
498546
}
499547

548+
func TestAccVolumeResource_uploadIntoLargerCapacity(t *testing.T) {
549+
poolPath := t.TempDir()
550+
testContent := bytes.Repeat([]byte("d"), 1024*1024)
551+
requestedCapacity := int64(2 * 1024 * 1024)
552+
553+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
554+
if r.URL.Path != "/image.raw" {
555+
http.NotFound(w, r)
556+
return
557+
}
558+
559+
http.ServeContent(w, r, "image.raw", time.Time{}, bytes.NewReader(testContent))
560+
}))
561+
defer server.Close()
562+
563+
resource.Test(t, resource.TestCase{
564+
PreCheck: func() { testAccPreCheck(t) },
565+
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
566+
Steps: []resource.TestStep{
567+
{
568+
Config: testAccVolumeResourceConfigUploadFromURL(
569+
"test-volume-upload-larger",
570+
poolPath,
571+
server.URL+"/image.raw",
572+
&requestedCapacity,
573+
),
574+
Check: resource.ComposeAggregateTestCheckFunc(
575+
resource.TestCheckResourceAttr("libvirt_volume.test", "capacity", "2097152"),
576+
testAccCheckVolumeFileSize("libvirt_volume.test", requestedCapacity),
577+
),
578+
},
579+
},
580+
})
581+
}
582+
500583
func TestAccVolumeResource_uploadFromHTTPWithoutContentLengthUsesCapacity(t *testing.T) {
501584
poolPath := t.TempDir()
502585
testContent := bytes.Repeat([]byte("b"), 1024*1024)
@@ -620,6 +703,53 @@ resource "libvirt_volume" "test" {
620703
`, name, poolPath, sourceFile)
621704
}
622705

706+
func testAccVolumeResourceConfigAllocation(name, poolPath string, allocation int64) string {
707+
return fmt.Sprintf(`
708+
resource "libvirt_pool" "test" {
709+
name = "test-pool-allocation-%[1]s"
710+
type = "dir"
711+
target = {
712+
path = %[2]q
713+
}
714+
}
715+
716+
resource "libvirt_volume" "test" {
717+
name = "%[1]s.raw"
718+
pool = libvirt_pool.test.name
719+
capacity = 1048576
720+
allocation = %[3]d
721+
target = {
722+
format = {
723+
type = "raw"
724+
}
725+
}
726+
}
727+
`, name, poolPath, allocation)
728+
}
729+
730+
func testAccCheckVolumeFileSize(resourceName string, expected int64) resource.TestCheckFunc {
731+
return func(state *terraform.State) error {
732+
volume, ok := state.RootModule().Resources[resourceName]
733+
if !ok {
734+
return fmt.Errorf("%s not found in state", resourceName)
735+
}
736+
737+
path := volume.Primary.Attributes["path"]
738+
if path == "" {
739+
return fmt.Errorf("%s path not found in state", resourceName)
740+
}
741+
742+
info, err := os.Stat(path)
743+
if err != nil {
744+
return fmt.Errorf("stat volume %s: %w", path, err)
745+
}
746+
if info.Size() != expected {
747+
return fmt.Errorf("expected volume file size %d, got %d", expected, info.Size())
748+
}
749+
return nil
750+
}
751+
}
752+
623753
func testAccVolumeResourceConfigUploadFromURL(name, poolPath, sourceURL string, capacity *int64) string {
624754
capacityConfig := ""
625755
if capacity != nil {

0 commit comments

Comments
 (0)