From bb2843498df6d534741fa7554bc36502dc9c603a Mon Sep 17 00:00:00 2001 From: Danny Jones Date: Wed, 16 Jul 2014 12:32:59 -0700 Subject: [PATCH] API modified to use source; now supports EmptyDirectory API is now modified to use a Source struct to handle multiple volumes. Two volume types are supported now, HostDirectory and EmptyDirectory. --- pkg/api/types.go | 20 +++++++++++++++++--- pkg/api/validation.go | 37 ++++++++++++++++++++++++++++--------- pkg/api/validation_test.go | 13 +++++++------ pkg/kubelet/kubelet.go | 22 ++++++++++++++-------- pkg/kubelet/kubelet_test.go | 10 +++++----- pkg/volume/doc.go | 2 +- pkg/volume/volume.go | 26 ++++++++++++++++++++------ pkg/volume/volume_test.go | 15 ++++++++++++--- 8 files changed, 104 insertions(+), 41 deletions(-) diff --git a/pkg/api/types.go b/pkg/api/types.go index 3ad179e3b0..29dc5fc5a9 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -62,10 +62,22 @@ type Volume struct { // Required: This must be a DNS_LABEL. Each volume in a pod must have // a unique name. Name string `yaml:"name" json:"name"` - // When multiple volume types are supported, only one of them may be specified. + // Source represents the location and type of a volume to mount. + // This is optional for now. If not specified, the Volume is implied to be an EmptyDir. + // This implied behavior is deprecated and will be removed in a future version. + Source *VolumeSource `yaml:"source" json:"source"` +} + +type VolumeSource struct { + // Only one of the following sources may be specified + // HostDirectory represents a pre-existing directory on the host machine that is directly + // exposed to the container. This is generally used for system agents or other privileged + // things that are allowed to see the host machine. Most containers will NOT need this. + // TODO(jonesdl) We need to restrict who can use host directory mounts and + // who can/can not mount host directories as read/write. HostDirectory *HostDirectory `yaml:"hostDir" json:"hostDir"` - // DEPRECATED: If no volume type is specified, HostDirectory will be assumed. - // The path of the directory will be specified in the VolumeMount struct in this case. + // EmptyDirectory represents a temporary directory that shares a pod's lifetime. + EmptyDirectory *EmptyDirectory `yaml:"emptyDir" json:"emptyDir"` } // Bare host directory volume. @@ -73,6 +85,8 @@ type HostDirectory struct { Path string `yaml:"path" json:"path"` } +type EmptyDirectory struct {} + // Port represents a network port in a single container type Port struct { // Optional: If specified, this must be a DNS_LABEL. Each named port diff --git a/pkg/api/validation.go b/pkg/api/validation.go index 92dd44e40b..8b7c76609c 100644 --- a/pkg/api/validation.go +++ b/pkg/api/validation.go @@ -76,25 +76,46 @@ func validateVolumes(volumes []Volume) (util.StringSet, errorList) { allNames := util.StringSet{} for i := range volumes { vol := &volumes[i] // so we can set default values - if vol.HostDirectory != nil { - errs := validateHostDir(vol.HostDirectory) - allErrs.Append(errs...) + errs := errorList{} + // TODO(thockin) enforce that a source is set once we deprecate the implied form. + if vol.Source != nil { + errs = validateSource(vol.Source) } if !util.IsDNSLabel(vol.Name) { - allErrs.Append(makeInvalidError("Volume.Name", vol.Name)) + errs.Append(makeInvalidError("Volume.Name", vol.Name)) } else if allNames.Has(vol.Name) { - allErrs.Append(makeDuplicateError("Volume.Name", vol.Name)) - } else { + errs.Append(makeDuplicateError("Volume.Name", vol.Name)) + } + if len(errs) == 0 { allNames.Insert(vol.Name) + } else { + allErrs.Append(errs...) } } return allNames, allErrs } +func validateSource(source *VolumeSource) errorList { + numVolumes := 0 + allErrs := errorList{} + if source.HostDirectory != nil { + numVolumes++ + allErrs.Append(validateHostDir(source.HostDirectory)...) + } + if source.EmptyDirectory != nil { + numVolumes++ + //EmptyDirs have nothing to validate + } + if numVolumes != 1 { + allErrs.Append(makeInvalidError("Volume.Source", source)) + } + return allErrs +} + func validateHostDir(hostDir *HostDirectory) errorList { allErrs := errorList{} if hostDir.Path == "" { - allErrs.Append(makeNotFoundError("Volume.HostDir.Path", hostDir.Path)) + allErrs.Append(makeNotFoundError("HostDir.Path", hostDir.Path)) } return allErrs } @@ -178,8 +199,6 @@ func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) errorLis if len(mnt.MountType) != 0 { glog.Warning("DEPRECATED: VolumeMount.MountType will be removed. The Volume struct will handle types") } - - } return allErrs } diff --git a/pkg/api/validation_test.go b/pkg/api/validation_test.go index eabd0cba37..e7a3d271e6 100644 --- a/pkg/api/validation_test.go +++ b/pkg/api/validation_test.go @@ -25,15 +25,16 @@ import ( func TestValidateVolumes(t *testing.T) { successCase := []Volume{ - {Name: "abc", HostDirectory: &HostDirectory{"/mnt/path1"}}, - {Name: "123", HostDirectory: &HostDirectory{"/mnt/path2"}}, - {Name: "abc-123", HostDirectory: &HostDirectory{"/mnt/path3"}}, + {Name: "abc"}, + {Name: "123", Source: &VolumeSource{HostDirectory: &HostDirectory{"/mnt/path2"}}}, + {Name: "abc-123", Source: &VolumeSource{HostDirectory: &HostDirectory{"/mnt/path3"}}}, + {Name: "empty", Source: &VolumeSource{EmptyDirectory: &EmptyDirectory{}}}, } names, errs := validateVolumes(successCase) if len(errs) != 0 { t.Errorf("expected success: %v", errs) } - if len(names) != 3 || !names.Has("abc") || !names.Has("123") || !names.Has("abc-123") { + if len(names) != 4 || !names.Has("abc") || !names.Has("123") || !names.Has("abc-123") || !names.Has("empty") { t.Errorf("wrong names result: %v", names) } @@ -206,8 +207,8 @@ func TestValidateManifest(t *testing.T) { { Version: "v1beta1", ID: "abc", - Volumes: []Volume{{Name: "vol1", HostDirectory: &HostDirectory{"/mnt/vol1"}}, - {Name: "vol2", HostDirectory: &HostDirectory{"/mnt/vol2"}}}, + Volumes: []Volume{{Name: "vol1", Source: &VolumeSource{HostDirectory: &HostDirectory{"/mnt/vol1"}}}, + {Name: "vol2", Source: &VolumeSource{HostDirectory: &HostDirectory{"/mnt/vol2"}}}}, Containers: []Container{ { Name: "abc", diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 65e3644d2b..4befbbc5f4 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -77,7 +77,6 @@ type Kubelet struct { HTTPCheckFrequency time.Duration pullLock sync.Mutex HealthChecker health.HealthChecker - extVolumes map[string]volumes.Interface } type manifestUpdate struct { @@ -187,13 +186,15 @@ func makeVolumesAndBinds(manifestID string, container *api.Container, podVolumes binds := []string{} for _, volume := range container.VolumeMounts { var basePath string - if hostVol, ok := podVolumes[volume.Name]; ok { + if vol, ok := podVolumes[volume.Name]; ok { // Host volumes are not Docker volumes and are directly mounted from the host. - basePath = fmt.Sprintf("%s:%s", hostVol.GetPath(), volume.MountPath) + basePath = fmt.Sprintf("%s:%s", vol.GetPath(), volume.MountPath) } else if volume.MountType == "HOST" { - // DEPRECATED: VolumeMount.MountType will be moved to the Volume struct. + // DEPRECATED: VolumeMount.MountType will be handled by the Volume struct. basePath = fmt.Sprintf("%s:%s", volume.MountPath, volume.MountPath) } else { + // TODO(jonesdl) This clause should be deleted and an error should be thrown. The default + // behavior is now supported by the EmptyDirectory type. volumes[volume.MountPath] = struct{}{} basePath = fmt.Sprintf("/exports/%s/%s:%s", manifestID, volume.Name, volume.MountPath) } @@ -251,11 +252,13 @@ func (kl *Kubelet) mountExternalVolumes(manifest *api.ContainerManifest) (volume if err != nil { return nil, err } - podVolumes[vol.Name] = extVolume - // Only prepare the volume if it is not already mounted. - if _, err := os.Stat(extVolume.GetPath()); os.IsNotExist(err) { - extVolume.SetUp() + // TODO(jonesdl) When the default volume behavior is no longer supported, this case + // should never occur and an error should be thrown instead. + if extVolume == nil { + continue } + podVolumes[vol.Name] = extVolume + extVolume.SetUp() } return podVolumes, nil } @@ -581,6 +584,9 @@ func (kl *Kubelet) syncManifest(manifest *api.ContainerManifest, dockerContainer } keepChannel <- netID podVolumes, err := kl.mountExternalVolumes(manifest) + if err != nil { + glog.Errorf("Unable to mount volumes for manifest %s: (%v)", manifest.ID, err) + } for _, container := range manifest.Containers { if dockerContainer, found := dockerContainers.FindPodContainer(manifest.ID, container.Name); found { containerID := DockerID(dockerContainer.ID) diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index 26dae7911d..71bdf0060b 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/pkg/kubelet/kubelet_test.go @@ -535,13 +535,15 @@ func TestMountExternalVolumes(t *testing.T) { Volumes: []api.Volume{ { Name: "host-dir", - HostDirectory: &api.HostDirectory{"/dir/path"}, + Source: &api.VolumeSource{ + HostDirectory: &api.HostDirectory{"/dir/path"}, + }, }, }, } podVolumes, _ := kubelet.mountExternalVolumes(&manifest) - expectedPodVolumes := make(map[string]volumes.Interface) - expectedPodVolumes["host-dir"] = &volumes.HostDirectoryVolume{"/dir/path"} + expectedPodVolumes := make(volumeMap) + expectedPodVolumes["host-dir"] = &volume.HostDirectory{"/dir/path"} if len(expectedPodVolumes) != len(podVolumes) { t.Errorf("Unexpected volumes. Expected %#v got %#v. Manifest was: %#v", expectedPodVolumes, podVolumes, manifest) } @@ -552,8 +554,6 @@ func TestMountExternalVolumes(t *testing.T) { } } - - func TestMakeVolumesAndBinds(t *testing.T) { container := api.Container{ VolumeMounts: []api.VolumeMount{ diff --git a/pkg/volume/doc.go b/pkg/volume/doc.go index e0f8d95904..dc6c5a49bb 100644 --- a/pkg/volume/doc.go +++ b/pkg/volume/doc.go @@ -14,6 +14,6 @@ See the License for the specific language governing permissions and limitations under the License. */ -// Package volumes includes internal representations of external volume types +// Package volume includes internal representations of external volume types // as well as utility methods required to mount/unmount volumes to kubelets. package volume diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 359e1487c9..05a590cbcb 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -21,16 +21,19 @@ import ( "fmt" "os" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/golang/glog" ) // All volume types are expected to implement this interface type Interface interface { // Prepares and mounts/unpacks the volume to a directory path. + // This procedure must be idempotent. SetUp() // Returns the directory path the volume is mounted to. GetPath() string // Unmounts the volume and removes traces of the SetUp procedure. + // This procedure must be idempotent. TearDown() } @@ -52,7 +55,6 @@ func (hostVol *HostDirectory) GetPath() string { // EmptyDirectory volumes are temporary directories exposed to the pod. // These do not persist beyond the lifetime of a pod. - type EmptyDirectory struct { Name string PodID string @@ -60,7 +62,11 @@ type EmptyDirectory struct { // SetUp creates the new directory. func (emptyDir *EmptyDirectory) SetUp() { - os.MkdirAll(emptyDir.GetPath(), 0750) + if _, err := os.Stat(emptyDir.GetPath()); os.IsNotExist(err) { + os.MkdirAll(emptyDir.GetPath(), 0750) + } else { + glog.Warningf("Directory already exists: (%v)", emptyDir.GetPath()) + } } // TODO(jonesdl) when we can properly invoke TearDown(), we should delete @@ -72,6 +78,7 @@ func (emptyDir *EmptyDirectory) GetPath() string { // directory for kubelet to write to. For now this will just be /exports return fmt.Sprintf("/exports/%v/%v", emptyDir.PodID, emptyDir.Name) } + // Interprets API volume as a HostDirectory func createHostDirectory(volume *api.Volume) *HostDirectory { return &HostDirectory{volume.Source.HostDirectory.Path} @@ -83,16 +90,23 @@ func createEmptyDirectory(volume *api.Volume, podID string) *EmptyDirectory { } // CreateVolume returns an Interface capable of mounting a volume described by an -// *api.Volume, or an error. +// *api.Volume and whether or not it is mounted, or an error. func CreateVolume(volume *api.Volume, podID string) (Interface, error) { + source := volume.Source + // TODO(jonesdl) We will want to throw an error here when we no longer + // support the default behavior. + if source == nil { + return nil, nil + } + var vol Interface // TODO(jonesdl) We should probably not check every pointer and directly // resolve these types instead. - source := volume.Source if source.HostDirectory != nil { - return createHostDirectory(volume), nil + vol = createHostDirectory(volume) } else if source.EmptyDirectory != nil { - return createEmptyDirectory(volume, podID), nil + vol = createEmptyDirectory(volume, podID) } else { return nil, errors.New("Unsupported volume type.") } + return vol, nil } diff --git a/pkg/volume/volume_test.go b/pkg/volume/volume_test.go index 625a488438..b28400f5a3 100644 --- a/pkg/volume/volume_test.go +++ b/pkg/volume/volume_test.go @@ -26,12 +26,21 @@ func TestCreateVolumes(t *testing.T) { volumes := []api.Volume{ { Name: "host-dir", - HostDirectory: &api.HostDirectory{"/dir/path"}, + Source: &api.VolumeSource{ + HostDirectory: &api.HostDirectory{"/dir/path"}, + }, + }, + { + Name: "empty-dir", + Source: &api.VolumeSource{ + EmptyDirectory: &api.EmptyDirectory{}, + }, }, } - expectedPaths := []string{"/dir/path"} + fakePodID := "my-id" + expectedPaths := []string{"/dir/path", "/exports/my-id/empty-dir"} for i, volume := range volumes { - extVolume, _ := CreateVolume(&volume) + extVolume, _ := CreateVolume(&volume, fakePodID) expectedPath := expectedPaths[i] path := extVolume.GetPath() if expectedPath != path {