Ubernetes-Lite GCE: Label volumes with zone information #19995
Conversation
|
Labelling this PR as size/M |
k8s-github-robot
added
the
size/M
|
@justinsb Thanks! I'll do an initial review now. |
|
GCE e2e test build/test passed for commit 3f8f7e0dcd3478ade3dbcfcf189120f393b965bb. |
| @@ -1644,6 +1645,30 @@ func (gce *GCECloud) DeleteDisk(diskToDelete string) error { | |||
| return gce.waitForZoneOp(deleteOp, disk.Zone) | |||
| } | |||
|
|
|||
| func (gce *GCECloud) GetPDLabels(name string) (map[string]string, error) { | |||
There was a problem hiding this comment.
nit: Worth adding a comment here describing what this does, as it's not entirely clear from the signature. In particular it does not return all of the labels currently applied to the PD. It returns the zone and region labels that we want to apply to the PD. Also, the name param is the name of the PD, not the name of the label, or of anything else.
When volumes are labeled, they will only be scheduled onto nodes in the same zone.
We don't cope well if a PD is in multiple zones, but this is actually fairly easy to detect. This is probably justified purely on the basis that we never want to delete the wrong volume (DeleteDisk), but also because this means that we now warn on creation if a disk is in multiple zones (with the labeling admission controller). This also means that with the scheduling predicate in place, that many of our volume problems "go away" in practice: you still can't create or delete a volume when it is ambiguous, but thereafter the volume will be labeled with the zone, that will match it only to nodes with the same zone, and then we query for the volume in that zone when we attach/detach it.
justinsb
force-pushed
the
gce_label_pd
branch
from
3f8f7e0 to
88eeec4
Compare
|
GCE e2e build/test failed for commit 88eeec4. |
|
@k8s-bot test this Looks like a flake? |
|
@justinsb As discussed offline, I'm not terribly happy about how plugin/pkg/admission/persistentvolume/label/admission.go is morphing from what was presumably intended to be a provider-agnostic generalization into an ugly amalgamation of provider-specific code. But I understand your point that much of this stems from previous decisions, and is not easy to fix in this PR. I'll open a separate issue to track the refactoring issue required to address that. Other than that, this PR LGTM! Thanks! |
ghost
added
the
lgtm
|
GCE e2e test build/test passed for commit 88eeec4. |
|
@k8s-bot test this Tests are more than 48 hours old. Re-running tests. |
|
GCE e2e test build/test passed for commit 88eeec4. |
|
@k8s-bot unit test this |
|
Merging manually to free up some merge queue bandwidth. |
Ubernetes-Lite GCE: Label volumes with zone information
ghost
merged commit 
|
@quinton-hoole Pinging @k8s-oncall for manual merges is probably a better way to go. :) |
| @@ -75,6 +77,13 @@ func (l *persistentVolumeLabel) Admit(a admission.Attributes) (err error) { | |||
| } | |||
| volumeLabels = labels | |||
| } | |||
| if volume.Spec.GCEPersistentDisk != nil { | |||
There was a problem hiding this comment.
@markturansky @saad-ali This should be part of volume plugins, I think. It should NOT be an open-coded list of cases. I'll file a new issue.
There was a problem hiding this comment.
Yes - this came up in review a few lines up ^^^. I agree with the criticism, but it isn't clear to me how best to fix it.
I haven't yet had time to test this, but wanted to share with you asap @quinton-hoole
This completes Ubernetes-Lite support for GCE, by labeling volumes with zones. It also enforces non-ambiguousness when referring to volumes purely by name, which ends up having some very nice properties: from commit 8dbb5c90b1da68c6ed01c770beeceeeedd973907