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: