Configure OSDs with bluestore #88

travisn · 2016-10-18T04:36:40Z

Castled can now be configured with devices to be provisioned for OSDs with bluestore, or OSDs with filestore in a local directory.

When configuring devices for bluestore, castled must be running privileged.
When utilizing a directory for filestore, no elevation is necessary, though if you want to use the default config directory (/var/lib/castle), you will need to create and chown before invoking castled.
If neither devices (storage-devices) nor directories (--storage-dirs) are specified, the current directory is assumed to be a filestore directory.

BlueStore example:
Three devices are formatted for bluestore configuration of the OSDs. The config files are generated in /var/lib/castle by default.
sudo ./castled --discovery-url=$discovery_token --private-ipv4=${COREOS_PRIVATE_IPV4} --storage-devices=sdb,sdc,sdd --force-format=true

FileStore example:
The current directory is used for all filestore journal and data. The --config-dir option is specified to use a different directory for config files.
castled --discovery-url=$discovery_token --private-ipv4=${COREOS_PRIVATE_IPV4} --config-dir=/home/core

bassam · 2016-10-18T04:38:34Z

ceph

@@ -1 +1 @@
-Subproject commit dc578817ca6d169e2225f1ec9428942668f46a8c
+Subproject commit 763750b0f06f6e27b976853bfe314aa555757e61


why was this updated?

i was just looking at this, it wasn't intended...

bassam · 2016-10-18T04:38:53Z

.gitignore

+
+mon0
+mon1
+mon2


why are these needed?

this was my last cleanup item. Some mon config is written in the current dir instead of the desired dir.

bassam · 2016-10-18T04:39:13Z

cmd/castlectl/castlectl.go

@@ -16,11 +16,11 @@ var (
 )


you need to rebase this to what is in master. castlectl is no longer used.

bassam · 2016-10-18T04:41:48Z

cmd/castled/castled.go

-	rootCmd.Flags().StringVar(&cfg.devices, "devices", "", "comma separated list of devices to use")
+	rootCmd.Flags().StringVar(&cfg.devices, "storage-devices", "", "comma separated list of devices to use for storage")
+	rootCmd.Flags().StringVar(&cfg.directories, "storage-dirs", "", "comma separated list of directories to use for storage")
+	rootCmd.Flags().StringVar(&cfg.configDir, "config-dir", "/var/lib/castle", "directory for storing configuration")


I think we need a different name for this. If it was truly a config dir then it would have gone in /etc. I think its closer to a data-dir but we would need to explain the overlap/confusion with storage-*

I think data-dir is the right flag. Our data-dir is made up of a number of mapped volumes (through storage-devices, and storage-dirs) but as a whole its still our data-dir.

my first inclination is to always use /var/lib/castle and not expose it. It's really just a small directory where we generate configuration for running ceph.

bassam · 2016-10-18T04:43:17Z

pkg/cephmgr/cephleader.go

@@ -89,7 +89,7 @@ func (c *cephLeader) handleOrchestratorEvents() {
 			// Perform a full refresh of the cluster to ensure the monitors are running with quorum
 			err := c.configureCephMons(e.Context())
 			if err != nil {
-				log.Printf("failed to configure ceph mons. %v", err)
+				log.Printf("FAILED TO CONFIGURE CEPH MONS. %v", err)


why the all caps?

just to make the error more obvious among the verbose logs. Log levels would really help.

bassam · 2016-10-18T04:45:30Z

pkg/cephmgr/osd.go

-		if err != nil {
-			return fmt.Errorf("command %s failed: %+v", cmd, err)
-		}
+func partitionBluestoreDevice(device, serial string, executor exec.Executor) error {


does this happen unconditionally?

no, only if they pass --force-format=true or if we don't find a file system. The method previous to this implements those checks

bassam · 2016-10-18T04:48:32Z

pkg/util/proc/procmanager.go

+func (p *ProcManager) Run(sudo bool, daemon string, args ...string) error {
+	sudoMsg := ""
+	if sudo {
+		sudoMsg = "sudo "


why is sudo needed?

i removed all usages of it, but missed this one. I'll remove it too

bassam · 2016-10-18T04:49:14Z

This is a big PR, would have been good to break it down into a few commits that build on each other.

bassam · 2016-10-18T04:51:41Z

which external tools do we still depend on in castled?

bassam · 2016-10-18T04:53:23Z

cmd/castled/castled.go

-	rootCmd.Flags().StringVar(&cfg.devices, "devices", "", "comma separated list of devices to use")
+	rootCmd.Flags().StringVar(&cfg.devices, "storage-devices", "", "comma separated list of devices to use for storage")
+	rootCmd.Flags().StringVar(&cfg.directories, "storage-dirs", "", "comma separated list of directories to use for storage")
+	rootCmd.Flags().StringVar(&cfg.configDir, "config-dir", "/var/lib/castle", "directory for storing configuration")


I think data-dir is the right flag. Our data-dir is made up of a number of mapped volumes (through storage-devices, and storage-dirs) but as a whole its still our data-dir.

bassam · 2016-10-18T04:57:06Z

pkg/cephmgr/osd.go

@@ -124,16 +189,17 @@ func createOSDBootstrapKeyring(conn client.Connection, clusterName string) error
 	// get-or-create-key for client.bootstrap-osd
 	bootstrapOSDKey, err := client.AuthGetOrCreateKey(conn, "client.bootstrap-osd", []string{"mon", "allow profile bootstrap-osd"})
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to get or create osd auth key %s. %+v", bootstrapOSDKeyringPath, err)
 	}

 	log.Printf("succeeded bootstrap OSD get/create key, bootstrapOSDKey: %s", bootstrapOSDKey)

 	// write the bootstrap-osd keyring to disk
 	bootstrapOSDKeyringDir := filepath.Dir(bootstrapOSDKeyringPath)


do we need a bootstrap key per OSD or can we use a bootstrap key per node, or not at all?

i know we talked about having a bootstrap key per node. I'll open a separate ticket for this

bassam · 2016-10-18T05:03:15Z

pkg/cephmgr/osd.go

@@ -144,7 +210,7 @@ func createOSDBootstrapKeyring(conn client.Connection, clusterName string) error
 }

 // format the given device for usage by an OSD
-func formatOSD(device string, forceFormat bool, executor exec.Executor) error {
+func formatDevice(device, serial string, forceFormat bool, executor exec.Executor) error {
 	// format the current volume
 	devFS, err := sys.GetDeviceFilesystem(device, executor)


what does this return when a disk has been partitioned for bluestore? I think need to figure out if this is a device that we ourselves have partitioned in the past too, right?

Agreed, checking for our partitioning needs to be tightened up. I'll look at that

bassam · 2016-10-18T05:04:41Z

pkg/cephmgr/osd.go

-			}
+	args := []string{}
+	gb := 1024
+	args = append(args, getSGDiskArgs(walPartition, 1, gb-1, fmt.Sprintf("osd-%s-wal", serial))...)


Can you please add a good long comment about the partition scheme? its hard to parse it out of the calls to SGdisk

definitely will add a comment

bassam · 2016-10-18T05:05:57Z

pkg/cephmgr/osd.go

+	args := []string{}
+	gb := 1024
+	args = append(args, getSGDiskArgs(walPartition, 1, gb-1, fmt.Sprintf("osd-%s-wal", serial))...)
+	args = append(args, getSGDiskArgs(databasePartition, gb, gb, fmt.Sprintf("osd-%s-db", serial))...)


Was ignition's sgdisk package not useful for our use cases?

There were a few reasons I didn't pull it in:

it would have required pulling in their internal logging package,

it has transactional semantics that aren't really transactional

it didn't expose the ability to have the last partition take all the remaining space

it was very simple code

bassam · 2016-10-18T05:08:27Z

Would be great if you can move all calls to exec to a system package so that platform dependencies are centralized and can be tightly controlled.

travisn

Still tracking down the mon folder issue...

travisn · 2016-10-18T04:59:44Z

cmd/castlectl/castlectl.go

@@ -16,11 +16,11 @@ var (
 )


travisn · 2016-10-18T05:01:03Z

cmd/castled/castled.go

-	rootCmd.Flags().StringVar(&cfg.devices, "devices", "", "comma separated list of devices to use")
+	rootCmd.Flags().StringVar(&cfg.devices, "storage-devices", "", "comma separated list of devices to use for storage")
+	rootCmd.Flags().StringVar(&cfg.directories, "storage-dirs", "", "comma separated list of directories to use for storage")
+	rootCmd.Flags().StringVar(&cfg.configDir, "config-dir", "/var/lib/castle", "directory for storing configuration")


my first inclination is to always use /var/lib/castle and not expose it. It's really just a small directory where we generate configuration for running ceph.

travisn · 2016-10-18T05:03:27Z

pkg/cephmgr/osd.go

-		if err != nil {
-			return fmt.Errorf("command %s failed: %+v", cmd, err)
-		}
+func partitionBluestoreDevice(device, serial string, executor exec.Executor) error {


no, only if they pass --force-format=true or if we don't find a file system. The method previous to this implements those checks

travisn · 2016-10-18T05:04:41Z

pkg/util/proc/procmanager.go

+func (p *ProcManager) Run(sudo bool, daemon string, args ...string) error {
+	sudoMsg := ""
+	if sudo {
+		sudoMsg = "sudo "


i removed all usages of it, but missed this one. I'll remove it too

travisn · 2016-10-18T05:12:27Z

pkg/cephmgr/cephleader.go

@@ -89,7 +89,7 @@ func (c *cephLeader) handleOrchestratorEvents() {
 			// Perform a full refresh of the cluster to ensure the monitors are running with quorum
 			err := c.configureCephMons(e.Context())
 			if err != nil {
-				log.Printf("failed to configure ceph mons. %v", err)
+				log.Printf("FAILED TO CONFIGURE CEPH MONS. %v", err)


just to make the error more obvious among the verbose logs. Log levels would really help.

travisn · 2016-10-18T05:29:36Z

These are the only external commands I see server-side that we call out to:

sgdisk
lsblk
bash
df

bash and df are only used in one place by the osd to see if a device has a filesystem. here is the command: df --output=source,fstype | grep '^/dev/%s ' | awk '{print $2}'. I didn't look for another way to do this yet.

The client tool also calls modprobe, mount, etc, but they should not be triggered server-side.

bassam · 2016-10-18T05:32:29Z

regarding bash and df, we should be able to use syscall.Statfs

still reviewing

bassam · 2016-10-18T16:49:38Z

pkg/cephmgr/osd.go

+// Partitions a device for use by a bluestore osd.
+// If there are any partitions or formatting already on the device, it will be wiped.
+// The result will be three partitions:
+// 1. WAL: Write ahead log


what are the sizes of the partitions. Also what about the case when we want to colocate the WAL/meta of multiple OSDs, some of these partitions will be empty/unused right?

does it make sense to reserve more space on each disk for future work? You don't want to be messing with partitions frequently. For a comparison, take a look at https://github.com/coreos/scripts/blob/master/build_library/disk_layout.json. This is how CoreOS lays out partitions.

We also need to store some information on this disk that tells us its one of ours, and possibly a version number of the partitioning scheme used. we might need to migrate it.

bassam · 2016-10-18T16:53:10Z

pkg/cephmgr/osd.go

-	args := []string{}
+	// This is a simple scheme to create the wal and db each of size 1GB,
+	// with the remainder of the disk being allocated for the raw data.
+	// TODO: Look at the disk size to determine smarter sizes for the wal and db.


yes that's two small. we need to use more informed sizes.

bassam · 2016-10-18T16:54:44Z

pkg/cephmgr/osd.go

+// The id is the partition number.
+// The offset and length are in MB. Under the covers this is translated to sectors.
+// If the length is -1, all remaining space will be assigned to the partition.
+func getPartitionArgs(id, mbOffset, mbLength int, label string) []string {
 	const sectorsPerMB = 2048


is this fixed?

continuing review

jbw976 · 2016-10-19T18:40:22Z

demo/vagrant/Vagrantfile

@@ -1,8 +1,10 @@
 VAGRANTFILE_API_VERSION = "2"

 $channel = "alpha"
-$nodes = 3
+$nodes = 1


why is the default "demo" scenario a single node now? is that important?

i'll change it back to 3

the idea of this demo was to start one machine, and then show growth by starting the other machines. this is also why autostart is false for node 2 and 3

bassam · 2016-10-19T18:45:43Z

demo/vagrant/Vagrantfile

@@ -11,10 +13,12 @@ Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
  set_coreos_box(config, $channel)

  (0..($nodes - 1)).each do |i|
-    config.vm.define name="castle%02d" % i, primary: (i == 0), autostart: (i == 0) do |node|
+    config.vm.define name="castle%02d" % i, primary: false, autostart: true do |node|


why change this? I like vagrant up bringing up the first node. and vagrant ssh going to the first node.

i'll change it back to primary: (i == 0), but leave autostart: true. The 2nd and 3rd machines weren't starting before

bassam · 2016-10-19T18:46:13Z

demo/vagrant/Vagrantfile

 $new_discovery_url="https://discovery.etcd.io/new?size=1"
+$node_disks = ENV['CASTLE_NODE_DISKS'] != nil ? ENV['CASTLE_NODE_DISKS'].to_i : 3


I dont think you need the env variables. just set it to a constant.

bassam · 2016-10-19T18:47:18Z

demo/vagrant/readme.md

+4. Start the Ubuntu VM
+5. Install VirtualBox 5.0
+6. sudo apt-get install nfs-common nfs-kernel-server
+7. vagrant up


these instructions are for internal audience. I think they need to be for a public audience.

ok, i'll remove it for now until it stabilizes with bluestore

bassam · 2016-10-19T18:47:41Z

demo/vagrant/util.rb

+# Add persistent storage volumes
+def attach_volumes(node, num_volumes, volume_size)
+
+  #if $provider == :virtualbox


please remove this line.

* change ob ClaimRef from UID to ObjectReference Signed-off-by: jeffvance <jeff.h.vance@gmail.com> * added UID to ob.ClaimRef Signed-off-by: jeffvance <jeff.h.vance@gmail.com> * allow obc to have no bkt name fields for brownfield Signed-off-by: jeffvance <jeff.h.vance@gmail.com> * shouldProvision Signed-off-by: jeffvance <jeff.h.vance@gmail.com>

bassam requested changes Oct 18, 2016

View reviewed changes

travisn commented Oct 18, 2016

View reviewed changes

bassam previously approved these changes Oct 18, 2016

View reviewed changes

bassam previously requested changes Oct 18, 2016

View reviewed changes

travisn added 2 commits October 19, 2016 11:35

configuration of osds with bluestore

7f9cf16

simple partition scheme with uuids

634c6cc

jbw976 reviewed Oct 19, 2016

View reviewed changes

bassam reviewed Oct 19, 2016

View reviewed changes

working vagrant for castled with filestore osds

579f78b

bassam approved these changes Oct 19, 2016

View reviewed changes

travisn merged commit c68f860 into rook:master Oct 19, 2016

travisn deleted the bluestore branch October 19, 2016 21:08

travisn mentioned this pull request Oct 19, 2016

Enable bluestore option for osd configuration #53

Closed

bassam mentioned this pull request Oct 24, 2016

Trim the tool dependencies for castled #73

Closed

cr1cr1 mentioned this pull request Mar 9, 2020

Metadata pool is used for data instead of the Erasure Coded one #4988

Closed

		@@ -1 +1 @@
		Subproject commit dc578817ca6d169e2225f1ec9428942668f46a8c
		Subproject commit 763750b0f06f6e27b976853bfe314aa555757e61

		$new_discovery_url="https://discovery.etcd.io/new?size=1"
		$node_disks = ENV['CASTLE_NODE_DISKS'] != nil ? ENV['CASTLE_NODE_DISKS'].to_i : 3

Configure OSDs with bluestore #88

Configure OSDs with bluestore #88

Conversation

travisn commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bassam commented Oct 18, 2016

bassam commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bassam commented Oct 18, 2016

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisn commented Oct 18, 2016

bassam commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment