Enable StoragePool #151
Enable StoragePool #151
Conversation
gardener-robot
added
needs/review
gardener-robot-ci-1
added
reviewed/ok-to-test
|
Corresponding extension PR: |
takoverflow
added
reviewed/ok-to-test
gardener-robot-ci-2
added
needs/ok-to-test
pkg/api/v1alpha1/provider_spec.go
Outdated
| // StoragePool in which the new disk is created. | ||
| // You can provide this as a partial or full URL to the resource. For example | ||
| // https://www.googleapis.com/compute/v1/projects/project/zones/zone | ||
| StoragePool string `json:"storagePool,omitempty"` |
There was a problem hiding this comment.
For backward compatibility, shouldn't this field be optional, i.e. of type *string and use it only when it is set?
There was a problem hiding this comment.
The only place where we use this param is here.
And by default, AttachedDiskInitializeParams fields with empty or default values are omitted from API requests.
So unless we set it from outside it will be basically ignored.
There was a problem hiding this comment.
I know this is not real API resource, it is just used for parsing yaml documents which are usually coming from providerConfig somewhere in a shoot manifest. In this case, it could be safe to not use pointers, but generally it would be nice to follow some conventions like https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#new-field-in-existing-api-version.
There was a problem hiding this comment.
Adjusted the PR to use *string instead.
gardener-robot-ci-3
added
reviewed/ok-to-test
kubernetes/machine-class.yaml
Outdated
| # kmsKeyServiceAccount: "[email protected]" # email of service account (optional) | ||
| # provisionedIops: 3000 # IOPS that the disk can handle (optional) | ||
| # provisionedThroughput: 140 # throughput unit in MB per sec (optional) | ||
| # storagePool: zones/zone/storagePools/storagePool # StoragePool where the new disk is created (optional) |
There was a problem hiding this comment.
Are these values fair and sensible defaults?
There was a problem hiding this comment.
Not sure if I interpreted your comment exactly right, but I tweaked the storagePool field in the example to make it a bit clearer.
There was a problem hiding this comment.
Yes, this is clear enough now
Thank you!
gardener-robot-ci-2
added
the
reviewed/ok-to-test
gardener-robot
added
reviewed/lgtm
gardener-robot
added
the
status/closed
8 participants
What this PR does / why we need it:
This PR enables extensions to pass values for the storagePool field in the provider spec.
See also https://cloud.google.com/backup-disaster-recovery/docs/concepts/storage-pools.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: