This commit incorporates the review comments

Signed-off-by: mittachaitu <[email protected]>

go.mod

@@ -5,7 +5,7 @@ go 1.13
 require (
 	github.com/evanphx/json-patch v4.5.0+incompatible // indirect
 	github.com/imdario/mergo v0.3.8 // indirect
-	github.com/openebs/api v0.0.0-20200316115502-9a4c0b1367c3
+	github.com/openebs/api v0.0.0-20200319173602-da787ed9fcbf
 	github.com/openebs/maya v0.0.0-20200226142318-6daf5f0486e8
 	github.com/pkg/errors v0.9.1
 	github.com/spf13/cobra v0.0.5

go.sum

@@ -403,6 +403,10 @@ github.com/openebs/api v0.0.0-20200311104100-f6a5bd72fae3 h1:30wuJe3AEcwYsq3w6t0
 github.com/openebs/api v0.0.0-20200311104100-f6a5bd72fae3/go.mod h1:Mj6izkLZT0jNKLs9u1CnbgNoP9BEgY/mgVI7Mc3/Hwc=
 github.com/openebs/api v0.0.0-20200316115502-9a4c0b1367c3 h1:/R2zYRJ0N9knT71TGXZHRJQIwj1RS/JWlQD8SB7vdnI=
 github.com/openebs/api v0.0.0-20200316115502-9a4c0b1367c3/go.mod h1:Mj6izkLZT0jNKLs9u1CnbgNoP9BEgY/mgVI7Mc3/Hwc=
+github.com/openebs/api v0.0.0-20200319152258-4190ca81ae69 h1:KRLrzRLf7oBnVSMDRCaSt0i+XwW7ucQ0sziey1CZAXw=
+github.com/openebs/api v0.0.0-20200319152258-4190ca81ae69/go.mod h1:Mj6izkLZT0jNKLs9u1CnbgNoP9BEgY/mgVI7Mc3/Hwc=
+github.com/openebs/api v0.0.0-20200319173602-da787ed9fcbf h1:QGAdLxow+WVfxTOxeli3RdzWp9SPRJl1GzNS/M8mOow=
+github.com/openebs/api v0.0.0-20200319173602-da787ed9fcbf/go.mod h1:Mj6izkLZT0jNKLs9u1CnbgNoP9BEgY/mgVI7Mc3/Hwc=
 github.com/openebs/maya v0.0.0-20200226142318-6daf5f0486e8 h1:ud86nr1TRU9fB3IeBzymBqYjaha/GjgjHCTeFTKeT8A=
 github.com/openebs/maya v0.0.0-20200226142318-6daf5f0486e8/go.mod h1:QQY9cOHKQwZ73qbv6O//UYUBLNV2S0MRDIfG7t5KOCk=
 github.com/openebs/maya v0.0.0-20200305100821-7118ad163bb4 h1:beXxZA3gQlP+r1mtNKfmdVjhs6NA5O3IXpNMoMoQXSY=

pkg/controllers/cspi-controller/handler.go

@@ -197,10 +197,14 @@ func (c *CStorPoolInstanceController) updateStatus(cspi *cstor.CStorPoolInstance
 	pool := zpool.PoolName()
 	propertyList := []string{"health", "io.openebs:readonly"}
 
+	// Since we quarried in following order health and io.openebs:readonly output also
+	// will be in same order
 	valueList, er := zpool.GetListOfPropertyValues(pool, propertyList)
 	if er != nil || len(valueList) != len(propertyList) {
 		return errors.Wrapf(err, "Failed to fetch %v output: %v", propertyList, valueList)
 	} else {
+		// valueList[0] will hold the value of health of cStor pool
+		// valueList[1] will hold the value of io.openebs:readonly of cStor pool
 		status.Phase = cstor.CStorPoolInstancePhase(valueList[0])
 		if valueList[1] == "on" {
 			status.ReadOnly = true
@@ -231,17 +235,23 @@ func (c *CStorPoolInstanceController) updateStatus(cspi *cstor.CStorPoolInstance
 // 1. If pool used space reached to roThresholdLimit then pool will be set to readonly mode
 // 2. If pool was in readonly mode if roThresholdLimit/pool expansion was happened then it
 //    unsets the ReadOnly Mode.
-// NOTE: This function must be invoked after having the updated cspiStatus information
+// NOTE: This function must be invoked after having the updated
+//       cspiStatus information from zfs/zpool
 func (c *CStorPoolInstanceController) updateROMode(
 	cspiStatus *cstor.CStorPoolInstanceStatus, cspi cstor.CStorPoolInstance) {
-	roThresholdLimit := cspi.Spec.PoolConfig.ROThresholdLimit
+	if cspi.Spec.PoolConfig.ROThresholdLimit == nil {
+		return
+	}
+	roThresholdLimit := *cspi.Spec.PoolConfig.ROThresholdLimit
 	totalInBytes := cspiStatus.Capacity.Total.Value()
 	usedInBytes := cspiStatus.Capacity.Used.Value()
 	pool := zpool.PoolName()
 
 	usedPercentage := (usedInBytes * 100) / totalInBytes
-	if (int(usedPercentage) >= roThresholdLimit) &&
-		(roThresholdLimit != 0 && roThresholdLimit != 100) {
+	// If roThresholdLimit sets 100% and pool used storage reached to 100%
+	// then there might be chances that operations will hung so it is not
+	// recommended to perform operations
+	if (int(usedPercentage) >= roThresholdLimit) && roThresholdLimit != 100 {
 		if !cspiStatus.ReadOnly {
 			if err := zpool.SetPoolRDMode(pool, true); err != nil {
 				// Here, we are just logging in next reconciliation it will be retried
@@ -251,7 +261,8 @@ func (c *CStorPoolInstanceController) updateROMode(
 				c.recorder.Event(&cspi,
 					corev1.EventTypeWarning,
 					"PoolReadOnlyThreshold",
-					"Pool storage limit reached to thresholdLimit. Pool expansion is required to make it's volume replicas RW",
+					"Pool storage limit reached to read only threshold limit. "+
+						"Pool expansion is required to make its volume replicas RW",
 				)
 			}
 		}

pkg/cspc/algorithm/build_csp.go

@@ -34,6 +34,7 @@ const (
 // GetCSPSpec returns a CSPI spec that should be created and claims all the
 // block device present in the CSPI spec
 func (ac *Config) GetCSPISpec() (*cstor.CStorPoolInstance, error) {
+	defaultROThresholdLimit := 85
 	poolSpec, nodeName, err := ac.SelectNode()
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to select a node")
@@ -58,8 +59,8 @@ func (ac *Config) GetCSPISpec() (*cstor.CStorPoolInstance, error) {
 		poolSpec.PoolConfig.PriorityClassName = ac.CSPC.Spec.DefaultPriorityClassName
 	}
 
-	if poolSpec.PoolConfig.ROThresholdLimit == 0 {
-		poolSpec.PoolConfig.ROThresholdLimit = 85
+	if poolSpec.PoolConfig.ROThresholdLimit == nil {
+		poolSpec.PoolConfig.ROThresholdLimit = &defaultROThresholdLimit
 	}
 
 	cspiLabels := ac.buildLabelsForCSPI(nodeName)

pkg/pool/operations/status.go

@@ -41,6 +41,7 @@ func GetPropertyValue(poolName, property string) (string, error) {
 }
 
 // GetListOfPropertyValues will return value list for given property list
+// NOTE: It will return the property values in the same order as property list
 func GetListOfPropertyValues(poolName string, propertyList []string) ([]string, error) {
 	ret, err := zfs.NewPoolGetProperty().
 		WithScriptedMode(true).
@@ -71,6 +72,11 @@ func GetCSPICapacity(poolName string) (cstor.CStorPoolInstanceCapacity, error) {
 			err,
 		)
 	}
+	// Since it was quarried in free, allocated and size output also
+	// will be in same order.
+	// valueList[0] contains value of free capacity in cStor pool
+	// valueList[1] contains value of allocated capacity in cStor pool
+	// valueList[2] contains total capacity of cStor pool
 	freeSizeInBinarySI := GetCapacityInBinarySi(valueList[0])
 	allocatedSizeInBinarySI := GetCapacityInBinarySi(valueList[1])
 	totalSizeInBinarySI := GetCapacityInBinarySi(valueList[2])