Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement]: Add SVM storage_limit parameter #328

Open
wants to merge 10 commits into
base: integration/main
Choose a base branch
from

Conversation

acch
Copy link

@acch acch commented Oct 16, 2024

Adds ability to configure storage_limit parameter of SVM.

Closes #302.

Acceptance tests pass:

$ TF_ACC=1 go test ./internal/provider/svm/svm_resource_test.go -v
=== RUN   TestAccSvmResource
--- PASS: TestAccSvmResource (33.36s)
PASS
ok      command-line-arguments  33.367s

Example Terraform Configuration:

// sample SVM with storage_limit parameter
resource "netapp-ontap_svm" "example" {
  cx_profile_name = ...
  name            = ...
  ...
  storage_limit   = "4294967296"
}

@acch acch marked this pull request as ready for review October 16, 2024 12:31
@carchi8py carchi8py added the enhancement New feature or request label Oct 29, 2024
@carchi8py carchi8py added this to the 2.0 milestone Oct 29, 2024
@carchi8py
Copy link
Contributor

@acch before we can accept this we need you to follow this (https://github.com/NetApp/terraform-provider-netapp-ontap/blob/integration/main/CONTRIBUTING.md). I think the only steps missing right now is step 5.

@carchi8py carchi8py modified the milestones: 2.0, 2.1 Oct 29, 2024
@acch
Copy link
Author

acch commented Oct 30, 2024

@carchi8py I had signed the NetApp CCLA on Oct 16... please let me know what's missing.

@carchi8py
Copy link
Contributor

@acch thank i have that now. Look like the email group we were using was not accepting External email so we didn't get a copy. We've fix that and have your CCLA.

@carchi8py
Copy link
Contributor

We have a major version 2.0 that in the final stages of our legal review.

We will be adding this in to our 2.1 release which will be the release after that one.

acch and others added 6 commits December 9, 2024 14:11
Signed-off-by: Achim Christ <achim.christ@sva.de>
Signed-off-by: Achim Christ <achim.christ@sva.de>
Signed-off-by: Achim Christ <achim.christ@sva.de>
Signed-off-by: Achim Christ <achim.christ@sva.de>
Signed-off-by: Achim Christ <achim.christ@sva.de>
Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch acch force-pushed the 302-svm-storage_limit-parameter branch from 2fcb74a to b576f5f Compare December 9, 2024 14:14
@carchi8py
Copy link
Contributor

@acch Happy Holidays, just a head up we will be reviewing this shorting. Sorry for the hold up we were waiting on Security to finish their review of 2.0 before releasing it and that ended up taking significantly longer than we had hope. Now that, that out of the way we are starting to merge thing in again. Right now I'm working on getting all the dependencies updated and then we'll start review these.

Probably will have review the first week of Jan (or end of this week if I have time).

@acch
Copy link
Author

acch commented Dec 22, 2024

Happy Holidays @carchi8py! Thanks for the update.
I'll be back in the office the first week of Jan., and will check if anything needs rebasing...

Please let me know what I can do to help.

Thanks much for all your efforts — enjoy a few days off! 🎄

@chuyich
Copy link
Contributor

chuyich commented Jan 2, 2025

@acch Since storage_limit is the new field and only supported 9.13 or higher, please update the support platform section on the document of data source and resource under docs.
Also, the version check has to be done. You may check this as a reference.

Copy link
Contributor

@chuyich chuyich left a comment

Choose a reason for hiding this comment

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

Please update doc and add version check.

Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch
Copy link
Author

acch commented Jan 6, 2025

Hello @chuyich, thank you very much for reviewing this PR.

I have updated the docs and added cluster version checks to GetSvmByName*(). Acceptance tests (still) pass.
Please let me know if this is acceptable, or what is missing.

Thanks much!

@acch acch requested a review from chuyich January 6, 2025 14:25
Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch acch force-pushed the 302-svm-storage_limit-parameter branch from 19f6f0b to 4880190 Compare January 6, 2025 14:26
@@ -13,6 +13,7 @@ Retrieves the configuration of SVM
## Supported Platforms

* On-prem ONTAP system 9.6 or higher
* `storage_limit` attribute supported with ONTAP system 9.13 or higher
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be added in the list of attributes on line 37-46

@@ -13,6 +13,7 @@ Retrieves the configuration of SVMs.
## Supported Platforms

* On-prem ONTAP system 9.6 or higher
* `storage_limit` attribute supported with ONTAP system 9.13 or higher
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be added in the list of attributes on line 64-72

@@ -21,11 +21,12 @@ Create/Modify/Delete a SVM
## Supported Platforms

* On-prem ONTAP system 9.6 or higher
* `storage_limit` attribute supported with ONTAP system 9.13 or higher
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be added in the list of attributes on line 47-63

MarkdownDescription: "Maximum storage permitted on svm, in bytes",
Optional: true,
Computed: true,
Default: int64default.StaticInt64(0),
Copy link
Contributor

Choose a reason for hiding this comment

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

It is an optional parameter based on the API doc https://docs.netapp.com/us-en/ontap-restapi-9131/ontap/post-svm-svms.html#recommended-optional-properties
The default value is not needed.

Copy link
Author

@acch acch Jan 7, 2025

Choose a reason for hiding this comment

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

Without a default value, the attribute remains "unknown" after the apply - resulting in the following error:

╷
│ Error: Provider returned invalid result object after apply
│ 
│ After the apply operation, the provider still indicated an unknown value for netapp-ontap_svm.example.storage_limit. All values must be known after apply, so this is
│ always a bug in the provider and should be reported in the provider's own repository. Terraform will still save the other known object values in the state.
╵

I could manually set the value to 0 in internal/provider/svm/svm_resource.go > Create(), but isn't a default value in the schema definition easier to read & understand?

Aggregates []map[string]string `mapstructure:"aggregates"`
Name string `mapstructure:"name,omitempty"`
SnapshotPolicy SnapshotPolicy `mapstructure:"snapshot_policy,omitempty"`
Storage Storage `mapstructure:"storage"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's optional, it should be set omitempty.

Copy link
Author

Choose a reason for hiding this comment

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

OK, I did mark the optional storage and (nested) limit params as 'omitempty'. However, there's some code in place to delete empty params from the request body, which I've adapted to... don't think 'omitempty' will make any difference.

https://github.com/acch/terraform-provider-netapp-ontap/blob/ceeb032a1923209353296a28db20468801db729d/internal/interfaces/svm.go#L221-L232

	if setAggrEmpty {
		delete(body, "aggregates")
	}
	if setCommentEmpty {
		delete(body, "comment")
	}
	if setStorageLimitEmpty {
		// delete storage.limit from request body, so that ONTAP uses default value
		if v, ok := body["storage"].(map[string]interface{}); ok {
			delete(v, "limit")
		}
	}

Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch
Copy link
Author

acch commented Jan 7, 2025

Thanks @carchi8py and @chuyich for your feedback!

I've added the storage_limit argument to the docs. I was under the impression that these lists are generated automatically, due to the schema generated by tfplugindocs comment... Now understand that they need to be updated manually.

I've also marked the optional params as 'omitempty', and did verify that acceptance tests (still) pass.
Please let me know if this is acceptable, or what is missing.

Thanks much!

Signed-off-by: Achim Christ <achim.christ@sva.de>
@acch acch force-pushed the 302-svm-storage_limit-parameter branch from ceeb032 to 98e9570 Compare January 7, 2025 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 2.1
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Add SVM storage_limit parameter
3 participants