address review comments from kjacque and knard-intel and other feedba…

…ck from sherintg

Features: control
Allow-unstable-test: true
Required-githooks: true

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>

src/common/dav_v2/dav_iface.c

@@ -277,8 +277,7 @@ dav_obj_create_v2(const char *path, int flags, size_t sz, mode_t mode, struct um
 		/* Open the file and obtain the size */
 		fd = open(path, O_RDWR|O_CLOEXEC);
 		if (fd == -1) {
-			D_ERROR("obj_create_v2 open %s to fetch size: %s (%d)\n", path,
-				strerror(errno), errno);
+			DS_ERROR(errno, "obj_create_v2 open %s to fetch size", path);
 			return NULL;
 		}
 
@@ -288,8 +287,7 @@ dav_obj_create_v2(const char *path, int flags, size_t sz, mode_t mode, struct um
 	} else {
 		fd = open(path, O_CREAT|O_EXCL|O_RDWR|O_CLOEXEC, mode);
 		if (fd == -1) {
-			D_ERROR("obj_create_v2 open %s to alloc: %s (%d)\n", path, strerror(errno),
-				errno);
+			DS_ERROR(errno, "obj_create_v2 open %s to alloc", path);
 			return NULL;
 		}
 
@@ -326,7 +324,7 @@ dav_obj_open_v2(const char *path, int flags, struct umem_store *store)
 
 	fd = open(path, O_RDWR|O_CLOEXEC);
 	if (fd == -1) {
-		D_ERROR("obj_create_v2 open %s: %s (%d)\n", path, strerror(errno), errno);
+		DS_ERROR(errno, "obj_create_v2 open %s", path);
 		return NULL;
 	}
 

src/control/cmd/daos/pretty/pool_test.go

@@ -292,8 +292,8 @@ Pool space info:
 						MediaType: daos.StorageMediaTypeScm,
 					},
 					{
-						Total:     2,
-						Free:      1,
+						Total:     4,
+						Free:      2,
 						MediaType: daos.StorageMediaTypeNvme,
 					},
 				},
@@ -311,8 +311,8 @@ Pool space info:
   Total size: 2 B
   Free: 1 B, min:0 B, max:0 B, mean:0 B
 - Data storage:
-  Total size: 2 B
-  Free: 1 B, min:0 B, max:0 B, mean:0 B
+  Total size: 4 B
+  Free: 2 B, min:0 B, max:0 B, mean:0 B
 `, poolUUID.String()),
 		},
 	} {

src/control/cmd/dmg/pool.go

@@ -51,7 +51,9 @@ type PoolCmd struct {
 
 var (
 	// Default to 6% SCM:94% NVMe
-	defaultTierRatios = []float64{0.06, 0.94}
+	defaultTierRatios         = []float64{0.06, 0.94}
+	errPoolCreateIncompatOpts = errors.New("unsupported option combination, use (--scm-size and " +
+		"--nvme-size) or (--scm-size and --nvme-size) or (--size)")
 )
 
 type tierRatioFlag struct {
@@ -203,7 +205,7 @@ type PoolCreateCmd struct {
 	NVMeSize   sizeFlag            `short:"n" long:"nvme-size" description:"Per-engine NVMe allocation for DAOS pool (manual)"`
 	MetaSize   sizeFlag            `long:"meta-size" description:"Per-engine Metadata-on-SSD allocation for DAOS pool (manual). Only valid in MD-on-SSD mode"`
 	DataSize   sizeFlag            `long:"data-size" description:"Per-engine Data-on-SSD allocation for DAOS pool (manual). Only valid in MD-on-SSD mode"`
-	MemRatio   tierRatioFlag       `long:"mem-ratio" description:"Percentage of the pool metadata storage size that should be used as the memory file (cache) size. An example value of 25.5 would result in a memory file size of 0.255 times the value set in --meta-size. Only valid in MD-on-SSD mode"`
+	MemRatio   tierRatioFlag       `long:"mem-ratio" description:"Percentage of the pool metadata storage size (on SSD) that should be used as the memory file size (on ram-disk). Default value is 100% and only valid in MD-on-SSD mode"`
 	RankList   ui.RankSetFlag      `short:"r" long:"ranks" description:"Storage engine unique identifiers (ranks) for DAOS pool"`
 
 	Args struct {
@@ -247,14 +249,10 @@ func (cmd *PoolCreateCmd) setMemRatio(req *control.PoolCreateReq, defVal float32
 }
 
 func (cmd *PoolCreateCmd) storageAutoPercentage(ctx context.Context, req *control.PoolCreateReq) error {
-	switch {
-	case cmd.ScmSize.IsSet() || cmd.NVMeSize.IsSet():
-		return errIncompatFlags("size", "scm-size", "nvme-size")
-	case cmd.MetaSize.IsSet() || cmd.DataSize.IsSet():
-		return errIncompatFlags("size", "meta-size", "data-size")
-	case cmd.NumRanks > 0:
+	if cmd.NumRanks > 0 {
 		return errIncompatFlags("size", "nranks")
-	case cmd.TierRatio.IsSet():
+	}
+	if cmd.TierRatio.IsSet() {
 		return errIncompatFlags("size=%", "tier-ratio")
 	}
 	cmd.Infof("Creating DAOS pool with %s of all storage", cmd.Size)
@@ -271,12 +269,7 @@ func (cmd *PoolCreateCmd) storageAutoPercentage(ctx context.Context, req *contro
 }
 
 func (cmd *PoolCreateCmd) storageAutoTotal(req *control.PoolCreateReq) error {
-	switch {
-	case cmd.ScmSize.IsSet() || cmd.NVMeSize.IsSet():
-		return errIncompatFlags("size", "scm-size", "nvme-size")
-	case cmd.MetaSize.IsSet() || cmd.DataSize.IsSet():
-		return errIncompatFlags("size", "meta-size", "data-size")
-	case cmd.NumRanks > 0 && !cmd.RankList.Empty():
+	if cmd.NumRanks > 0 && !cmd.RankList.Empty() {
 		return errIncompatFlags("nranks", "ranks")
 	}
 
@@ -301,13 +294,6 @@ func (cmd *PoolCreateCmd) storageAutoTotal(req *control.PoolCreateReq) error {
 }
 
 func (cmd *PoolCreateCmd) storageManualMdOnSsd(req *control.PoolCreateReq) error {
-	if cmd.ScmSize.IsSet() {
-		return errIncompatFlags("mem-size", "scm-size")
-	}
-	if cmd.NVMeSize.IsSet() {
-		return errIncompatFlags("data-size", "nvme-size")
-	}
-
 	metaBytes := cmd.MetaSize.bytes
 	dataBytes := cmd.DataSize.bytes
 	req.TierBytes = []uint64{metaBytes, dataBytes}
@@ -341,8 +327,6 @@ func (cmd *PoolCreateCmd) storageManual(req *control.PoolCreateReq) error {
 		return errIncompatFlags("mem-ratio", "scm-size", "nvme-size")
 	case cmd.NVMeSize.IsSet() && !cmd.ScmSize.IsSet():
 		return errors.New("--nvme-size cannot be set without --scm-size")
-	case !cmd.ScmSize.IsSet():
-		return errors.New("at least one size parameter must be set")
 	}
 
 	scmBytes := cmd.ScmSize.bytes
@@ -388,6 +372,20 @@ func (cmd *PoolCreateCmd) Execute(args []string) error {
 		}
 	}
 
+	// Refuse unsupported input value combinations.
+
+	pmemParams := cmd.ScmSize.IsSet() || cmd.NVMeSize.IsSet()
+	mdParams := cmd.MetaSize.IsSet() || cmd.DataSize.IsSet()
+
+	switch {
+	case (pmemParams || mdParams) && cmd.Size.IsSet():
+		return errPoolCreateIncompatOpts
+	case pmemParams && mdParams:
+		return errPoolCreateIncompatOpts
+	case !pmemParams && !mdParams && !cmd.Size.IsSet():
+		return errPoolCreateIncompatOpts
+	}
+
 	// Validate supported input values and set request fields.
 
 	switch {

src/control/cmd/dmg/pool_test.go

@@ -233,7 +233,7 @@ func TestPoolCommands(t *testing.T) {
 			"Create pool with missing size",
 			"pool create label",
 			"",
-			errors.New("at least one size parameter must be set"),
+			errPoolCreateIncompatOpts,
 		},
 		{
 			"Create pool with missing label",
@@ -245,13 +245,13 @@ func TestPoolCommands(t *testing.T) {
 			"Create pool with incompatible arguments (auto nvme-size)",
 			fmt.Sprintf("pool create label --size %s --nvme-size %s", testSizeStr, testSizeStr),
 			"",
-			errors.New("may not be mixed"),
+			errPoolCreateIncompatOpts,
 		},
 		{
 			"Create pool with incompatible arguments (auto scm-size)",
 			fmt.Sprintf("pool create label --size %s --scm-size %s", testSizeStr, testSizeStr),
 			"",
-			errors.New("may not be mixed"),
+			errPoolCreateIncompatOpts,
 		},
 		{
 			"Create pool with incompatible arguments (% size nranks)",
@@ -287,19 +287,19 @@ func TestPoolCommands(t *testing.T) {
 			"Create pool with incompatible arguments (auto with meta-size)",
 			fmt.Sprintf("pool create label --size %s --meta-size 32G", testSizeStr),
 			"",
-			errors.New("may not be mixed"),
+			errPoolCreateIncompatOpts,
 		},
 		{
 			"Create pool with incompatible arguments (scm-size with meta-size)",
 			fmt.Sprintf("pool create label --scm-size %s --meta-size 32G", testSizeStr),
 			"",
-			errors.New("may not be mixed"),
+			errPoolCreateIncompatOpts,
 		},
 		{
 			"Create pool with incompatible arguments (scm-size with data-size)",
 			fmt.Sprintf("pool create label --scm-size %s --data-size 32G", testSizeStr),
 			"",
-			errors.New("may not be mixed"),
+			errPoolCreateIncompatOpts,
 		},
 		{
 			"Create pool with too-large tier-ratio (auto)",

src/control/cmd/dmg/pretty/pool_test.go

@@ -582,6 +582,33 @@ one  6.0 TB Ready 83%%  16%%       0/16
 			verbose:     true,
 			expPrintStr: msgNoPools + "\n",
 		},
+		"verbose, two pools": {
+			resp: &control.ListPoolsResp{
+				Pools: []*daos.PoolInfo{
+					{
+						UUID:             test.MockPoolUUID(1),
+						TierStats:        exampleTierStats,
+						TotalTargets:     16,
+						ActiveTargets:    16,
+						DisabledTargets:  0,
+						State:            daos.PoolServiceStateReady,
+						PoolLayoutVer:    1,
+						UpgradeLayoutVer: 2,
+						Rebuild: &daos.PoolRebuildStatus{
+							State: daos.PoolRebuildStateIdle,
+						},
+						QueryMask: daos.DefaultPoolQueryMask,
+					},
+				},
+			},
+			verbose: true,
+			expPrintStr: `
+Label UUID                                 State SvcReps SCM Size SCM Used SCM Imbalance NVME Size NVME Used NVME Imbalance Disabled UpgradeNeeded? Rebuild State 
+----- ----                                 ----- ------- -------- -------- ------------- --------- --------- -------------- -------- -------------- ------------- 
+-     00000001-0001-0001-0001-000000000001 Ready N/A     100 GB   80 GB    16%           6.0 TB    5.0 TB    8%             0/16     1->2           idle          
+
+`,
+		},
 	} {
 		t.Run(name, func(t *testing.T) {
 			var bld strings.Builder

src/control/server/mgmt_pool.go

@@ -182,6 +182,8 @@ func (svc *mgmtSvc) calculateCreateStorage(req *mgmtpb.PoolCreateReq) error {
 	case mdOnSSD && req.MemRatio == 0:
 		// Set reasonable default if not set in MD-on-SSD mode.
 		req.MemRatio = storage.DefaultMemoryFileRatio
+		svc.log.Infof("Default memory-file:md-on-ssd ratio of %d%% applied",
+			int(storage.DefaultMemoryFileRatio)*100)
 	}
 
 	// NB: The following logic is based on the assumption that a request will always include SCM