add cspc-operator #1

sonasingh46 · 2020-02-19T11:44:42Z

This PR adds cspc-operator.
Following are pending items :

Add cspi operator

NOTES: The Second commit contains the vendor code.
Signed-off-by: Ashutosh Kumar [email protected]

mittachaitu

I will continue reviewing tomorrow...

cspc-operator/app/start.go

cspc-operator/examples/crds/cspccrd.yaml

cspc-operator/examples/crds/cspicrd.yaml

pkg/controllers/cspc-controller/controller.go

pkg/cspc/algorithm/select_node.go

pkg/cspc/algorithm/build_csp.go

mittachaitu · 2020-02-21T19:41:34Z

pkg/cspc/algorithm/build_csp.go

+		WithName(ac.CSPC.Name + "-" + rand.String(4)).
+		WithNamespace(ac.Namespace).
+		WithNodeSelectorByReference(poolSpec.NodeSelector).
+		WithNodeName(nodeName).


It is saying WithNodeName but it is really setting HostName.

https://github.com/openebs/api/blob/b896c9073fd4c29042a2da9148512b260a0eace2/pkg/apis/cstor/v1/cstorpoolinstancebuilder.go#L97

mittachaitu · 2020-02-21T19:49:20Z

pkg/cspc/algorithm/build_csp.go

+		WithNodeSelectorByReference(poolSpec.NodeSelector).
+		WithNodeName(nodeName).
+		WithPoolConfig(poolSpec.PoolConfig).
+		WithRaidGroups(poolSpec.DataRaidGroups).


We missed poolSpec.WriteCacheRaidGroups?

WriteCache things will be taken in a separate PR.

mittachaitu · 2020-02-21T19:51:17Z

pkg/cspc/algorithm/select_node.go

+		for _, bd := range group.BlockDevices {
+			BDList = append(BDList, bd.BlockDeviceName)
+		}
+	}


Here also we missed iterating over WriteCacheRaidGroups.

WriteCache things will be taken in a separate PR.

pkg/controllers/cspc-controller/scale.go

mittachaitu · 2020-02-22T09:32:56Z

pkg/cspc/algorithm/build_csp.go

+
+const (
+	// StoragePoolKindCSPC holds the value of CStorPoolCluster
+	StoragePoolKindCSPC = "CStorPoolCluster"


How about moving them to openebs/api/pkg/apis/types?

mittachaitu · 2020-02-22T09:37:31Z

pkg/cspc/algorithm/select_node.go

+}
+
+// GetBDListForNode returns a list of BD from the pool spec.
+func GetBDListForNode(pool cstor.PoolSpec) []string {


How about moving to API repo because it looks like tightly coupled with PoolSpec. IMO looks like a getter method from the spec.

pkg/cspc/algorithm/build_deploy.go

mittachaitu · 2020-02-22T17:58:58Z

pkg/cspc/algorithm/select_node.go

+			klog.Errorf("could not use node for selectors {%v}", pool.NodeSelector)
+			continue
+		}
+		if ac.VisitedNodes[nodeName] {


ac.VisitedNodes[nodeName] will be always false because we are initializing Config inside loop

for poolCount := 1; poolCount <= pendingPoolCount; poolCount++ { }

fixed this.

mittachaitu · 2020-02-23T10:14:38Z

pkg/controllers/cspc-controller/scale.go

+	return nil
+}
+
+// getOrphanedCStorPools returns a list of CSPI names that should be deleted.


getOrphanedCStorPools need to revisit when the migration of pools from one node to another node when the same data disks were attached to other nodes.

mittachaitu · 2020-02-23T10:17:30Z

pkg/controllers/cspc-controller/scale.go

+		c.recorder.Event(cspc, corev1.EventTypeNormal,
+			"DownScale", "De-provisioning pool "+cspiName)
+
+		// TODO : As part of deleting a CSP, do we need to delete associated BDCs ?


We can remove TODO because of cleanupCSPIResources is doing a cleanup work of CSPI.

pkg/cspc/algorithm/build_deploy.go

Signed-off-by: Ashutosh Kumar <[email protected]>

pkg/cspc/algorithm/build_csp.go

mittachaitu · 2020-02-26T14:16:23Z

pkg/cspc/algorithm/select_node_test.go

+	return newNodeList
+}
+
+func NewFixture() *fixture {


Can we change this to the package scope? IMO outside the package, no one should see the test code.

mittachaitu · 2020-02-27T07:50:10Z

pkg/cspc/algorithm/build_csp.go

+		return nil, errors.Errorf("failed to select a node as empty node name received from SelectNode")
+	}
+
+	// ToDo: Move following to mutating webhook.


Should we really need to move to mutate webhooks?
cc: @prateekpandey14

mittachaitu · 2020-02-27T08:24:30Z

cspc-operator/app/start.go

+	// If multiple controllers happen to call this AddToScheme same time,
+	// it causes panic with error saying concurrent map access.
+	// This lock is used to serialize the AddToScheme call of all controllers.
+	//controllerMtx.Lock()


Is this lock is really required?

mittachaitu · 2020-02-27T09:33:09Z

pkg/controllers/cspc-controller/controller.go

+	return cb.Controller, nil
+}
+
+// addSpc is the add event handler for cspc


addSpc --> addCSPC

pkg/controllers/cspc-controller/controller.go

mittachaitu

good to go for merging to unblock other PRs(Volume replated PRs). But I will be reviewing after merging also.

Signed-off-by: Ashutosh Kumar <[email protected]>

This commit removes some of the maya dependencies Signed-off-by: Ashutosh Kumar <[email protected]>

cspc-operator/main.go

mittachaitu · 2020-02-29T12:00:45Z

pkg/controllers/cspc-controller/sync.go

+// EstimateCSPCVersion returns the cspi version if any cspi is present for the cspc or
+// returns the maya version as the new cspi created will be of maya version
+func (c *Controller) EstimateCSPCVersion(cspc *cstor.CStorPoolCluster) (string, error) {
+	cspiList, err := c.clientset.CstorV1().


nit: we can use c.GetCSPIListForCSPC(cspcObj)

mittachaitu · 2020-02-29T12:17:12Z

pkg/cspc/algorithm/config.go

+func NewBuilder() *Builder {
+	return &Builder{
+		ConfigObj: &Config{
+			CSPC:         &cstor.CStorPoolCluster{},


Why do we need to allocated unnecessary memory(to be safe from crashes)? IMO caller should use WithCSPC func.

mittachaitu · 2020-02-29T12:19:30Z

pkg/controllers/cspc-controller/sync.go

+	pc := NewPoolConfig().WithAlgorithmConfig(ac).WithController(c)
+
+	// Create pools if required.
+	if len(cspiList.Items) < len(cspc.Spec.Pools) {


Here chances are there to crash because caller code didn't handle error case when cspiList API call failed.

mittachaitu · 2020-02-29T12:21:28Z

pkg/controllers/cspc-controller/handler.go

+
+	// Deep-copy otherwise we are mutating our cache.
+	cspcGot := cspc.DeepCopy()
+	cspiList, _ := c.GetCSPIListForCSPC(cspcGot)


If an error occurs we should log and return from here else if sync() is making use of cspiList chances will be there for process crash.

mittachaitu · 2020-02-29T12:31:01Z

pkg/cspc/algorithm/select_node.go

+// If the block device(s) is/are already calimed for the same CSPC -- it is left as it is and can be used for
+// pool provisioning.
+// If the block device(s) is/are unclaimed, then those are claimed.
+func (ac *Config) ClaimBDsForNode(BD []string) error {


Can we change BD ---> bdNameList?

mittachaitu · 2020-02-29T18:19:09Z

pkg/controllers/cspc-controller/operations.go

+	var err error
+	for _, cspcRaidGroup := range cspcPoolSpec.DataRaidGroups {
+		cspcRaidGroup := cspcRaidGroup
+		cspiRaidGroup := getReplacedCSPIRaidGroup(&cspcRaidGroup, cspi)


This piece of code needs to be updated properly may be in subsequent PR's for code readability purpose

raidGroupIndex, cspiRaidGroup := getReplacedCSPIRaidGroup(&cspcRaidGroup, cspi) if raidGroupIndex != -1 { replacedRaidGroup, err := pc.getReplaceRaidGroup(cspcRaidGroup, cspiRaidGroup) if err != nil { // RETUR error } cspi.Spec.RaidGroup[raidGroupIndex] = replacedRaidGroup }

example please....

mittachaitu · 2020-02-29T18:21:36Z

pkg/controllers/cspc-controller/operations.go

+}
+
+// replaceBlockDevice replaces block devices in cStor pools as specified in CSPC.
+func (pc *PoolConfig) replaceBlockDevice() error {


Events need to be added

pool-manager/app/start.go

mittachaitu · 2020-03-01T16:16:49Z

pkg/pool/pool.go

+// CheckForZreplInitial is blocking call for checking status of zrepl in cstor-pool container.
+func CheckForZreplInitial(ZreplRetryInterval time.Duration) {
+	for {
+		_, err := RunnerVar.RunCombinedOutput(zpool.PoolOperator, "status")


TODO: We can use a builder pattern to execute zpool & zfs commands.

mittachaitu · 2020-03-01T17:11:25Z

pkg/controllers/cspi-controller/handler.go

+		return err
+	}
+
+	if IsReconcileDisabled(cspi) {


Can we have cspi as a receiver type?

shubham14bajpai · 2020-03-02T06:15:20Z

cspc-operator/examples/cspc/cspi.yaml

+  name: cspi-stripe
+  namespace: openebs
+spec:
+  pools:


CStorPoolInstance spec does not have pools field and each CSPI will have only one pool spec. Please update the yaml with correct fields.

shubham14bajpai · 2020-03-02T06:15:48Z

cspc-operator/examples/cspc/cspi1.yaml

+  name: cspi-stripe-1
+  namespace: openebs
+spec:
+  pools:


Same comment as above.

mittachaitu

Changes are good didn't go through all the builders of ZFS & ZPOOL.....

mittachaitu · 2020-03-02T05:43:25Z

pkg/controllers/cspi-controller/handler.go

+}
+
+// addPoolProtectionFinalizer is to add PoolProtectionFinalizer finalizer of cstorpoolinstance resource.
+func (c *CStorPoolInstanceController) addPoolProtectionFinalizer(cspi *cstor.CStorPoolInstance) error {


Here we should return the updated object...

mittachaitu · 2020-03-02T05:49:10Z

pkg/pool/operations/import.go

+	"k8s.io/klog"
+)
+
+// Import will import pool for given CSP object.


given CSPI object.

mittachaitu · 2020-03-02T06:11:07Z

pkg/pool/operations/import.go

+		return false, err
+	}
+
+	klog.Infof("Importing pool %s %s", string(cspi.GetUID()), PoolName(cspi))


Can we change PoolName(cspi) ---> cspi.GetPoolName()?

mittachaitu · 2020-03-02T06:26:51Z

pkg/pool/operations/import.go

+		klog.Errorf("Failed to import pool : %s : %s", cmdOut, err.Error())
+		return false, err
+	}
+


Here If the pool is imported we have to set the value of common.SyncResources.IsImported = true

mittachaitu · 2020-03-02T07:26:38Z

pkg/pool/operations/disk_operation.go

+		return usedPath, nil
+	}
+
+	if len(oldPaths) == 0 {


Existing code comments: comments required saying specifcally for pool expansion case.

mittachaitu · 2020-03-02T07:33:20Z

pkg/pool/operations/modify.go

+		// If raidGroup doesn't have any blockdevice then remove that raidGroup
+		// and set isObjChanged
+		if isRaidGroupChanged {
+			if len(raidGroup.BlockDevices) == 0 {


Why raidGroup exists without blockdevices? Are there any such cases? Are we are supporting removing devices from raidGroups?

mittachaitu · 2020-03-02T09:34:46Z

pkg/controllers/cspi-controller/handler.go

+		status.Phase = cstor.CStorPoolInstancePhase(state)
+	}
+
+	freeSize, er := zpool.GetPoolCapacity(pool, "free")


IMO: Instead of making multiple calls to zfs good to get all the details in one call.

mittachaitu · 2020-03-02T09:43:08Z

pkg/pool/operations/pool_utils.go

+
+	devID := pool.GetDevPathIfNotSlashDev(bdPath[0])
+	if len(devID) != 0 {
+		cmdOut, err = zfs.NewPoolImport().WithDirectory(devID).Execute()


Is it good to log the error message?

prateekpandey14 · 2020-03-03T07:11:59Z

pkg/hash/hash.go

@@ -0,0 +1,36 @@
+/*


IMO better to have this in openebs/api/pkg/util/hash.go repo ?

other way is i am going to raise a PR in api repo and we can import this later as well.

prateekpandey14 · 2020-03-03T07:27:16Z

pkg/pool-manager-utils/common.go

@@ -0,0 +1,271 @@
+/*


we can name it to pkg/controller-utils as its a generics things can be used by cvr and cv controllers ? or have it inside controllers dir i.e. pkg/controllers/utils

Signed-off-by: Ashutosh Kumar <[email protected]>

sonasingh46 · 2020-03-03T10:18:26Z

I have incorporated some review comments. Few will be incorporated in the next PR.

Signed-off-by: Ashutosh Kumar <[email protected]>

prateekpandey14

lgtm , further changes will taken up in upcoming PR's

sonasingh46 force-pushed the cspc-operator-patch1 branch from 2c9a0e2 to eba4742 Compare February 19, 2020 11:46

sonasingh46 requested review from mittachaitu, shubham14bajpai and prateekpandey14 February 19, 2020 11:48

sonasingh46 force-pushed the cspc-operator-patch1 branch 4 times, most recently from 7a08de6 to 22271a3 Compare February 19, 2020 12:27

mittachaitu reviewed Feb 21, 2020

View reviewed changes

mittachaitu reviewed Feb 22, 2020

View reviewed changes

mittachaitu reviewed Feb 23, 2020

View reviewed changes

sonasingh46 force-pushed the cspc-operator-patch1 branch from 80958f1 to 5dceebb Compare February 26, 2020 08:55

sonasingh46 commented Feb 26, 2020

View reviewed changes

pkg/cspc/algorithm/build_deploy.go Outdated Show resolved Hide resolved

add cspc-operator and pool-manager

4056f72

Signed-off-by: Ashutosh Kumar <[email protected]>

sonasingh46 force-pushed the cspc-operator-patch1 branch from 72f4ff6 to 4056f72 Compare February 26, 2020 09:19

add vendor code

a1df5ba

Signed-off-by: Ashutosh Kumar <[email protected]>

prateekpandey14 reviewed Feb 26, 2020

View reviewed changes

pkg/cspc/algorithm/build_csp.go Outdated Show resolved Hide resolved

mittachaitu reviewed Feb 28, 2020

View reviewed changes

mittachaitu approved these changes Feb 28, 2020

View reviewed changes

Ashutosh Kumar added 2 commits February 28, 2020 16:10

vendor code changes

346053e

Signed-off-by: Ashutosh Kumar <[email protected]>

incorporate review comments

dfbc828

This commit removes some of the maya dependencies Signed-off-by: Ashutosh Kumar <[email protected]>

prateekpandey14 reviewed Feb 28, 2020

View reviewed changes

cspc-operator/main.go Outdated Show resolved Hide resolved

mittachaitu reviewed Mar 2, 2020

View reviewed changes

shubham14bajpai reviewed Mar 2, 2020

View reviewed changes

mittachaitu approved these changes Mar 2, 2020

View reviewed changes

prateekpandey14 reviewed Mar 3, 2020

View reviewed changes

incorporate review comments

a54361a

Signed-off-by: Ashutosh Kumar <[email protected]>

incorporate review comments

1d6a629

Signed-off-by: Ashutosh Kumar <[email protected]>

prateekpandey14 approved these changes Mar 3, 2020

View reviewed changes

prateekpandey14 merged commit aa7353d into openebs-archive:master Mar 3, 2020

add cspc-operator #1

add cspc-operator #1

Conversation

sonasingh46 commented Feb 19, 2020 • edited Loading

mittachaitu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mittachaitu Feb 21, 2020 • edited Loading

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

mittachaitu 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

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

mittachaitu 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

mittachaitu Mar 2, 2020 • edited Loading

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

sonasingh46 commented Mar 3, 2020

prateekpandey14 left a comment

Choose a reason for hiding this comment

sonasingh46 commented Feb 19, 2020 •

edited

Loading

mittachaitu Feb 21, 2020 •

edited

Loading

mittachaitu Mar 2, 2020 •

edited

Loading