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.

Sign up for GitHub

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

Jump to bottom

Netapp volumes: Add restoreParameters #9918

Merged
merged 19 commits into from
Feb 15, 2024
Merged

Netapp volumes: Add restoreParameters #9918

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c26e36b
Add force delete to delete volumes with nested snapshots
okrause Jan 19, 2024
10c647f
Merge branch 'main' into netapp-volume
okrause Jan 23, 2024
cbf790f
few cleanups
okrause Jan 23, 2024
e785eb8
Make virtual field verifies happy
okrause Jan 23, 2024
7c1ed3f
Minor test improvements
okrause Jan 23, 2024
8937f05
Remove ignore_read for deletion_policy
okrause Jan 25, 2024
3d9e0fd
Merge branch 'main' into netapp-volume
okrause Jan 29, 2024
d540112
Fix typos in snapRestore comment
okrause Jan 29, 2024
4d23511
Adding missing fields state, StateDetails, backupConfig
okrause Jan 29, 2024
9f0011d
BackupConfig is work in progress
okrause Jan 30, 2024
7114826
Merge branch 'main' into netapp-volume
okrause Feb 2, 2024
bc6bfd8
Implemented restoreParameters
okrause Feb 2, 2024
aa9f798
More improvements on restore_parameters
okrause Feb 5, 2024
0cdf2c7
Comment cleanups
okrause Feb 5, 2024
9b852a9
Remove backupConfig support for now.
okrause Feb 5, 2024
2e66ef2
Fix small typos
okrause Feb 5, 2024
1b9f077
- Adding state, state_details and create_time fields
okrause Feb 7, 2024
7a0b658
convert tabs to spaces in test resource blocks
okrause Feb 13, 2024
50c53e6
remove backupConfig. Will PR later
okrause Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 79 additions & 14 deletions mmv1/products/netapp/volume.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ parameters:
url_param_only: true
examples:
- !ruby/object:Provider::Terraform::Examples
name: 'volume_basic'
name: 'netapp_volume_basic'
Copy link
Member

@shuyama1 shuyama1 Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Any specific reason that we want to change this name + handwritten test names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the inner Monk in me. I don't like the fact that the example isn't prefixed with netapp_volume to make clear where it belongs too. Having that prefix makes the content of the example folder more tidy, move the example file closer to other netapp_volume resources in the folder listing and tells anyone looking at the file name to which resource it belongs too.
So basically this is cleanup. Is this causing other problems I don't see?

Copy link
Member

Choose a reason for hiding this comment

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

No big issues. It's just we run integration tests nightly and changing name may lose history of test results. So just want to make sure this change is intended

primary_resource_id: 'test_volume'
vars:
volume_name: 'test-volume'
Expand All @@ -64,6 +64,30 @@ examples:
test_vars_overrides:
network_name: 'acctest.BootstrapSharedServiceNetworkingConnection(t, "gcnv-network-config-1", acctest.ServiceNetworkWithParentService("netapp.servicenetworking.goog"))'
properties:
- !ruby/object:Api::Type::Enum
name: 'state'
description: |
State of the volume.
values:
- STATE_UNSPECIFIED
- READY
- CREATING
- DELETING
- UPDATING
- RESTORING
- DISABLED
- ERROR
output: true
- !ruby/object:Api::Type::String
name: 'stateDetails'
description: |
State details of the volume.
output: true
- !ruby/object:Api::Type::String
name: 'createTime'
description: |
Create time of the volume. A timestamp in RFC3339 UTC "Zulu" format. Examples: "2023-06-22T09:13:01.617Z".
output: true
- !ruby/object:Api::Type::String
name: 'shareName'
description: |
Expand Down Expand Up @@ -205,11 +229,11 @@ properties:
name: 'description'
description: |
An optional description of this resource.
# Use of snapReserve is depricated. We don't expose it intentially.
# Use of snapReserve is deprecated. Here as a comment to express intention.
# - !ruby/object:Api::Type::Integer
# name: 'snapReserve'
# description: |
# Snap_reserve specifies percentage of volume storage reserved for snapshot storage. Default is 0 percent. Use is deprecated.
# `snap_reserve` specifies percentage of volume storage reserved for snapshot storage. Default is 0 percent. Use is deprecated.
- !ruby/object:Api::Type::Boolean
name: 'snapshotDirectory'
description: |
Expand Down Expand Up @@ -245,6 +269,34 @@ properties:
description: |
Reports the resource name of the Active Directory policy being used. Inherited from storage pool.
output: true
- !ruby/object:Api::Type::NestedObject
name: 'restoreParameters'
description: |-
Used to create this volume from a snapshot (= cloning) or an backup.
immutable: true
# This parameter is only used at CREATE. READs will omit it.
ignore_read: true
properties:
- !ruby/object:Api::Type::String
name: 'sourceSnapshot'
description: |-
Full name of the snapshot to use for creating this volume.
`source_snapshot` and `source_backup` cannot be used simultaneously.
Format: `projects/{{project}}/locations/{{location}}/volumes/{{volume}}/snapshots/{{snapshot}}`.
exactly_one_of:
- restore_parameters.0.source_backup
- restore_parameters.0.source_snapshot
immutable: true
- !ruby/object:Api::Type::String
name: 'sourceBackup'
Copy link
Member

Choose a reason for hiding this comment

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

Can we set this in a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet. We don't have a backup resource yet to create backups to restore from.
Backup is a bit complex. I have a draft for backup, which I didn't PR yet, since it needs more work. CRUD for backup isn't that complex itself, but the interaction between vault, backups and volumes is a bit complex. I want to make backupConfig parameter of volume work too first (making good progress, may need to update this PR to include it today or tomorrow). Also note that backup tests will extend test runtime for many minutes.

Back to this one: We currently cannot test it. I can remove it for now, but voted to keep it in, since the TF interaction is pretty simple. Specify an valid URN for a backup. There isn't too much which can go wrong in TF here, right?
What is your take: Omit it for now, or leave it in with the small risk that it might fail?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late reply.
Can you test it locally, like create a backup resource not via Terraform and use that for sourceBackup for volume creation via Terraform. I think we can keep it and local testing should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it and it works as expected. They only fact worth mentioning: The API triggers the restore. The LRO returns after the restore job is set up. The new volume resource is being created with state==RESTORING, which will eventually (after minutes/hours/days, depending on the amount of data to restore) will transition to state=READY.
This means the restore operation triggered will run asynchronously and the will not wait for it to complete.

description: |-
Full name of the snapshot to use for creating this volume.
`source_snapshot` and `source_backup` cannot be used simultaneously.
Format: `projects/{{project}}/locations/{{location}}/backupVaults/{{backupVaultId}}/backups/{{backup}}`.
exactly_one_of:
- restore_parameters.0.source_backup
- restore_parameters.0.source_snapshot
immutable: true
- !ruby/object:Api::Type::String
name: 'kmsConfig'
description: |
Expand Down Expand Up @@ -405,17 +457,30 @@ properties:
description: |-
Set the day or days of the month to make a snapshot (1-31). Accepts a comma separated number of days. Defaults to '1'.
default_value: '1'
# This is disabled until we have support for backup resource and can test it.
# - !ruby/object:Api::Type::NestedObject
# name: restoreParameters
# description: Specifies the source information to create a volume from.
# immutable: true
# properties:
# - !ruby/object:Api::Type::String
# name: 'sourceSnapshot'
# description: |-
# Full name of the snapshot resource. Format: `projects/{{project}}/locations/{{location}}/volumes/{{volume}}/snapshots/{{snapshot}}`.
# required: true
# backupConfig is an upcoming feature. Needs more work/testing before release.
# - !ruby/object:Api::Type::NestedObject
# name: 'backupConfig'
# description: |-
# Backup configuration for the volume.
# update_mask_fields:
# - 'backup_config.backup_policies'
# - 'backup_config.backup_vault'
# - 'backup_config.scheduled_backup_enabled'
# properties:
# - !ruby/object:Api::Type::Array
# name: 'backupPolicies'
# description: |-
# Specify a single backup policy ID for scheduled backups. Format: `projects/{{projectId}}/locations/{{location}}/backupPolicies/{{backupPolicyName}}``
# item_type: Api::Type::String
# - !ruby/object:Api::Type::String
# name: 'backupVault'
# description: |-
# ID of the backup vault to use. A backup vault is reqired to create manual or scheduled backups.
# Format: `projects/{{projectId}}/locations/{{location}}/backupVaults/{{backupVaultName}}``
# - !ruby/object:Api::Type::Boolean
# name: 'scheduledBackupEnabled'
# description: |-
# When set to true, scheduled backup is enabled on the volume. Omit if no backup_policy is specified.
virtual_fields:
- !ruby/object:Api::Type::Enum
name: 'deletion_policy'
Expand Down
0 ...es/terraform/examples/volume_basic.tf.erb → ...aform/examples/netapp_volume_basic.tf.erb
View file
Open in desktop
File renamed without changes.
Loading
Loading