From 78d471912518309b714de305968f87e68f2c7939 Mon Sep 17 00:00:00 2001 From: Blaine Gardner Date: Mon, 28 Jan 2019 17:39:10 -0700 Subject: [PATCH] ceph: no longer create fallback osd Rook's behavior should not be to create a default OSD in dataDirHostPath when no devices are present on a node. Any preexisting fallback osd should be kept as long as no dirs or disks have been specified to keep the legacy behavior for those clusters running with these osds in place. The legacy deletion behavior is also kept, removing the osd as soon as any dir or device is specified other than the dataDirHostPath dir. Signed-off-by: Blaine Gardner --- PendingReleaseNotes.md | 4 ++ cluster/examples/kubernetes/ceph/cluster.yaml | 8 +-- pkg/daemon/ceph/osd/daemon.go | 21 +++++--- pkg/daemon/ceph/osd/daemon_test.go | 5 +- pkg/operator/ceph/cluster/osd/osd.go | 11 ---- pkg/operator/k8sutil/volume.go | 36 ++++++++++--- pkg/operator/k8sutil/volume_test.go | 52 +++++++++++++++++++ tests/framework/installer/ceph_manifests.go | 2 + 8 files changed, 110 insertions(+), 29 deletions(-) create mode 100644 pkg/operator/k8sutil/volume_test.go diff --git a/PendingReleaseNotes.md b/PendingReleaseNotes.md index a2eaf51c9..10b330308 100644 --- a/PendingReleaseNotes.md +++ b/PendingReleaseNotes.md @@ -22,6 +22,10 @@ - Rook no longer supports Kubernetes `1.8` and `1.9`. +### Ceph +- Rook will no longer create a directory-based osd in the `dataDirHostPath` if no directories or + devices are specified or if there are no disks on the host. + ## Known Issues diff --git a/cluster/examples/kubernetes/ceph/cluster.yaml b/cluster/examples/kubernetes/ceph/cluster.yaml index 48cfeeb41..cbafb1429 100755 --- a/cluster/examples/kubernetes/ceph/cluster.yaml +++ b/cluster/examples/kubernetes/ceph/cluster.yaml @@ -166,7 +166,7 @@ spec: # After nautilus is released, Rook will be updated to support nautilus. # Do not set to true in production. allowUnsupported: false - # The path on the host where configuration files will be persisted. If not specified, a kubernetes emptyDir will be created (not recommended). + # The path on the host where configuration files will be persisted. Must be specified. # Important: if you reinstall the cluster, make sure you delete this directory from each host or else the mons will fail to start on the new cluster. # In Minikube, the '/data' directory is configured to persist across reboots. Use "/data/rook" in Minikube environment. dataDirHostPath: /var/lib/rook @@ -237,8 +237,10 @@ spec: journalSizeMB: "1024" # this value can be removed for environments with normal sized disks (20 GB or larger) osdsPerDevice: "1" # this value can be overridden at the node or device level # Cluster level list of directories to use for storage. These values will be set for all nodes that have no `directories` set. -# directories: -# - path: /rook/storage-dir + directories: + # By default create a osd in the dataDirHostPath directory. This should be removed for + # environments where nodes have disks available for Rook to use. + - path: /var/lib/rook # Individual nodes and their config can be specified as well, but 'useAllNodes' above must be set to false. Then, only the named # nodes below will be used as storage resources. Each node's 'name' field should match their 'kubernetes.io/hostname' label. # nodes: diff --git a/pkg/daemon/ceph/osd/daemon.go b/pkg/daemon/ceph/osd/daemon.go index 152ccd581..1cc517dae 100644 --- a/pkg/daemon/ceph/osd/daemon.go +++ b/pkg/daemon/ceph/osd/daemon.go @@ -250,10 +250,10 @@ func getDataDirs(context *clusterd.Context, kv *k8sutil.ConfigMapKVStore, desire dirList = strings.Split(desiredDirs, ",") } - if len(dirList) == 0 && !devicesSpecified { - // user has not specified any dirs or any devices, give them the default dir at least - dirList = append(dirList, context.ConfigDir) - } + // when user has not specified any dirs or any devices, legacy behavior was to give them the + // default dir. no longer automatically create this fallback osd. the legacy conditional is + // still important for determining when the fallback osd may be deleted. + noDirsOrDevicesSpecified := len(dirList) == 0 && !devicesSpecified removedDirs = make(map[string]int) @@ -263,7 +263,7 @@ func getDataDirs(context *clusterd.Context, kv *k8sutil.ConfigMapKVStore, desire addDirsToDirMap(dirList, &dirMap) // determine which dirs are still active, which should be removed, then return them - activeDirs, removedDirs := getActiveAndRemovedDirs(dirList, dirMap) + activeDirs, removedDirs := getActiveAndRemovedDirs(dirList, dirMap, context.ConfigDir, noDirsOrDevicesSpecified) return activeDirs, removedDirs, nil } @@ -324,12 +324,21 @@ func getRemovedDevices(agent *OsdAgent) (*config.PerfScheme, *DeviceOsdMapping, return removedDevicesScheme, removedDevicesMapping, nil } -func getActiveAndRemovedDirs(currentDirList []string, savedDirMap map[string]int) (activeDirs, removedDirs map[string]int) { +func getActiveAndRemovedDirs( + currentDirList []string, savedDirMap map[string]int, configDir string, noDirsOrDevicesSpecified bool, +) (activeDirs, removedDirs map[string]int) { activeDirs = map[string]int{} removedDirs = map[string]int{} for savedDir, id := range savedDirMap { foundSavedDir := false + + // If a legacy 'fallback' osd and no dirs/devices are yet specified, keep it to preserve + // legacy behavior for migrated clusters. + if savedDir == configDir && noDirsOrDevicesSpecified { + foundSavedDir = true + } + for _, dir := range currentDirList { if dir == savedDir { foundSavedDir = true diff --git a/pkg/daemon/ceph/osd/daemon_test.go b/pkg/daemon/ceph/osd/daemon_test.go index fc3851a5d..43ac9ac8e 100644 --- a/pkg/daemon/ceph/osd/daemon_test.go +++ b/pkg/daemon/ceph/osd/daemon_test.go @@ -90,11 +90,10 @@ func TestGetDataDirs(t *testing.T) { assert.Equal(t, 0, len(dirMap)) assert.Equal(t, 0, len(removedDirMap)) - // user has no devices specified, should return default dir + // user has no devices specified, should NO LONGER return default dir dirMap, removedDirMap, err = getDataDirs(context, kv, "", false, nodeName) assert.Nil(t, err) - assert.Equal(t, 1, len(dirMap)) - assert.Equal(t, unassignedOSDID, dirMap[context.ConfigDir]) + assert.Equal(t, 0, len(dirMap)) assert.Equal(t, 0, len(removedDirMap)) // user has no devices specified but does specify dirs, those should be returned diff --git a/pkg/operator/ceph/cluster/osd/osd.go b/pkg/operator/ceph/cluster/osd/osd.go index 804bef85c..d32d21949 100644 --- a/pkg/operator/ceph/cluster/osd/osd.go +++ b/pkg/operator/ceph/cluster/osd/osd.go @@ -539,16 +539,5 @@ func (c *Cluster) resolveNode(nodeName string) *rookalpha.Node { } rookNode.Resources = k8sutil.MergeResourceRequirements(rookNode.Resources, c.resources) - // ensure no invalid dirs are specified - var validDirs []rookalpha.Directory - for _, dir := range rookNode.Directories { - if dir.Path == k8sutil.DataDir || dir.Path == c.dataDirHostPath { - logger.Warningf("skipping directory %s that would conflict with the dataDirHostPath", dir.Path) - continue - } - validDirs = append(validDirs, dir) - } - rookNode.Directories = validDirs - return rookNode } diff --git a/pkg/operator/k8sutil/volume.go b/pkg/operator/k8sutil/volume.go index 74b791052..255bc6590 100644 --- a/pkg/operator/k8sutil/volume.go +++ b/pkg/operator/k8sutil/volume.go @@ -20,10 +20,10 @@ package k8sutil import ( "fmt" "os" - "path/filepath" "strings" "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/validation" ) const ( @@ -34,16 +34,40 @@ const ( func PathToVolumeName(path string) string { // kubernetes volume names must match this regex: [a-z0-9]([-a-z0-9]*[a-z0-9])? - // first replace all filepath separators with hyphens - volumeName := strings.Replace(path, string(filepath.Separator), "-", -1) - - // convert underscores to hyphens - volumeName = strings.Replace(volumeName, "_", "-", -1) + sanitized := make([]rune, 0, len(path)) + for _, c := range path { + switch { + case '0' <= c && c <= '9': + fallthrough + case 'a' <= c && c <= 'z': + sanitized = append(sanitized, c) + continue + case 'A' <= c && c <= 'Z': + // convert upper to lower case + sanitized = append(sanitized, c+('a'-'A')) + default: + // convert any non alphanum char to a hyphen + sanitized = append(sanitized, '-') + } + } + volumeName := string(sanitized) // trim any leading/trailing hyphens volumeName = strings.TrimPrefix(volumeName, "-") volumeName = strings.TrimSuffix(volumeName, "-") + if len(volumeName) > validation.DNS1123LabelMaxLength { + // keep an equal sample of the original name from both the beginning and from the end, + // and add some characters from a hash of the full name to help prevent name collisions. + // Make room for 3 hyphens in the middle (~ellipsis) and 1 hyphen to separate the hash. + hashLength := 8 + sampleLength := int((validation.DNS1123LabelMaxLength - hashLength - 3 - 1) / 2) + first := volumeName[0:sampleLength] + last := volumeName[len(volumeName)-sampleLength:] + hash := Hash(volumeName) + volumeName = fmt.Sprintf("%s---%s-%s", first, last, hash[0:hashLength]) + } + return volumeName } diff --git a/pkg/operator/k8sutil/volume_test.go b/pkg/operator/k8sutil/volume_test.go new file mode 100644 index 000000000..d6c73f251 --- /dev/null +++ b/pkg/operator/k8sutil/volume_test.go @@ -0,0 +1,52 @@ +/* +Copyright 2016 The Rook Authors. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package k8sutil for Kubernetes helpers. +package k8sutil + +import "testing" + +func TestPathToVolumeName(t *testing.T) { + tests := []struct { + name string // test name + path string // argument + want string + }{ + {"simple", "simple", "simple"}, + {"preceding slash", "/preceding", "preceding"}, + {"trailing slash", "trailing/", "trailing"}, + {"convert to lower case", "ASDFGHJKLQWERTYUIOPZXCVBNM", "asdfghjklqwertyuiopzxcvbnm"}, + {"preserve nums", "0123456789", "0123456789"}, + {"preserve lower case", "qwertyuiopasdfghjklzxcvbnm", "qwertyuiopasdfghjklzxcvbnm"}, + {"convert any non-lower-case-alphanum symbols to dash", + "a/.;,=[]_~!@`#$%^&*()_+-<>?:\"\\'|}{z", // symbols on U.S. keyboard + "a---------------------------------z"}, + {"various currency symbols", // only those written left-to-right + "z£¢©®™¥€§฿₽₨₱¤₡₫ƒ₲₴₭č₾֏₣лвдине₤₺₼₥₦₱៛₹₪৳₸₩ła", + "z------------------------------------------a"}, + {"full-width symbols", "q￠￥￦℃℉p", "q-----p"}, // only those written left-to-right + {"longer than 63 chars", // if you change the arg string, the hash on the end will change + "/this/is/some-path/.that/is_longer/than/$63/chars/1234567890/and/i'm/still/typing", + "this-is-some-path--that-i---7890-and-i-m-still-typing-c4132749"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := PathToVolumeName(tt.path); got != tt.want { + t.Errorf("PathToVolumeName() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/tests/framework/installer/ceph_manifests.go b/tests/framework/installer/ceph_manifests.go index 50d2eadc6..c9de26f8e 100644 --- a/tests/framework/installer/ceph_manifests.go +++ b/tests/framework/installer/ceph_manifests.go @@ -661,6 +661,8 @@ spec: storage: useAllNodes: true useAllDevices: ` + strconv.FormatBool(settings.UseAllDevices) + ` + directories: + - path: ` + settings.DataDirHostPath + /* simulate legacy fallback osd behavior so existing tests still work */ ` deviceFilter: location: config: