-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
StorageOS Volume Plugin #42156
StorageOS Volume Plugin #42156
Conversation
Hi @croomes. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
I'll rebase this as soon as I get back from DockerCon |
/assign |
@k8s-bot ok to test |
pkg/api/types.go
Outdated
// namespace is specified, the namespace of the pod will be used. Set to | ||
// "default" if you are not using namespaces within StorageOS. | ||
// +optional | ||
Namespace string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need NameSpace in volume source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use namespaces within StorageOS too, so by default it will use the Pod's namespace but you can force it not to use StorageOS namespace's at all by setting to "default". You can also specify a different StorageOS namespace here. Normally the StorageOS namespaces will mirror Kubernetes'.
pkg/volume/storageos/doc.go
Outdated
@@ -0,0 +1,19 @@ | |||
/* | |||
Copyright 2015 The Kubernetes Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,333 @@ | |||
/* | |||
Copyright 2016 The Kubernetes Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2017
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/volume/storageos/storageos.go
Outdated
podUID: pod.UID, | ||
pvName: spec.Name(), | ||
volName: source.VolumeName, | ||
namespace: source.Namespace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can get namespace from Pod pod.Namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As comment above, this is the volume namespace and could be different from the Pod namespace.
pkg/volume/storageos/storageos.go
Outdated
storageos: &storageos{ | ||
pvName: spec.Name(), | ||
volName: spec.PersistentVolume.Spec.StorageOS.VolumeName, | ||
namespace: spec.PersistentVolume.Spec.StorageOS.Namespace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can get namespace from PV's ClaimRef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the volume's namespace, not necessarily the Pod's. Starting to think I should rename it volNamespace to make it more clear - what do you think?
pkg/volume/storageos/storageos.go
Outdated
|
||
// Set parameters from StorageClass | ||
if len(v.options.Parameters) > 0 { | ||
if description, ok := v.options.Parameters["description"]; ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iterate parameters and use switch/case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much better - done
pkg/volume/storageos/storageos.go
Outdated
v.volName = v.options.PVName | ||
v.namespace = v.options.PVC.Namespace | ||
v.labels = make(map[string]string) | ||
for k, val := range v.options.PVC.Labels { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why setting labels here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We copy the labels from the PVC to the requested volume. StorageOS uses the labels from k8s to set policy. Fox example, if the PVC has environment=production set, a policy rule within StorageOS might set the number of replicas to 2.
pv.Labels = make(map[string]string) | ||
} | ||
for k, v := range vol.Labels { | ||
pv.Labels[k] = v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passing labels from PVC to PV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is setting labels created by StorageOS when the volume is provisioned and applying them to the PV. So in the example above, the label storageos.feature.replicas=2
would be set on the PV. These labels are only informational.
Hi @rootfs - thanks for the review. I'm updating and will reply to the remaining points soon. |
f8e244a
to
dda964f
Compare
@@ -251,7 +251,7 @@ func (g *Graph) AddPV(pv *api.PersistentVolume) { | |||
|
|||
// since we don't know the other end of the pvc -> pod -> node chain (or it may not even exist yet), we can't decorate these edges with kubernetes node info | |||
g.graph.SetEdge(simple.Edge{F: pvVertex, T: g.getOrCreateVertex_locked(pvcVertexType, pv.Spec.ClaimRef.Namespace, pv.Spec.ClaimRef.Name)}) | |||
pvutil.VisitPVSecretNames(pv, func(secret string) bool { | |||
pvutil.VisitPVSecretNames(pv, func(namespace, secret string) bool { | |||
// This grants access to the named secret in the same namespace as the bound PVC | |||
g.graph.SetEdge(simple.Edge{F: g.getOrCreateVertex_locked(secretVertexType, pv.Spec.ClaimRef.Namespace, secret), T: pvVertex}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the provided namespace here instead of pv.Spec.ClaimRef.Namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
for _, pv := range pvs { | ||
VisitPVSecretNames(pv, func(name string) bool { | ||
VisitPVSecretNames(pv, func(namespace, name string) bool { | ||
fmt.Printf("%s,%s\n", namespace, name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop the printf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops - done
pkg/volume/storageos/storageos.go
Outdated
|
||
func (plugin *storageosPlugin) NewMounter(spec *volume.Spec, pod *v1.Pod, _ volume.VolumeOptions) (volume.Mounter, error) { | ||
|
||
source, _, err := getPersistentVolumeSource(spec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this mean the storageos plugin would only support persistentvolumes, not a storageos volume source specified in a pod spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - this is a mistake. I added the code to detect both in newMounterInternal but not NewMounter (my tests were failing on it). I will refactor a bit and add to both. I couldn't think of a better way to handle both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this is part of the ripple of not being able to share a single volume source type :-/
pkg/volume/storageos/storageos.go
Outdated
} | ||
|
||
var apiCfg *storageosAPIConfig | ||
if source.SecretRef != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expected two paths here, one using secret name/namespace for a PV source, one using secret name and pod namespace for a pod source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, you can do this inside newMounterInternal, where you already have two branches
@dchen1107 held the v1.7 exceptions meeting today. She left it up to @liggitt and I to decide if this PR gets in for 1.7 or not. @liggitt and I just had a meeting to discuss the current revision of this PR where the VolumeSource for this plugin is split between a PV (with So overall we are fine with this approach. @liggitt and I will both take a final pass reviewing this PR. In the meantime, @croomes please test all the paths for this code (provisioning, mounting, unmounting, deleting, etc.). Once we have a thumbs up from you on the testing front and no further feedback, we will approve/LGTM this for merge (Friday at the latest). |
Fantastic, thanks! I'm going to do another push in 30 mins that should fix the last comments, but I will spend the day tomorrow double-checking everything. I've re-run our basic acceptance tests and they pass for PVs and volumes in Pod specs and dynamic provisioning. I need to do more with the various secret namespace combinations. To help review latest, I've only changed NewMounter and newMounterInternal, and changed validation.go to return proper path to secretRef name/namespace: fldPath.Child("secretRef", "namespace") Really appreciate all the time you've both put into helping me get this in. 👍 |
pkg/volume/storageos/storageos.go
Outdated
} | ||
|
||
var apiCfg *storageosAPIConfig | ||
if source.SecretRef != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, you can do this inside newMounterInternal, where you already have two branches
pkg/volume/storageos/storageos.go
Outdated
capacity := c.options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)] | ||
c.sizeGB = int(volume.RoundUpSize(capacity.Value(), 1024*1024*1024)) | ||
|
||
if adminSecretName == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| adminSecretNamespace == ""
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, does requiring this mean you can no longer use default storageos credentials?
pkg/volume/storageos/storageos.go
Outdated
|
||
// parseSecret will lookup secret in secretNamespace if set, otherwise default | ||
// to pod namespace. | ||
func parseSecret(secretNamespace, secretName string, pod *v1.Pod, kubeClient clientset.Interface) (*storageosAPIConfig, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think parseSecret() should exist... we should either call parsePodSecret() or parsePVSecret() directly, based on whether the source of the data was a PV or a Pod
pkg/volume/storageos/storageos.go
Outdated
|
||
func (plugin *storageosPlugin) NewMounter(spec *volume.Spec, pod *v1.Pod, _ volume.VolumeOptions) (volume.Mounter, error) { | ||
|
||
secretNamespace, secretName, err := getSecretInfoFromSpec(spec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than spreading this logic between three functions, and inferring the source was a PV because namespace == "" in parseSecret
, it would be much more straightforward to combine getSecretInfoFromSpec and parseSecret
func getAPICfg(spec *volume.Spec, pod *v1.Pod, kubeClient clientset.Interface) (*storageosAPIConfig, error) {
if spec.PersistentVolume != nil {
source, _, err := getPersistentVolumeSource(spec)
if err != nil {
return nil, err
}
if source.SecretRef == nil {
// not sure whether you require apiCfg in this case
// would you expect to fall back to default values, or return an error?
return nil, nil
}
return parsePVSecret(source.SecretRef.Namespace, source.SecretRef.Name, kubeClient)
}
if spec.Volume != nil {
source, _, err := getVolumeSource(spec)
if err != nil {
return nil, err
}
if source.SecretRef == nil {
// not sure whether you require apiCfg in this case
// would you expect to fall back to default values, or return an error?
return nil, nil
}
return parsePodSecret(pod, source.SecretRef.Name, kubeClient)
}
return nil, fmt.Errorf("spec not Volume or PersistentVolume")
}
pkg/api/persistentvolume/util.go
Outdated
@@ -20,34 +20,46 @@ import ( | |||
"k8s.io/kubernetes/pkg/api" | |||
) | |||
|
|||
func getNamespace(pv *api.PersistentVolume) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: rename this to getClaimRefNamespace
pkg/api/validation/validation.go
Outdated
if len(storageos.VolumeNamespace) > 0 { | ||
allErrs = append(allErrs, ValidateDNS1123Label(storageos.VolumeNamespace, fldPath.Child("volumeNamespace"))...) | ||
} | ||
return allErrs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if secretRef != nil, ensure a name is set?
pkg/api/validation/validation.go
Outdated
if len(storageos.VolumeNamespace) > 0 { | ||
allErrs = append(allErrs, ValidateDNS1123Label(storageos.VolumeNamespace, fldPath.Child("volumeNamespace"))...) | ||
} | ||
if storageos.SecretRef != nil && (len(storageos.SecretRef.Name) > 0 || len(storageos.SecretRef.Namespace) > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if secretRef != nil, I'd expect the name and namespace to be set... a non-nil secretRef with empty data seems wrong
one comment about combining getSecretInfoFromSpec and parseSecret to make it very clear that parsePVSecret() is only called with data from a PV, and a couple validation questions. the graph and pv secret util changes LGTM |
I've made the changes suggested by @liggitt (good ideas), and we've done a lot of testing. Functionally we're happy, but I want to make one final pass on the examples and README to make sure we didn't miss anything. I'll have it rebased and ready to go for start of day (PT) Friday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
// StorageOS represents a StorageOS volume that is attached to the kubelet's host machine and mounted into the pod | ||
// More info: https://releases.k8s.io/HEAD/examples/volumes/storageos/README.md | ||
// +optional | ||
StorageOS *StorageOSPersistentVolumeSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe add a comment indicating why it is not StorageOSVolumeSource
.
👍 It's like another election night - the team is in the pub waiting for the votes to come in. Big cheer from over here - thanks. |
Automatic merge from submit-queue |
What this PR does / why we need it:
This PR adds a new volume plugin for StorageOS volumes. StorageOS runs as a container on Kubelet nodes, aggregating local or attached storage and making its capacity available to all nodes within the cluster. More information at http://storageos.com.
The StorageOS plugin supports:
Which issue this PR fixes
A feature request has been created:
kubernetes/enhancements#190
This isn't on the schedule for 1.6 as I wasn't sure when it would be ready. We intend to make the StorageOS container openly available within the 1.6 lifetime.
Special notes for your reviewer:
Separate commits for feature and godep changes.
Release note: