Issue 59: Fixing Bookie failure (#60)

* Issue 58: Fixing Bookie failure Signed-off-by: SrishT <[email protected]> * Issue 59: Fixing Bookie failure Signed-off-by: SrishT <[email protected]> * Issue 59: Fixing Bookie failure Signed-off-by: SrishT <[email protected]> * Issue 59: Addressing review comments Signed-off-by: SrishT <[email protected]> * Issue 59: Increasing UT coverage Signed-off-by: SrishT <[email protected]> Co-authored-by: SrishT <[email protected]>
pravega · Jul 8, 2020 · 0fd31cc · 0fd31cc
1 parent e138886
commit 0fd31cc
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 42 deletions.
diff --git a/charts/bookkeeper/README.md b/charts/bookkeeper/README.md
@@ -46,17 +46,17 @@ The following table lists the configurable parameters of the Bookkeeper chart an
 | `zookeeperUri` | Zookeeper client service URI | `zookeeper-client:2181` |
 | `pravegaClusterName` | Name of the pravega cluster | `pravega` |
 | `autoRecovery`| Enable bookkeeper auto-recovery | `true` |
-| `resources.requests.cpu` | Requests for CPU resources | `500m` |
-| `resources.requests.memory` | Requests for memory resources | `1Gi` |
-| `resources.limits.cpu` | Limits for CPU resources | `1` |
-| `resources.limits.memory` | Limits for memory resources | `2Gi` |
+| `resources.requests.cpu` | Requests for CPU resources | `1000m` |
+| `resources.requests.memory` | Requests for memory resources | `4Gi` |
+| `resources.limits.cpu` | Limits for CPU resources | `2000m` |
+| `resources.limits.memory` | Limits for memory resources | `4Gi` |
 | `storage.ledger.className` | Storage class name for bookkeeper ledgers | `standard` |
 | `storage.ledger.volumeSize` | Requested size for bookkeeper ledger persistent volumes | `10Gi` |
 | `storage.journal.className` | Storage class name for bookkeeper journals | `standard` |
 | `storage.journal.volumeSize` | Requested size for bookkeeper journal persistent volumes | `10Gi` |
 | `storage.index.className` | Storage class name for bookkeeper index | `standard` |
 | `storage.index.volumeSize` | Requested size for bookkeeper index persistent volumes | `10Gi` |
-| `jvmOptions.memoryOpts` | Memory Options passed to the JVM for bookkeeper performance tuning | `[]` |
+| `jvmOptions.memoryOpts` | Memory Options passed to the JVM for bookkeeper performance tuning | `["-Xms1g", "-XX:MaxDirectMemorySize=2g"]` |
 | `jvmOptions.gcOpts` | Garbage Collector (GC) Options passed to the JVM for bookkeeper bookkeeper performance tuning | `[]` |
 | `jvmOptions.gcLoggingOpts` | GC Logging Options passed to the JVM for bookkeeper performance tuning | `[]` |
 | `jvmOptions.extraOpts` | Extra Options passed to the JVM for bookkeeper performance tuning | `[]` |

diff --git a/charts/bookkeeper/values.yaml b/charts/bookkeeper/values.yaml
@@ -21,11 +21,11 @@ autoRecovery: true
 
 resources:
   requests:
-    cpu: 500m
-    memory: 1Gi
+    cpu: 1000m
+    memory: 4Gi
   limits:
-    cpu: 1
-    memory: 2Gi
+    cpu: 2000m
+    memory: 4Gi
 
 storage:
   ledger:
@@ -39,10 +39,26 @@ storage:
     volumeSize: 10Gi
 
 jvmOptions:
-  memoryOpts: []
+  memoryOpts: ["-Xms1g", "-XX:MaxDirectMemorySize=2g"]
   gcOpts: []
   gcLoggingOpts: []
   extraOpts: []
 
 options:
-  # useHostNameAsBookieID: "true"
+  useHostNameAsBookieID: "true"
+  ## We need an agressive data compaction policy in Bookkeeper, given that we may have IO heavy workloads that may fill up the disks.
+  ## For more information on these parameters, please see https://github.com/pravega/pravega/issues/4008.
+  minorCompactionThreshold: "0.4"
+  minorCompactionInterval: "1800"
+  majorCompactionThreshold: "0.8"
+  majorCompactionInterval: "43200"
+  isForceGCAllowWhenNoSpace: "true"
+  ## Use multiple journal and ledger directories to try exploiting more parallelism at the drive level.
+  journalDirectories: "/bk/journal/j0,/bk/journal/j1,/bk/journal/j2,/bk/journal/j3"
+  ledgerDirectories: "/bk/ledgers/l0,/bk/ledgers/l1,/bk/ledgers/l2,/bk/ledgers/l3"
+  ## We have validated that this simpler ledger type prevents Bookie restarts due to heap OOM compared to the default SortedLedgerStorage.
+  ## As we do not read from Bookkeeper (only during container recovery), this ledger type looks more efficient given our requirements.
+  ledgerStorageClass: "org.apache.bookkeeper.bookie.InterleavedLedgerStorage"
+  ## Only use these parameters if you want Bookkeeper to publish metrics (via Prometheus).
+  # enableStatistics: "true"
+  # statsProviderClass: "org.apache.bookkeeper.stats.prometheus.PrometheusMetricsProvider"
diff --git a/pkg/controller/bookkeepercluster/bookie.go b/pkg/controller/bookkeepercluster/bookie.go
@@ -12,6 +12,7 @@ package bookkeepercluster
 
 import (
 	"fmt"
+	"strconv"
 	"strings"
 
 	"github.com/pravega/bookkeeper-operator/pkg/apis/bookkeeper/v1alpha1"
@@ -114,20 +115,28 @@ func makeBookiePodSpec(bk *v1alpha1.BookkeeperCluster) *corev1.PodSpec {
 		})
 	}
 
-	var ledgerDirs, journalDirs, indexDirs string
+	var ledgerDirs, journalDirs, indexDirs []string
 	var ok bool
 
-	if ledgerDirs, ok = bk.Spec.Options["ledgerDirectories"]; !ok {
+	if _, ok = bk.Spec.Options["ledgerDirectories"]; ok {
+		ledgerDirs = strings.Split(bk.Spec.Options["ledgerDirectories"], ",")
+	} else {
 		// default value if user did not set ledgerDirectories in options
-		ledgerDirs = "/bk/ledgers"
+		ledgerDirs = append(ledgerDirs, "/bk/ledgers")
 	}
-	if journalDirs, ok = bk.Spec.Options["journalDirectories"]; !ok {
+
+	if _, ok = bk.Spec.Options["journalDirectories"]; ok {
+		journalDirs = strings.Split(bk.Spec.Options["journalDirectories"], ",")
+	} else {
 		// default value if user did not set journalDirectories in options
-		journalDirs = "/bk/journal"
+		journalDirs = append(journalDirs, "/bk/journal")
 	}
-	if indexDirs, ok = bk.Spec.Options["indexDirectories"]; !ok {
+
+	if _, ok = bk.Spec.Options["indexDirectories"]; ok {
+		indexDirs = strings.Split(bk.Spec.Options["indexDirectories"], ",")
+	} else {
 		// default value if user did not set indexDirectories in options
-		indexDirs = "/bk/index"
+		indexDirs = append(indexDirs, "/bk/index")
 	}
 
 	podSpec := &corev1.PodSpec{
@@ -142,26 +151,9 @@ func makeBookiePodSpec(bk *v1alpha1.BookkeeperCluster) *corev1.PodSpec {
 						ContainerPort: 3181,
 					},
 				},
-				EnvFrom: environment,
-				VolumeMounts: []corev1.VolumeMount{
-					{
-						Name:      LedgerDiskName,
-						MountPath: ledgerDirs,
-					},
-					{
-						Name:      JournalDiskName,
-						MountPath: journalDirs,
-					},
-					{
-						Name:      IndexDiskName,
-						MountPath: indexDirs,
-					},
-					{
-						Name:      heapDumpName,
-						MountPath: heapDumpDir,
-					},
-				},
-				Resources: *bk.Spec.Resources,
+				EnvFrom:      environment,
+				VolumeMounts: createVolumeMount(ledgerDirs, journalDirs, indexDirs),
+				Resources:    *bk.Spec.Resources,
 				ReadinessProbe: &corev1.Probe{
 					Handler: corev1.Handler{
 						Exec: &corev1.ExecAction{
@@ -207,6 +199,38 @@ func makeBookiePodSpec(bk *v1alpha1.BookkeeperCluster) *corev1.PodSpec {
 	return podSpec
 }
 
+func createVolumeMount(ledgerDirs []string, journalDirs []string, indexDirs []string) []corev1.VolumeMount {
+	var volumeMounts []corev1.VolumeMount
+	for i, ledger := range ledgerDirs {
+		name := LedgerDiskName + strconv.Itoa(i)
+		v := corev1.VolumeMount{
+			Name:      LedgerDiskName,
+			MountPath: ledger,
+			SubPath:   name,
+		}
+		volumeMounts = append(volumeMounts, v)
+	}
+	for i, journal := range journalDirs {
+		name := JournalDiskName + strconv.Itoa(i)
+		v := corev1.VolumeMount{
+			Name:      JournalDiskName,
+			MountPath: journal,
+			SubPath:   name,
+		}
+		volumeMounts = append(volumeMounts, v)
+	}
+	for i, index := range indexDirs {
+		name := IndexDiskName + strconv.Itoa(i)
+		v := corev1.VolumeMount{
+			Name:      IndexDiskName,
+			MountPath: index,
+			SubPath:   name,
+		}
+		volumeMounts = append(volumeMounts, v)
+	}
+	return volumeMounts
+}
+
 func makeBookieVolumeClaimTemplates(bk *v1alpha1.BookkeeperCluster) []corev1.PersistentVolumeClaim {
 	return []corev1.PersistentVolumeClaim{
 		{

diff --git a/pkg/controller/bookkeepercluster/bookie_test.go b/pkg/controller/bookkeepercluster/bookie_test.go
@@ -67,6 +67,7 @@ var _ = Describe("Bookie", func() {
 					Options: map[string]string{
 						"journalDirectories": "/bk/journal/j0,/bk/journal/j1,/bk/journal/j2,/bk/journal/j3",
 						"ledgerDirectories":  "/bk/ledgers/l0,/bk/ledgers/l1,/bk/ledgers/l2,/bk/ledgers/l3",
+						"indexDirectories":   "/bk/index/i0,/bk/index/i1",
 					},
 				}
 				bk.WithDefaults()
@@ -94,10 +95,26 @@ var _ = Describe("Bookie", func() {
 
 				It("should have journal and ledgers dir set to the values given by user", func() {
 					sts := bookkeepercluster.MakeBookieStatefulSet(bk)
-					mountledger := sts.Spec.Template.Spec.Containers[0].VolumeMounts[0].MountPath
-					Ω(mountledger).Should(Equal("/bk/ledgers/l0,/bk/ledgers/l1,/bk/ledgers/l2,/bk/ledgers/l3"))
-					mountjournal := sts.Spec.Template.Spec.Containers[0].VolumeMounts[1].MountPath
-					Ω(mountjournal).Should(Equal("/bk/journal/j0,/bk/journal/j1,/bk/journal/j2,/bk/journal/j3"))
+					mountledger0 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[0].MountPath
+					mountledger1 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[1].MountPath
+					mountledger2 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[2].MountPath
+					mountledger3 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[3].MountPath
+					Ω(mountledger0).Should(Equal("/bk/ledgers/l0"))
+					Ω(mountledger1).Should(Equal("/bk/ledgers/l1"))
+					Ω(mountledger2).Should(Equal("/bk/ledgers/l2"))
+					Ω(mountledger3).Should(Equal("/bk/ledgers/l3"))
+					mountjournal0 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[4].MountPath
+					mountjournal1 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[5].MountPath
+					mountjournal2 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[6].MountPath
+					mountjournal3 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[7].MountPath
+					Ω(mountjournal0).Should(Equal("/bk/journal/j0"))
+					Ω(mountjournal1).Should(Equal("/bk/journal/j1"))
+					Ω(mountjournal2).Should(Equal("/bk/journal/j2"))
+					Ω(mountjournal3).Should(Equal("/bk/journal/j3"))
+					mountindex0 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[8].MountPath
+					mountindex1 := sts.Spec.Template.Spec.Containers[0].VolumeMounts[9].MountPath
+					Ω(mountindex0).Should(Equal("/bk/index/i0"))
+					Ω(mountindex1).Should(Equal("/bk/index/i1"))
 				})
 			})
 		})
@@ -132,6 +149,8 @@ var _ = Describe("Bookie", func() {
 					Ω(mountledger).Should(Equal("/bk/ledgers"))
 					mountjournal := sts.Spec.Template.Spec.Containers[0].VolumeMounts[1].MountPath
 					Ω(mountjournal).Should(Equal("/bk/journal"))
+					indexjournal := sts.Spec.Template.Spec.Containers[0].VolumeMounts[2].MountPath
+					Ω(indexjournal).Should(Equal("/bk/index"))
 				})
 			})
 		})