Fix restoring GCP regional disks #1200
Conversation
pkg/cloudprovider/gcp/block_store.go
Outdated
| if err != nil { | ||
| return "", err | ||
| } | ||
| disk.ReplicaZones = createZoneURLs(b.project, volumeAZ) |
There was a problem hiding this comment.
I think there may be an option to not hardcode the URL template -- you should be able to do something like:
zone, err := b.gce.Zones.Get("<project>", "zone>").Do()
// TODO handle error
url := zone.SelfLink
The url value looks like it will be a relative one, e.g. /projects/<proj>/... -- but I think based on other API documentation that this should be an acceptable value for ReplicaZones. If this works, I think it's probably preferable.
There was a problem hiding this comment.
Cool, thanks. I think I'd prefer that to a templated URL, too. Will give it a test.
There was a problem hiding this comment.
Not 100% sure that the relative URL will be accepted, but 🤞
There was a problem hiding this comment.
It works!
% k get pv -o yaml
apiVersion: v1
items:
- apiVersion: v1
kind: PersistentVolume
labels:
failure-domain.beta.kubernetes.io/region: us-west1
failure-domain.beta.kubernetes.io/zone: us-west1-b__us-west1-c
velero-restore: nginx-regional-20190211133203
velero.io/backup-name: nginx-regional
velero.io/restore-name: nginx-regional-20190211133203
I'll look around how to make the unit testing work with this client call.
There was a problem hiding this comment.
Changed, though I didn't mock out the client seeing as none of the rest of that logic is mocked currently.
There was a problem hiding this comment.
Yeah, I think it's OK for now given precedent. Is this otherwise good to go? I'll do a quick test on it as well - got any sample YAMLs I could use?
There was a problem hiding this comment.
I had to change 2 things to get this to work:
- underscore issue (see below comment)
- add 'compute.zones.get' permission to the IAM role
I think adding the latter as a requirement is fine, but we'll need to update docs and make sure we call it out in the changelog.
pkg/cloudprovider/gcp/block_store.go
Outdated
| // in the failure-domain tag. | ||
| // For example | ||
| // Cluster nodes in us-central1-c, us-central1-f | ||
| // Storage class zones us-central1-a, us-central1-f, us-east1-a, us-east1-d | ||
| // The failure-domain tag would be: us-central1-a__us-central1-f | ||
| func parseRegion(volumeAZ string) (string, error) { | ||
| func parseZone(volumeAZ string) (string, error) { |
There was a problem hiding this comment.
I think this was called parseRegion because it returns a region name (e.g. us-west1), not a zone name. Maybe we just call it getRegion?
There was a problem hiding this comment.
I took it back to parseRegion - I had misunderstood it on reading, and thought it was getting a zone, not a region, but you're correct.
|
Fixes and docs pushed. Will squash if they look good. |
|
retest looks good |
To create a regional disk, the URLs for the zones in which the disk is replicated must be provided to the GCP API. Fixes vmware-tanzu#1183 Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
To create a regional disk, the URLs for the zones in which the disk is
replicated must be provided to the GCP API.
Fixes #1183
Signed-off-by: Nolan Brubaker brubakern@vmware.com