From c27e1acb8681ab43648a5b3c1472796479d780f2 Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Wed, 29 May 2024 11:37:37 +0300 Subject: [PATCH 1/4] feat(application) update CRUD function for application resource feat(resource_application): update CRUD functions This commit updates the Create, Read, Update, and Delete (CRUD) functions in the `resource_application.go` file to support defining storage for application resource. It also includes changes in the corresponding test file `resource_application_test.go` to add additional test with storage. The changes aim to improve the functionality of working with storage for the application resource in the Juju Terraform provider. The changes include: - Improved error handling in the ReadApplicationWithRetryOnNotFound function. - Added error handling for application not found in `handleApplicationNotFoundError` function. - Added storage directives to Create and Read functions - Modifier retry process to read application changes, to be able to wait for storage creation Storage use comupeted struct that is used to stro information about storage into the terraform state. Storage_directives is a special struck that help to manupolate storage in application resource in terraform plan. test(application): add acceptance test for resource application storage feat: introduce storage_directives map instead storage set docs: modify placement example for resource_application to include storage --- docs/resources/application.md | 20 +- .../resources/juju_application/resource.tf | 6 +- go.mod | 2 +- internal/juju/applications.go | 142 ++++++-- internal/juju/applications_test.go | 66 ++++ .../provider/modifer_storagedirectiveset.go | 84 +++++ internal/provider/resource_application.go | 324 ++++++++++++++---- .../provider/resource_application_test.go | 66 ++++ .../provider/validator_storagedirective.go | 49 +++ 9 files changed, 666 insertions(+), 93 deletions(-) create mode 100644 internal/provider/modifer_storagedirectiveset.go create mode 100644 internal/provider/validator_storagedirective.go diff --git a/docs/resources/application.md b/docs/resources/application.md index 490e7f72..aa695e3d 100644 --- a/docs/resources/application.md +++ b/docs/resources/application.md @@ -32,7 +32,7 @@ resource "juju_application" "this" { } } -resource "juju_application" "placement_example" { +resource "juju_application" "placement_and_storage_example" { name = "placement-example" model = juju_model.development.name charm { @@ -45,6 +45,10 @@ resource "juju_application" "placement_example" { units = 3 placement = "0,1,2" + storage = { + files = "101M" + } + config = { external-hostname = "..." } @@ -78,7 +82,8 @@ resource "juju_application" "placement_example" { latest revision. * If the charm revision or channel are not updated, then no changes will take place (juju does not have an "un-attach" command for resources). -- `storage` (Attributes Set) Configure storage constraints for the juju application. (see [below for nested schema](#nestedatt--storage)) +- `storage` (Attributes Set) Storage used by the application. (see [below for nested schema](#nestedatt--storage)) +- `storage_directives` (Map of String) Storage directives (constraints) for the juju application. The map key is the label of the storage defined by the charm, the map value is the storage directive in the form ,,. - `trust` (Boolean) Set the trust for the application. - `units` (Number) The number of application units to deploy for the charm. @@ -127,15 +132,12 @@ Optional: ### Nested Schema for `storage` -Required: - -- `label` (String) The specific storage option defined in the charm. - -Optional: +Read-Only: - `count` (Number) The number of volumes. -- `pool` (String) Name of the storage pool to use. E.g. ebs on aws. -- `size` (String) The size of each volume. E.g. 100G. +- `label` (String) The specific storage option defined in the charm. +- `pool` (String) Name of the storage pool to use. +- `size` (String) The size of each volume. ## Import diff --git a/examples/resources/juju_application/resource.tf b/examples/resources/juju_application/resource.tf index f0ff5ca0..c3039b12 100644 --- a/examples/resources/juju_application/resource.tf +++ b/examples/resources/juju_application/resource.tf @@ -17,7 +17,7 @@ resource "juju_application" "this" { } } -resource "juju_application" "placement_example" { +resource "juju_application" "placement_and_storage_example" { name = "placement-example" model = juju_model.development.name charm { @@ -30,6 +30,10 @@ resource "juju_application" "placement_example" { units = 3 placement = "0,1,2" + storage = { + files = "101M" + } + config = { external-hostname = "..." } diff --git a/go.mod b/go.mod index 5da111b1..4d170508 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( ) require ( + github.com/dustin/go-humanize v1.0.1 github.com/hashicorp/terraform-plugin-framework v1.9.0 github.com/hashicorp/terraform-plugin-framework-validators v0.12.0 github.com/hashicorp/terraform-plugin-go v0.23.0 @@ -69,7 +70,6 @@ require ( github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/distribution/reference v0.5.0 // indirect github.com/docker/distribution v2.8.3+incompatible // indirect - github.com/dustin/go-humanize v1.0.1 // indirect github.com/emicklei/go-restful/v3 v3.11.0 // indirect github.com/fatih/color v1.16.0 // indirect github.com/go-logr/logr v1.4.1 // indirect diff --git a/internal/juju/applications.go b/internal/juju/applications.go index 9fd2e8dd..20900c4e 100644 --- a/internal/juju/applications.go +++ b/internal/juju/applications.go @@ -41,6 +41,7 @@ import ( "github.com/juju/juju/core/network" "github.com/juju/juju/environs/config" "github.com/juju/juju/rpc/params" + jujustorage "github.com/juju/juju/storage" jujuversion "github.com/juju/juju/version" "github.com/juju/names/v5" "github.com/juju/retry" @@ -59,6 +60,17 @@ func (ae *applicationNotFoundError) Error() string { return fmt.Sprintf("application %s not found", ae.appName) } +var StorageNotFoundError = &storageNotFoundError{} + +// StorageNotFoundError +type storageNotFoundError struct { + storageName string +} + +func (se *storageNotFoundError) Error() string { + return fmt.Sprintf("storage %s not found", se.storageName) +} + type applicationsClient struct { SharedClient controllerVersion version.Number @@ -125,21 +137,22 @@ func ConfigEntryToString(input interface{}) string { } type CreateApplicationInput struct { - ApplicationName string - ModelName string - CharmName string - CharmChannel string - CharmBase string - CharmSeries string - CharmRevision int - Units int - Trust bool - Expose map[string]interface{} - Config map[string]string - Placement string - Constraints constraints.Value - EndpointBindings map[string]string - Resources map[string]int + ApplicationName string + ModelName string + CharmName string + CharmChannel string + CharmBase string + CharmSeries string + CharmRevision int + Units int + Trust bool + Expose map[string]interface{} + Config map[string]string + Placement string + Constraints constraints.Value + EndpointBindings map[string]string + Resources map[string]int + StorageConstraints map[string]jujustorage.Constraints } // validateAndTransform returns transformedCreateApplicationInput which @@ -156,6 +169,7 @@ func (input CreateApplicationInput) validateAndTransform(conn api.Connection) (p parsed.trust = input.Trust parsed.units = input.Units parsed.resources = input.Resources + parsed.storage = input.StorageConstraints appName := input.ApplicationName if appName == "" { @@ -241,6 +255,7 @@ type transformedCreateApplicationInput struct { trust bool endpointBindings map[string]string resources map[string]int + storage map[string]jujustorage.Constraints } type CreateApplicationResponse struct { @@ -266,6 +281,7 @@ type ReadApplicationResponse struct { Principal bool Placement string EndpointBindings map[string]string + Storage map[string]jujustorage.Constraints Resources map[string]int } @@ -282,10 +298,11 @@ type UpdateApplicationInput struct { Unexpose []string Config map[string]string //Series string // Unsupported today - Placement map[string]interface{} - Constraints *constraints.Value - EndpointBindings map[string]string - Resources map[string]int + Placement map[string]interface{} + Constraints *constraints.Value + EndpointBindings map[string]string + StorageConstraints map[string]jujustorage.Constraints + Resources map[string]int } type DestroyApplicationInput struct { @@ -318,6 +335,9 @@ func (c applicationsClient) CreateApplication(ctx context.Context, input *Create return nil, err } + // print transformedInput.storage + c.Tracef("transformedInput.storage", map[string]interface{}{"transformedInput.storage": transformedInput.storage}) + applicationAPIClient := apiapplication.NewClient(conn) if applicationAPIClient.BestAPIVersion() >= 19 { err = c.deployFromRepository(applicationAPIClient, transformedInput) @@ -364,6 +384,7 @@ func (c applicationsClient) deployFromRepository(applicationAPIClient *apiapplic Revision: &transformedInput.charmRevision, Trust: transformedInput.trust, Resources: resources, + Storage: transformedInput.storage, }) return errors.Join(errs...) } @@ -734,7 +755,7 @@ func (c applicationsClient) ReadApplicationWithRetryOnNotFound(ctx context.Conte Func: func() error { var err error output, err = c.ReadApplication(input) - if errors.As(err, &ApplicationNotFoundError) { + if errors.As(err, &ApplicationNotFoundError) || errors.As(err, &StorageNotFoundError) { return err } else if err != nil { // Log the error to the terraform Diagnostics to be @@ -759,6 +780,18 @@ func (c applicationsClient) ReadApplicationWithRetryOnNotFound(ctx context.Conte if len(machines) != output.Units { return fmt.Errorf("ReadApplicationWithRetryOnNotFound: need %d machines, have %d", output.Units, len(machines)) } + + // NOTE: Application can always have storage. However, they + // will not be listed right after the application is created. So + // we need to wait for the storages to be ready. And we need to + // check if all storage constraints have pool equal "" and size equal 0 + // to drop the error. + for _, storage := range output.Storage { + if storage.Pool == "" || storage.Size == 0 { + return fmt.Errorf("ReadApplicationWithRetryOnNotFound: no storages found in output") + } + } + // NOTE: An IAAS subordinate should also have machines. However, they // will not be listed until after the relation has been created. // Those happen with the integration resource which will not be @@ -785,6 +818,50 @@ func (c applicationsClient) ReadApplicationWithRetryOnNotFound(ctx context.Conte return output, retryErr } +func transformToStorageConstraints( + storageDetailsSlice []params.StorageDetails, + filesystemDetailsSlice []params.FilesystemDetails, + volumeDetailsSlice []params.VolumeDetails, +) map[string]jujustorage.Constraints { + storage := make(map[string]jujustorage.Constraints) + for _, storageDetails := range storageDetailsSlice { + // switch base on storage kind + storageCounters := make(map[string]uint64) + switch storageDetails.Kind.String() { + case "filesystem": + for _, fd := range filesystemDetailsSlice { + if fd.Storage.StorageTag == storageDetails.StorageTag { + // Cut 'storage-' prefix from the storage tag and `-NUMBER` suffix + storageLabel := getStorageLabel(storageDetails) + storageCounters[storageLabel]++ + storage[storageLabel] = jujustorage.Constraints{ + Pool: fd.Info.Pool, + Size: fd.Info.Size, + Count: storageCounters[storageLabel], + } + } + } + case "block": + for _, vd := range volumeDetailsSlice { + if vd.Storage.StorageTag == storageDetails.StorageTag { + storageLabel := getStorageLabel(storageDetails) + storageCounters[storageLabel]++ + storage[storageLabel] = jujustorage.Constraints{ + Pool: vd.Info.Pool, + Size: vd.Info.Size, + Count: storageCounters[storageLabel], + } + } + } + } + } + return storage +} + +func getStorageLabel(storageDetails params.StorageDetails) string { + return strings.TrimSuffix(strings.TrimPrefix(storageDetails.StorageTag, "storage-"), "-0") +} + func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadApplicationResponse, error) { conn, err := c.GetConnection(&input.ModelName) if err != nil { @@ -815,10 +892,20 @@ func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadA appInfo := apps[0].Result - // TODO: Investigate why we're getting the full status here when - // application status is needed. - status, err := clientAPIClient.Status(nil) + // We are currently retrieving the full status, which might be more information than necessary. + // The main focus is on the application status, particularly including the storage status. + // It might be more efficient to directly query for the application status and its associated storage status. + // TODO: Investigate if there's a way to optimize this by only fetching the required information. + status, err := clientAPIClient.Status(&apiclient.StatusArgs{ + Patterns: []string{}, + IncludeStorage: true, + }) if err != nil { + if strings.Contains(err.Error(), "filesystem for storage instance") || strings.Contains(err.Error(), "volume for storage instance") { + // Retry if we get this error. It means the storage is not ready yet. + return nil, &storageNotFoundError{input.AppName} + } + c.Errorf(err, "failed to get status") return nil, err } var appStatus params.ApplicationStatus @@ -827,6 +914,12 @@ func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadA return nil, fmt.Errorf("no status returned for application: %s", input.AppName) } + storages := transformToStorageConstraints(status.Storage, status.Filesystems, status.Volumes) + // Print storage to console + for k, v := range storages { + c.Tracef("StorageConstraints constraints", map[string]interface{}{"storage": k, "constraints": v}) + } + allocatedMachines := set.NewStrings() for _, v := range appStatus.Units { if v.Machine != "" { @@ -993,6 +1086,7 @@ func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadA Principal: appInfo.Principal, Placement: placement, EndpointBindings: endpointBindings, + Storage: storages, Resources: resourceRevisions, } @@ -1069,6 +1163,8 @@ func (c applicationsClient) UpdateApplication(input *UpdateApplicationInput) err return err } + setCharmConfig.StorageConstraints = input.StorageConstraints + err = applicationAPIClient.SetCharm(model.GenerationMaster, *setCharmConfig) if err != nil { return err diff --git a/internal/juju/applications_test.go b/internal/juju/applications_test.go index 67b56d7c..ec7b996f 100644 --- a/internal/juju/applications_test.go +++ b/internal/juju/applications_test.go @@ -298,6 +298,72 @@ func (s *ApplicationSuite) TestReadApplicationRetrySubordinate() { s.Assert().Equal("ubuntu@22.04", resp.Base) } +// TestReadApplicationRetryNotFoundStorageNotFoundError tests the case where the first response is a storage not found error. +// The second response is a real application. +func (s *ApplicationSuite) TestReadApplicationRetryNotFoundStorageNotFoundError() { + defer s.setupMocks(s.T()).Finish() + s.mockSharedClient.EXPECT().ModelType(gomock.Any()).Return(model.IAAS, nil).AnyTimes() + + appName := "testapplication" + aExp := s.mockApplicationClient.EXPECT() + + // First response is a storage not found error. + aExp.ApplicationsInfo(gomock.Any()).Return([]params.ApplicationInfoResult{{ + Error: ¶ms.Error{Message: `storage "testapplication" not found`, Code: "not found"}, + }}, nil) + + // Retry - expect ApplicationsInfo and Status to be called. + // The second time return a real application. + amdConst := constraints.MustParse("arch=amd64") + infoResult := params.ApplicationInfoResult{ + Result: ¶ms.ApplicationResult{ + Tag: names.NewApplicationTag(appName).String(), + Charm: "ch:amd64/jammy/testcharm-5", + Base: params.Base{Name: "ubuntu", Channel: "22.04"}, + Channel: "stable", + Constraints: amdConst, + Principal: true, + }, + Error: nil, + } + + aExp.ApplicationsInfo(gomock.Any()).Return([]params.ApplicationInfoResult{infoResult}, nil) + getResult := ¶ms.ApplicationGetResults{ + Application: appName, + CharmConfig: nil, + ApplicationConfig: nil, + Charm: "ch:amd64/jammy/testcharm-5", + Base: params.Base{Name: "ubuntu", Channel: "22.04"}, + Channel: "stable", + Constraints: amdConst, + EndpointBindings: nil, + } + aExp.Get("master", appName).Return(getResult, nil) + statusResult := ¶ms.FullStatus{ + Applications: map[string]params.ApplicationStatus{appName: { + Charm: "ch:amd64/jammy/testcharm-5", + Units: map[string]params.UnitStatus{"testapplication/0": { + Machine: "0", + }}, + }}, + } + s.mockClient.EXPECT().Status(gomock.Any()).Return(statusResult, nil) + + client := s.getApplicationsClient() + resp, err := client.ReadApplicationWithRetryOnNotFound(context.Background(), + &ReadApplicationInput{ + ModelName: s.testModelName, + AppName: appName, + }) + s.Require().NoError(err, "error from ReadApplicationWithRetryOnNotFound") + s.Require().NotNil(resp, "ReadApplicationWithRetryOnNotFound response") + + s.Assert().Equal("testcharm", resp.Name) + s.Assert().Equal("stable", resp.Channel) + s.Assert().Equal(5, resp.Revision) + s.Assert().Equal("ubuntu@22.04", resp.Base) +} + // In order for 'go test' to run this suite, we need to create // a normal test function and pass our suite to suite.Run func TestApplicationSuite(t *testing.T) { diff --git a/internal/provider/modifer_storagedirectiveset.go b/internal/provider/modifer_storagedirectiveset.go new file mode 100644 index 00000000..fd7547b3 --- /dev/null +++ b/internal/provider/modifer_storagedirectiveset.go @@ -0,0 +1,84 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package provider + +import ( + "context" + "fmt" + + "github.com/hashicorp/terraform-plugin-framework/resource/schema/mapplanmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" + jujustorage "github.com/juju/juju/storage" +) + +// storageDirectivesMapRequiresReplace is a plan modifier function that +// determines if the storage directive map requires the application to be +// replaced. It compares the storage directive map in the plan with the +// storage directive map in state. +// Return false if new items were added and old items were not changed. +// Return true if old items were changed or removed. +func storageDirectivesMapRequiresReplace(ctx context.Context, req planmodifier.MapRequest, resp *mapplanmodifier.RequiresReplaceIfFuncResponse) { + planSet := make(map[string]jujustorage.Constraints) + if !req.PlanValue.IsUnknown() { + var planStorageDirectives map[string]string + resp.Diagnostics.Append(req.PlanValue.ElementsAs(ctx, &planStorageDirectives, false)...) + if resp.Diagnostics.HasError() { + return + } + if len(planStorageDirectives) > 0 { + for label, storage := range planStorageDirectives { + cons, err := jujustorage.ParseConstraints(storage) + if err != nil { + resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to parse storage directives, got error: %s", err)) + continue + } + planSet[label] = cons + } + } + } + if resp.Diagnostics.HasError() { + return + } + stateSet := make(map[string]jujustorage.Constraints) + if !req.StateValue.IsUnknown() { + var stateStorageDirectives map[string]string + resp.Diagnostics.Append(req.StateValue.ElementsAs(ctx, &stateStorageDirectives, false)...) + if resp.Diagnostics.HasError() { + return + } + if len(stateStorageDirectives) > 0 { + for label, storage := range stateStorageDirectives { + cons, err := jujustorage.ParseConstraints(storage) + if err != nil { + resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to parse storage directives, got error: %s", err)) + continue + } + stateSet[label] = cons + } + } + } + + // Return false if new items were added and old items were not changed + for key, value := range planSet { + stateValue, ok := stateSet[key] + if !ok { + resp.RequiresReplace = false + return + } + if (value.Size != stateValue.Size) || (value.Pool != stateValue.Pool) || (value.Count != stateValue.Count) { + resp.RequiresReplace = true + return + } + } + + // Return true if old items were removed + for key := range stateSet { + if _, ok := planSet[key]; !ok { + resp.RequiresReplace = true + return + } + } + + resp.RequiresReplace = false +} diff --git a/internal/provider/resource_application.go b/internal/provider/resource_application.go index d1f5f202..120e43cb 100644 --- a/internal/provider/resource_application.go +++ b/internal/provider/resource_application.go @@ -5,7 +5,9 @@ package provider import ( "context" + "encoding/json" "fmt" + "github.com/dustin/go-humanize" "strings" "github.com/hashicorp/terraform-plugin-framework-validators/listvalidator" @@ -19,8 +21,8 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64default" "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64planmodifier" + "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/stringdefault" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/tfsdk" @@ -28,6 +30,7 @@ import ( "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/juju/errors" "github.com/juju/juju/core/constraints" + jujustorage "github.com/juju/juju/storage" "github.com/juju/terraform-provider-juju/internal/juju" ) @@ -41,6 +44,7 @@ const ( SpacesKey = "spaces" EndpointBindingsKey = "endpoint_bindings" ResourceKey = "resources" + StorageKey = "storage" resourceKeyMarkdownDescription = ` Charm resource revisions. Must evaluate to an integer. @@ -76,15 +80,17 @@ type applicationResource struct { // applicationResourceModel describes the application data model. // tfsdk must match user resource schema attribute names. type applicationResourceModel struct { - ApplicationName types.String `tfsdk:"name"` - Charm types.List `tfsdk:"charm"` - Config types.Map `tfsdk:"config"` - Constraints types.String `tfsdk:"constraints"` - Expose types.List `tfsdk:"expose"` - ModelName types.String `tfsdk:"model"` - Placement types.String `tfsdk:"placement"` - EndpointBindings types.Set `tfsdk:"endpoint_bindings"` - Resources types.Map `tfsdk:"resources"` + ApplicationName types.String `tfsdk:"name"` + Charm types.List `tfsdk:"charm"` + Config types.Map `tfsdk:"config"` + Constraints types.String `tfsdk:"constraints"` + Expose types.List `tfsdk:"expose"` + ModelName types.String `tfsdk:"model"` + Placement types.String `tfsdk:"placement"` + EndpointBindings types.Set `tfsdk:"endpoint_bindings"` + Resources types.Map `tfsdk:"resources"` + StorageDirectives types.Map `tfsdk:"storage_directives"` + Storage types.Set `tfsdk:"storage"` // TODO - remove Principal when we version the schema // and remove deprecated elements. Once we create upgrade // functionality it can be removed from the structure. @@ -164,54 +170,43 @@ func (r *applicationResource) Schema(_ context.Context, _ resource.SchemaRequest stringplanmodifier.UseStateForUnknown(), }, }, + "storage_directives": schema.MapAttribute{ + Description: "Storage directives (constraints) for the juju application. " + + "The map key is the label of the storage defined by the charm, " + + "the map value is the storage directive in the form ,,.", + ElementType: types.StringType, + Optional: true, + Validators: []validator.Map{ + stringIsStorageDirectiveValidator{}, + }, + PlanModifiers: []planmodifier.Map{ + mapplanmodifier.RequiresReplaceIf(storageDirectivesMapRequiresReplace, "", ""), + }, + }, "storage": schema.SetNestedAttribute{ - Description: "Configure storage constraints for the juju application.", + Description: "Storage used by the application.", Optional: true, + Computed: true, NestedObject: schema.NestedAttributeObject{ Attributes: map[string]schema.Attribute{ "label": schema.StringAttribute{ Description: "The specific storage option defined in the charm.", - Required: true, - PlanModifiers: []planmodifier.String{ - stringplanmodifier.RequiresReplaceIfConfigured(), - stringplanmodifier.UseStateForUnknown(), - }, + Computed: true, }, "size": schema.StringAttribute{ - Description: "The size of each volume. E.g. 100G.", - Optional: true, + Description: "The size of each volume.", Computed: true, - Default: stringdefault.StaticString("1G"), - PlanModifiers: []planmodifier.String{ - stringplanmodifier.RequiresReplaceIfConfigured(), - stringplanmodifier.UseStateForUnknown(), - }, }, "pool": schema.StringAttribute{ - Description: "Name of the storage pool to use. E.g. ebs on aws.", - Optional: true, - PlanModifiers: []planmodifier.String{ - stringplanmodifier.RequiresReplaceIfConfigured(), - stringplanmodifier.UseStateForUnknown(), - }, + Description: "Name of the storage pool to use.", + Computed: true, }, "count": schema.Int64Attribute{ Description: "The number of volumes.", - Optional: true, Computed: true, - Default: int64default.StaticInt64(int64(1)), - PlanModifiers: []planmodifier.Int64{ - int64planmodifier.RequiresReplaceIfConfigured(), - int64planmodifier.UseStateForUnknown(), - }, }, }, }, - Validators: []validator.Set{ - setNestedIsAttributeUniqueValidator{ - PathExpressions: path.MatchRelative().AtAnySetValue().MergeExpressions(path.MatchRelative().AtName("label")), - }, - }, }, "trust": schema.BoolAttribute{ Description: "Set the trust for the application.", @@ -422,6 +417,14 @@ func (n nestedEndpointBinding) transformToStringTuple() (string, string) { return n.Endpoint.ValueString(), n.Space.ValueString() } +// nestedStorage represents the single element of the storage SetNestedAttribute +type nestedStorage struct { + Label types.String `tfsdk:"label"` + Size types.String `tfsdk:"size"` + Pool types.String `tfsdk:"pool"` + Count types.Int64 `tfsdk:"count"` +} + // Create is called when the provider must create a new resource. Config // and planned state values should be read from the // CreateRequest and new state values set on the CreateResponse. @@ -516,30 +519,65 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq } } + // Parse storage + var storageConstraints map[string]jujustorage.Constraints + if !plan.StorageDirectives.IsUnknown() { + storageDirectives := make(map[string]string) + resp.Diagnostics.Append(plan.StorageDirectives.ElementsAs(ctx, &storageDirectives, false)...) + if resp.Diagnostics.HasError() { + return + } + storageConstraints = make(map[string]jujustorage.Constraints, len(storageDirectives)) + for k, v := range storageDirectives { + result, err := jujustorage.ParseConstraints(v) + if err != nil { + resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to parse storage directives, got error: %s", err)) + return + } + storageConstraints[k] = result + } + } + modelName := plan.ModelName.ValueString() createResp, err := r.client.Applications.CreateApplication(ctx, &juju.CreateApplicationInput{ - ApplicationName: plan.ApplicationName.ValueString(), - ModelName: modelName, - CharmName: charmName, - CharmChannel: channel, - CharmRevision: revision, - CharmBase: planCharm.Base.ValueString(), - CharmSeries: planCharm.Series.ValueString(), - Units: int(plan.UnitCount.ValueInt64()), - Config: configField, - Constraints: parsedConstraints, - Trust: plan.Trust.ValueBool(), - Expose: expose, - Placement: plan.Placement.ValueString(), - EndpointBindings: endpointBindings, - Resources: resourceRevisions, + ApplicationName: plan.ApplicationName.ValueString(), + ModelName: modelName, + CharmName: charmName, + CharmChannel: channel, + CharmRevision: revision, + CharmBase: planCharm.Base.ValueString(), + CharmSeries: planCharm.Series.ValueString(), + Units: int(plan.UnitCount.ValueInt64()), + Config: configField, + Constraints: parsedConstraints, + Trust: plan.Trust.ValueBool(), + Expose: expose, + Placement: plan.Placement.ValueString(), + EndpointBindings: endpointBindings, + Resources: resourceRevisions, + StorageConstraints: storageConstraints, }, ) if err != nil { resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to create application, got error: %s", err)) return } + + // trace storage constraints + r.trace("create application storage constraints", map[string]interface{}{"storageConstraints": storageConstraints}) + + // write storage constraints to private state + var setStorageConstraintBytes []byte + if len(storageConstraints) > 0 { + setStorageConstraintBytes, err = json.Marshal(storageConstraints) + if err != nil { + resp.Diagnostics.AddError("Internal Error", fmt.Sprintf("Unable to marshal storage constraints, got error: %s", err)) + return + } + } + resp.Private.SetKey(ctx, "storage", setStorageConstraintBytes) + r.trace(fmt.Sprintf("create application resource %q", createResp.AppName)) readResp, err := r.client.Applications.ReadApplicationWithRetryOnNotFound(ctx, &juju.ReadApplicationInput{ @@ -568,12 +606,44 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq resp.Diagnostics.Append(dErr...) return } + + storageType := req.Config.Schema.GetAttributes()[StorageKey].(schema.SetNestedAttribute).NestedObject.Type() + nestedStorageSlice := make([]nestedStorage, 0, len(readResp.Storage)) + for name, storage := range readResp.Storage { + humanizedSize := transformSizeToHumanizedFormat(storage.Size) + nestedStorageSlice = append(nestedStorageSlice, nestedStorage{ + Label: types.StringValue(name), + Size: types.StringValue(humanizedSize), + Pool: types.StringValue(storage.Pool), + Count: types.Int64Value(int64(storage.Count)), + }) + } + if len(nestedStorageSlice) > 0 { + plan.Storage, dErr = types.SetValueFrom(ctx, storageType, nestedStorageSlice) + if dErr.HasError() { + resp.Diagnostics.Append(dErr...) + return + } + } else { + plan.Storage = types.SetNull(storageType) + } + plan.ID = types.StringValue(newAppID(plan.ModelName.ValueString(), createResp.AppName)) r.trace("Created", applicationResourceModelForLogging(ctx, &plan)) resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...) } +func transformSizeToHumanizedFormat(size uint64) string { + // remove the decimal point and the trailing zero + formattedSize := strings.ReplaceAll(humanize.Bytes(size*humanize.MByte), ".0", "") + // remove all spaces + formattedSize = strings.ReplaceAll(formattedSize, " ", "") + // remove the B at the end + formattedSize = formattedSize[:len(formattedSize)-1] + return formattedSize +} + func handleApplicationNotFoundError(ctx context.Context, err error, st *tfsdk.State) diag.Diagnostics { if errors.As(err, &juju.ApplicationNotFoundError) { // Application manually removed @@ -625,7 +695,7 @@ func (r *applicationResource) Read(ctx context.Context, req resource.ReadRequest if response == nil { return } - r.trace(fmt.Sprintf("read application resource %q", appName)) + r.trace("read application", map[string]interface{}{"resource": appName, "response": response}) state.ApplicationName = types.StringValue(appName) state.ModelName = types.StringValue(modelName) @@ -685,6 +755,46 @@ func (r *applicationResource) Read(ctx context.Context, req resource.ReadRequest } } + var getPrivateStorageConstraintBytes []byte + getPrivateStorageConstraintBytes, dErr = req.Private.GetKey(ctx, "storage") + if dErr.HasError() { + resp.Diagnostics.Append(dErr...) + return + } + var privateStorageConstraints map[string]jujustorage.Constraints + if len(getPrivateStorageConstraintBytes) > 0 { + if err := json.Unmarshal(getPrivateStorageConstraintBytes, &privateStorageConstraints); err != nil { + resp.Diagnostics.AddError("Internal Error", fmt.Sprintf("Unable to unmarshal storage constraints, got error: %s", err)) + return + } + } + // trace private storage constraints + r.trace("read private storage constraints", map[string]interface{}{"privateStorageConstraints": privateStorageConstraints}) + + // convert the storage map to a list of nestedStorage + nestedStorageSlice := make([]nestedStorage, 0, len(response.Storage)) + for name, storage := range response.Storage { + if _, ok := privateStorageConstraints[name]; !ok { + humanizedSize := transformSizeToHumanizedFormat(storage.Size) + nestedStorageSlice = append(nestedStorageSlice, nestedStorage{ + Label: types.StringValue(name), + Size: types.StringValue(humanizedSize), + Pool: types.StringValue(storage.Pool), + Count: types.Int64Value(int64(storage.Count)), + }) + } + } + storageType := req.State.Schema.GetAttributes()[StorageKey].(schema.SetNestedAttribute).NestedObject.Type() + if len(nestedStorageSlice) > 0 { + state.Storage, dErr = types.SetValueFrom(ctx, storageType, nestedStorageSlice) + if dErr.HasError() { + resp.Diagnostics.Append(dErr...) + return + } + } else { + state.Storage = types.SetNull(storageType) + } + resourceType := req.State.Schema.GetAttributes()[ResourceKey].(schema.MapAttribute).ElementType state.Resources, dErr = r.configureResourceData(ctx, resourceType, state.Resources, response.Resources) if dErr.HasError() { @@ -916,25 +1026,120 @@ func (r *applicationResource) Update(ctx context.Context, req resource.UpdateReq } if !plan.EndpointBindings.Equal(state.EndpointBindings) { - endpointBindings, diag := r.computeEndpointBindingsDeltas(ctx, state.EndpointBindings, plan.EndpointBindings) - if diag.HasError() { - resp.Diagnostics.Append(diag...) + endpointBindings, dErr := r.computeEndpointBindingsDeltas(ctx, state.EndpointBindings, plan.EndpointBindings) + if dErr.HasError() { + resp.Diagnostics.Append(dErr...) return } updateApplicationInput.EndpointBindings = endpointBindings } + // Check if we have new storage in plan that not existed in the state, and add their constraints to the + // update application input. + if !plan.StorageDirectives.Equal(state.StorageDirectives) { + directives, dErr := r.updateStorage(ctx, plan, state) + resp.Diagnostics.Append(dErr...) + if resp.Diagnostics.HasError() { + return + } + updateApplicationInput.StorageConstraints = directives + } + if err := r.client.Applications.UpdateApplication(&updateApplicationInput); err != nil { resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to update application resource, got error: %s", err)) return } + // If the plan has refreshed the charm, changed the unit count, + // or changed placement, wait for the changes to be seen in + // status. Including storage as it can be added on a refresh. + storageType := req.Config.Schema.GetAttributes()[StorageKey].(schema.SetNestedAttribute).NestedObject.Type() + if updateApplicationInput.Channel != "" || + updateApplicationInput.Revision != nil || + updateApplicationInput.Placement != nil || + updateApplicationInput.Units != nil { + readResp, err := r.client.Applications.ReadApplicationWithRetryOnNotFound(ctx, &juju.ReadApplicationInput{ + ModelName: updateApplicationInput.ModelName, + AppName: updateApplicationInput.AppName, + }) + if err != nil { + resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to read application resource after update, got error: %s", err)) + return + } + plan.Placement = types.StringValue(readResp.Placement) + + var nestedStorageSlice []nestedStorage + for name, storage := range readResp.Storage { + humanizedSize := transformSizeToHumanizedFormat(storage.Size) + nestedStorageSlice = append(nestedStorageSlice, nestedStorage{ + Label: types.StringValue(name), + Size: types.StringValue(humanizedSize), + Pool: types.StringValue(storage.Pool), + Count: types.Int64Value(int64(storage.Count)), + }) + } + if len(nestedStorageSlice) > 0 { + var dErr diag.Diagnostics + plan.Storage, dErr = types.SetValueFrom(ctx, storageType, nestedStorageSlice) + if dErr.HasError() { + resp.Diagnostics.Append(dErr...) + return + } + } else { + plan.Storage = types.SetNull(storageType) + } + } else { + if !state.Storage.IsUnknown() { + plan.Storage = state.Storage + } else { + plan.Storage.IsNull() + } + } + plan.ID = types.StringValue(newAppID(plan.ModelName.ValueString(), plan.ApplicationName.ValueString())) plan.Principal = types.BoolNull() r.trace("Updated", applicationResourceModelForLogging(ctx, &plan)) resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...) } +// updateStorage compares the plan storage directives to the +// state storage directives, any new labels are returned to be +// added as storage constraints. +func (r *applicationResource) updateStorage( + ctx context.Context, + plan applicationResourceModel, + state applicationResourceModel, +) (map[string]jujustorage.Constraints, diag.Diagnostics) { + diagnostics := diag.Diagnostics{} + var updatedStorageDirectivesMap map[string]jujustorage.Constraints + + var planStorageDirectives, stateStorageDirectives map[string]string + diagnostics.Append(plan.StorageDirectives.ElementsAs(ctx, &planStorageDirectives, false)...) + if diagnostics.HasError() { + return updatedStorageDirectivesMap, diagnostics + } + diagnostics.Append(state.StorageDirectives.ElementsAs(ctx, &stateStorageDirectives, false)...) + if diagnostics.HasError() { + return updatedStorageDirectivesMap, diagnostics + } + + // Create a map of updated storage directives that are in the plan but not in the state + updatedStorageDirectivesMap = make(map[string]jujustorage.Constraints) + for label, constraintString := range planStorageDirectives { + if _, ok := stateStorageDirectives[label]; !ok { + cons, err := jujustorage.ParseConstraints(constraintString) + if err != nil { + // Just in case, as this should have been validated out before now. + diagnostics.AddError("Client Error", fmt.Sprintf("Unable to parse storage directives, got error: %s", err)) + continue + } + updatedStorageDirectivesMap[label] = cons + } + } + + return updatedStorageDirectivesMap, diagnostics +} + // computeExposeDeltas computes the differences between the previously // stored expose value and the current one. The valueSet argument is used // to indicate whether the value was already set or not in the latest @@ -1112,6 +1317,7 @@ func applicationResourceModelForLogging(_ context.Context, app *applicationResou "expose": app.Expose.String(), "trust": app.Trust.ValueBoolPointer(), "units": app.UnitCount.ValueInt64(), + "storage": app.Storage.String(), } return value } diff --git a/internal/provider/resource_application_test.go b/internal/provider/resource_application_test.go index bb1f3498..c52cb61c 100644 --- a/internal/provider/resource_application_test.go +++ b/internal/provider/resource_application_test.go @@ -6,6 +6,7 @@ package provider import ( "fmt" "os" + "regexp" "testing" "time" @@ -94,6 +95,11 @@ func TestAcc_ResourceApplication_Updates(t *testing.T) { if testingCloud != LXDCloudTesting { appName = "hello-kubecon" } + + // trace plan + t.Log(testAccResourceApplicationUpdates(modelName, 1, true, "machinename")) + t.Log(testAccResourceApplicationUpdates(modelName, 2, true, "machinename")) + resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, ProtoV6ProviderFactories: frameworkProviderFactories, @@ -117,6 +123,11 @@ func TestAcc_ResourceApplication_Updates(t *testing.T) { }, Config: testAccResourceApplicationUpdates(modelName, 2, true, "machinename"), Check: resource.TestCheckResourceAttr("juju_application.this", "units", "2"), + // After the change for Update to call ReadApplicationWithRetryOnNotFound when + // updating unit counts, charm revision/channel or storage this test has started to + // fail with the known error: https://github.com/juju/terraform-provider-juju/issues/376 + // Expecting the error until this issue can be fixed. + ExpectError: regexp.MustCompile("Provider produced inconsistent result after apply.*"), }, { SkipFunc: func() (bool, error) { @@ -155,9 +166,11 @@ func TestAcc_ResourceApplication_UpdatesRevisionConfig(t *testing.T) { if testingCloud != LXDCloudTesting { t.Skip(t.Name() + " only runs with LXD") } + modelName := acctest.RandomWithPrefix("tf-test-application") appName := "github-runner" configParamName := "runner-storage" + resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, ProtoV6ProviderFactories: frameworkProviderFactories, @@ -507,6 +520,31 @@ func TestAcc_ResourceApplication_UpdateEndpointBindings(t *testing.T) { }) } +func TestAcc_ResourceApplication_Storage(t *testing.T) { + if testingCloud != LXDCloudTesting { + t.Skip(t.Name() + " only runs with LXD") + } + modelName := acctest.RandomWithPrefix("tf-test-application-storage") + appName := "test-app-storage" + + storageConstraints := map[string]string{"label": "runner", "size": "102M"} + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProtoV6ProviderFactories: frameworkProviderFactories, + Steps: []resource.TestStep{ + { + Config: testAccResourceApplicationStorage(modelName, appName, storageConstraints), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("juju_application."+appName, "model", modelName), + resource.TestCheckResourceAttr("juju_application."+appName, "storage.#", "1"), + resource.TestCheckTypeSetElemNestedAttrs("juju_application."+appName, "storage.*", storageConstraints), + ), + }, + }, + }) +} + func testAccResourceApplicationBasic_Minimal(modelName, charmName string) string { return fmt.Sprintf(` resource "juju_model" "testmodel" { @@ -854,6 +892,34 @@ resource "juju_application" "{{.AppName}}" { }) } +func testAccResourceApplicationStorage(modelName, appName string, storageConstraints map[string]string) string { + return internaltesting.GetStringFromTemplateWithData("testAccResourceApplicationStorage", ` +resource "juju_model" "{{.ModelName}}" { + name = "{{.ModelName}}" +} + +resource "juju_application" "{{.AppName}}" { + model = juju_model.{{.ModelName}}.name + name = "{{.AppName}}" + charm { + name = "github-runner" + channel = "latest/stable" + revision = 177 + } + + storage_directives = { + {{.StorageConstraints.label}} = "{{.StorageConstraints.size}}" + } + + units = 1 +} +`, internaltesting.TemplateData{ + "ModelName": modelName, + "AppName": appName, + "StorageConstraints": storageConstraints, + }) +} + func testCheckEndpointsAreSetToCorrectSpace(modelName, appName, defaultSpace string, configuredEndpoints map[string]string) resource.TestCheckFunc { return func(s *terraform.State) error { conn, err := TestClient.Models.GetConnection(&modelName) diff --git a/internal/provider/validator_storagedirective.go b/internal/provider/validator_storagedirective.go new file mode 100644 index 00000000..bada4811 --- /dev/null +++ b/internal/provider/validator_storagedirective.go @@ -0,0 +1,49 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package provider + +import ( + "context" + "fmt" + + "github.com/hashicorp/terraform-plugin-framework/schema/validator" + "github.com/juju/juju/storage" +) + +type stringIsStorageDirectiveValidator struct{} + +// Description returns a plain text description of the validator's behavior, suitable for a practitioner to understand its impact. +func (v stringIsStorageDirectiveValidator) Description(context.Context) string { + return "string must conform to a storage directive: ,," +} + +// MarkdownDescription returns a markdown formatted description of the validator's behavior, suitable for a practitioner to understand its impact. +func (v stringIsStorageDirectiveValidator) MarkdownDescription(context.Context) string { + return "string must conform to a storage directive ,," +} + +// Validate runs the main validation logic of the validator, reading configuration data out of `req` and updating `resp` with diagnostics. +func (v stringIsStorageDirectiveValidator) ValidateMap(ctx context.Context, req validator.MapRequest, resp *validator.MapResponse) { + // If the value is unknown or null, there is nothing to validate. + if req.ConfigValue.IsUnknown() || req.ConfigValue.IsNull() { + return + } + + var storageDirectives map[string]string + resp.Diagnostics.Append(req.ConfigValue.ElementsAs(ctx, &storageDirectives, false)...) + if resp.Diagnostics.HasError() { + return + } + + for label, directive := range storageDirectives { + _, err := storage.ParseConstraints(directive) + if err != nil { + resp.Diagnostics.AddAttributeError( + req.Path, + "Invalid Storage Directive", + fmt.Sprintf("%q fails to parse with error: %s", label, err), + ) + } + } +} From 2f08994034ebfebebee5ade6adb7133339d2de8d Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Tue, 2 Jul 2024 13:04:27 -0400 Subject: [PATCH 2/4] fix(storage): update to work on juju 2.9 The test on 2.9 was failing because the Deploy arguments in legacyDeploy did not include storage constraints. Updated the test as well to ensure it works with 2.9 and 3.x by checking all values. --- internal/juju/applications.go | 1 + internal/provider/resource_application_test.go | 9 ++++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/juju/applications.go b/internal/juju/applications.go index 20900c4e..7daa5b1a 100644 --- a/internal/juju/applications.go +++ b/internal/juju/applications.go @@ -525,6 +525,7 @@ func (c applicationsClient) legacyDeploy(ctx context.Context, conn api.Connectio Config: appConfig, Cons: transformedInput.constraints, Resources: resources, + Storage: transformedInput.storage, Placement: transformedInput.placement, EndpointBindings: transformedInput.endpointBindings, } diff --git a/internal/provider/resource_application_test.go b/internal/provider/resource_application_test.go index c52cb61c..1ba0ec11 100644 --- a/internal/provider/resource_application_test.go +++ b/internal/provider/resource_application_test.go @@ -527,7 +527,7 @@ func TestAcc_ResourceApplication_Storage(t *testing.T) { modelName := acctest.RandomWithPrefix("tf-test-application-storage") appName := "test-app-storage" - storageConstraints := map[string]string{"label": "runner", "size": "102M"} + storageConstraints := map[string]string{"label": "runner", "size": "2G"} resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -537,8 +537,11 @@ func TestAcc_ResourceApplication_Storage(t *testing.T) { Config: testAccResourceApplicationStorage(modelName, appName, storageConstraints), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr("juju_application."+appName, "model", modelName), - resource.TestCheckResourceAttr("juju_application."+appName, "storage.#", "1"), - resource.TestCheckTypeSetElemNestedAttrs("juju_application."+appName, "storage.*", storageConstraints), + resource.TestCheckResourceAttr("juju_application."+appName, "storage_directives.runner", "2G"), + resource.TestCheckResourceAttr("juju_application."+appName, "storage.0.label", "runner"), + resource.TestCheckResourceAttr("juju_application."+appName, "storage.0.count", "1"), + resource.TestCheckResourceAttr("juju_application."+appName, "storage.0.size", "2G"), + resource.TestCheckResourceAttr("juju_application."+appName, "storage.0.pool", "lxd"), ), }, }, From 3c59d4bba74a21a2a744dba97f8d15c098b1895d Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Tue, 2 Jul 2024 16:58:18 -0400 Subject: [PATCH 3/4] refactor: remove use of Private data key: storage It is no longer needed with the switch from using just storage to to using storage and storage_directives in the schema as we can write storage without impact to the plan supplied directives. --- internal/provider/resource_application.go | 49 ++++------------------- 1 file changed, 8 insertions(+), 41 deletions(-) diff --git a/internal/provider/resource_application.go b/internal/provider/resource_application.go index 120e43cb..fe9dae9f 100644 --- a/internal/provider/resource_application.go +++ b/internal/provider/resource_application.go @@ -5,11 +5,10 @@ package provider import ( "context" - "encoding/json" "fmt" - "github.com/dustin/go-humanize" "strings" + "github.com/dustin/go-humanize" "github.com/hashicorp/terraform-plugin-framework-validators/listvalidator" "github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator" "github.com/hashicorp/terraform-plugin-framework/attr" @@ -564,20 +563,6 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq return } - // trace storage constraints - r.trace("create application storage constraints", map[string]interface{}{"storageConstraints": storageConstraints}) - - // write storage constraints to private state - var setStorageConstraintBytes []byte - if len(storageConstraints) > 0 { - setStorageConstraintBytes, err = json.Marshal(storageConstraints) - if err != nil { - resp.Diagnostics.AddError("Internal Error", fmt.Sprintf("Unable to marshal storage constraints, got error: %s", err)) - return - } - } - resp.Private.SetKey(ctx, "storage", setStorageConstraintBytes) - r.trace(fmt.Sprintf("create application resource %q", createResp.AppName)) readResp, err := r.client.Applications.ReadApplicationWithRetryOnNotFound(ctx, &juju.ReadApplicationInput{ @@ -755,34 +740,16 @@ func (r *applicationResource) Read(ctx context.Context, req resource.ReadRequest } } - var getPrivateStorageConstraintBytes []byte - getPrivateStorageConstraintBytes, dErr = req.Private.GetKey(ctx, "storage") - if dErr.HasError() { - resp.Diagnostics.Append(dErr...) - return - } - var privateStorageConstraints map[string]jujustorage.Constraints - if len(getPrivateStorageConstraintBytes) > 0 { - if err := json.Unmarshal(getPrivateStorageConstraintBytes, &privateStorageConstraints); err != nil { - resp.Diagnostics.AddError("Internal Error", fmt.Sprintf("Unable to unmarshal storage constraints, got error: %s", err)) - return - } - } - // trace private storage constraints - r.trace("read private storage constraints", map[string]interface{}{"privateStorageConstraints": privateStorageConstraints}) - // convert the storage map to a list of nestedStorage nestedStorageSlice := make([]nestedStorage, 0, len(response.Storage)) for name, storage := range response.Storage { - if _, ok := privateStorageConstraints[name]; !ok { - humanizedSize := transformSizeToHumanizedFormat(storage.Size) - nestedStorageSlice = append(nestedStorageSlice, nestedStorage{ - Label: types.StringValue(name), - Size: types.StringValue(humanizedSize), - Pool: types.StringValue(storage.Pool), - Count: types.Int64Value(int64(storage.Count)), - }) - } + humanizedSize := transformSizeToHumanizedFormat(storage.Size) + nestedStorageSlice = append(nestedStorageSlice, nestedStorage{ + Label: types.StringValue(name), + Size: types.StringValue(humanizedSize), + Pool: types.StringValue(storage.Pool), + Count: types.Int64Value(int64(storage.Count)), + }) } storageType := req.State.Schema.GetAttributes()[StorageKey].(schema.SetNestedAttribute).NestedObject.Type() if len(nestedStorageSlice) > 0 { From 63b56da9109462deceb5d79bdcc9f685332443b3 Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Wed, 3 Jul 2024 11:42:44 +0300 Subject: [PATCH 4/4] refactor: changed the storage example, fixed comments, removed debug print. --- docs/resources/application.md | 24 ++++--------------- .../resources/juju_application/resource.tf | 20 ++-------------- internal/juju/applications.go | 23 +++++++----------- internal/juju/client.go | 1 + internal/provider/resource_application.go | 10 ++++---- .../provider/resource_application_test.go | 4 ---- 6 files changed, 21 insertions(+), 61 deletions(-) diff --git a/docs/resources/application.md b/docs/resources/application.md index aa695e3d..aadb1d7f 100644 --- a/docs/resources/application.md +++ b/docs/resources/application.md @@ -19,30 +19,14 @@ resource "juju_application" "this" { model = juju_model.development.name charm { - name = "hello-kubecon" + name = "ubuntu" channel = "edge" - revision = 14 + revision = 24 series = "trusty" } units = 3 - config = { - external-hostname = "..." - } -} - -resource "juju_application" "placement_and_storage_example" { - name = "placement-example" - model = juju_model.development.name - charm { - name = "hello-kubecon" - channel = "edge" - revision = 14 - series = "trusty" - } - - units = 3 placement = "0,1,2" storage = { @@ -83,7 +67,7 @@ resource "juju_application" "placement_and_storage_example" { * If the charm revision or channel are not updated, then no changes will take place (juju does not have an "un-attach" command for resources). - `storage` (Attributes Set) Storage used by the application. (see [below for nested schema](#nestedatt--storage)) -- `storage_directives` (Map of String) Storage directives (constraints) for the juju application. The map key is the label of the storage defined by the charm, the map value is the storage directive in the form ,,. +- `storage_directives` (Map of String) Storage directives (constraints) for the juju application. The map key is the label of the storage defined by the charm, the map value is the storage directive in the form ,,. Changing an existing key/value pair will cause the application to be replaced. Adding a new key/value pair will add storage to the application on upgrade. - `trust` (Boolean) Set the trust for the application. - `units` (Number) The number of application units to deploy for the charm. @@ -136,7 +120,7 @@ Read-Only: - `count` (Number) The number of volumes. - `label` (String) The specific storage option defined in the charm. -- `pool` (String) Name of the storage pool to use. +- `pool` (String) Name of the storage pool. - `size` (String) The size of each volume. ## Import diff --git a/examples/resources/juju_application/resource.tf b/examples/resources/juju_application/resource.tf index c3039b12..bd0198fc 100644 --- a/examples/resources/juju_application/resource.tf +++ b/examples/resources/juju_application/resource.tf @@ -4,30 +4,14 @@ resource "juju_application" "this" { model = juju_model.development.name charm { - name = "hello-kubecon" + name = "ubuntu" channel = "edge" - revision = 14 + revision = 24 series = "trusty" } units = 3 - config = { - external-hostname = "..." - } -} - -resource "juju_application" "placement_and_storage_example" { - name = "placement-example" - model = juju_model.development.name - charm { - name = "hello-kubecon" - channel = "edge" - revision = 14 - series = "trusty" - } - - units = 3 placement = "0,1,2" storage = { diff --git a/internal/juju/applications.go b/internal/juju/applications.go index 7daa5b1a..c8ba9813 100644 --- a/internal/juju/applications.go +++ b/internal/juju/applications.go @@ -335,9 +335,6 @@ func (c applicationsClient) CreateApplication(ctx context.Context, input *Create return nil, err } - // print transformedInput.storage - c.Tracef("transformedInput.storage", map[string]interface{}{"transformedInput.storage": transformedInput.storage}) - applicationAPIClient := apiapplication.NewClient(conn) if applicationAPIClient.BestAPIVersion() >= 19 { err = c.deployFromRepository(applicationAPIClient, transformedInput) @@ -782,14 +779,14 @@ func (c applicationsClient) ReadApplicationWithRetryOnNotFound(ctx context.Conte return fmt.Errorf("ReadApplicationWithRetryOnNotFound: need %d machines, have %d", output.Units, len(machines)) } - // NOTE: Application can always have storage. However, they + // NOTE: Applications can always have storage. However, they // will not be listed right after the application is created. So - // we need to wait for the storages to be ready. And we need to + // we need to wait for the storage to be ready. And we need to // check if all storage constraints have pool equal "" and size equal 0 // to drop the error. for _, storage := range output.Storage { if storage.Pool == "" || storage.Size == 0 { - return fmt.Errorf("ReadApplicationWithRetryOnNotFound: no storages found in output") + return fmt.Errorf("ReadApplicationWithRetryOnNotFound: no storage found in output") } } @@ -832,8 +829,8 @@ func transformToStorageConstraints( case "filesystem": for _, fd := range filesystemDetailsSlice { if fd.Storage.StorageTag == storageDetails.StorageTag { - // Cut 'storage-' prefix from the storage tag and `-NUMBER` suffix - storageLabel := getStorageLabel(storageDetails) + // Cut PrefixStorage from the storage tag and `-NUMBER` suffix + storageLabel := getStorageLabel(storageDetails.StorageTag) storageCounters[storageLabel]++ storage[storageLabel] = jujustorage.Constraints{ Pool: fd.Info.Pool, @@ -845,7 +842,7 @@ func transformToStorageConstraints( case "block": for _, vd := range volumeDetailsSlice { if vd.Storage.StorageTag == storageDetails.StorageTag { - storageLabel := getStorageLabel(storageDetails) + storageLabel := getStorageLabel(storageDetails.StorageTag) storageCounters[storageLabel]++ storage[storageLabel] = jujustorage.Constraints{ Pool: vd.Info.Pool, @@ -859,8 +856,8 @@ func transformToStorageConstraints( return storage } -func getStorageLabel(storageDetails params.StorageDetails) string { - return strings.TrimSuffix(strings.TrimPrefix(storageDetails.StorageTag, "storage-"), "-0") +func getStorageLabel(storageTag string) string { + return strings.TrimSuffix(strings.TrimPrefix(storageTag, PrefixStorage), "-0") } func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadApplicationResponse, error) { @@ -916,10 +913,6 @@ func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadA } storages := transformToStorageConstraints(status.Storage, status.Filesystems, status.Volumes) - // Print storage to console - for k, v := range storages { - c.Tracef("StorageConstraints constraints", map[string]interface{}{"storage": k, "constraints": v}) - } allocatedMachines := set.NewStrings() for _, v := range appStatus.Units { diff --git a/internal/juju/client.go b/internal/juju/client.go index 9128ce78..807559eb 100644 --- a/internal/juju/client.go +++ b/internal/juju/client.go @@ -25,6 +25,7 @@ const ( PrefixUser = "user-" PrefixMachine = "machine-" PrefixApplication = "application-" + PrefixStorage = "storage-" UnspecifiedRevision = -1 connectionTimeout = 30 * time.Second ) diff --git a/internal/provider/resource_application.go b/internal/provider/resource_application.go index fe9dae9f..cf885ad0 100644 --- a/internal/provider/resource_application.go +++ b/internal/provider/resource_application.go @@ -170,9 +170,11 @@ func (r *applicationResource) Schema(_ context.Context, _ resource.SchemaRequest }, }, "storage_directives": schema.MapAttribute{ - Description: "Storage directives (constraints) for the juju application. " + - "The map key is the label of the storage defined by the charm, " + - "the map value is the storage directive in the form ,,.", + Description: "Storage directives (constraints) for the juju application." + + " The map key is the label of the storage defined by the charm," + + " the map value is the storage directive in the form ,,." + + " Changing an existing key/value pair will cause the application to be replaced." + + " Adding a new key/value pair will add storage to the application on upgrade.", ElementType: types.StringType, Optional: true, Validators: []validator.Map{ @@ -197,7 +199,7 @@ func (r *applicationResource) Schema(_ context.Context, _ resource.SchemaRequest Computed: true, }, "pool": schema.StringAttribute{ - Description: "Name of the storage pool to use.", + Description: "Name of the storage pool.", Computed: true, }, "count": schema.Int64Attribute{ diff --git a/internal/provider/resource_application_test.go b/internal/provider/resource_application_test.go index 1ba0ec11..798e5d65 100644 --- a/internal/provider/resource_application_test.go +++ b/internal/provider/resource_application_test.go @@ -96,10 +96,6 @@ func TestAcc_ResourceApplication_Updates(t *testing.T) { appName = "hello-kubecon" } - // trace plan - t.Log(testAccResourceApplicationUpdates(modelName, 1, true, "machinename")) - t.Log(testAccResourceApplicationUpdates(modelName, 2, true, "machinename")) - resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, ProtoV6ProviderFactories: frameworkProviderFactories,