Merge pull request #2585 from SUSE/do-not-create-default-osd

ceph: no longer create fallback osd

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
 
 

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:

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

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

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
 }

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
 }
 

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)
+			}
+		})
+	}
+}

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: