feat(PDB, cStor Pools): add a support to create PDB for cStor #1573

mittachaitu · 2020-01-03T06:44:22Z

Signed-off-by: mittachaitu sai.chaithanya@mayadata.io

What this PR does:
This PR adds support to create PodDisruptionBudget only for cStor Pool pods
which contains HA volume replicas(volume provisioned with >= 3 replicas) in cStor pools.

why we need it:
Valid voluntary disruptions will be disallowed on cStor pool pods if eviction of cStor pool pod affects the volume quorum factor.

Draining a node for repair or upgrade.
Draining a node from a cluster to scale the cluster down.

Example for PDB creation:
Provisioned a CSPC with five pool specs that intern creates 5 cStor pools. Now hundreds of HA volumes are provisioned with 3 replicas and volume replicas can be scheduled in any random order across CSPC cStor pools, no.of PDBs’ will be combination among the cStor pools. In this case, at most 5C3 PDBs will be created i.e 10 PDBs will be created[what if my storage replica count is 4? 5C3 + 5C4 PDB will be created].

Sample PDB YAML:

apiVersion: policy/v1beta1
kind: PodDisruptionBudget
metadata:
  creationTimestamp: "2020-01-03T07:21:28Z"
  generateName: cstor-sparse-cspc
  generation: 1
  labels:
    openebs.io/cstor-pool-cluster: cstor-sparse-cspc
    openebs.io/cstor-sparse-cspc-jdgt: true
    openebs.io/cstor-sparse-cspc-klx6: true
    openebs.io/cstor-sparse-cspc-tblc: true
  name: cstor-sparse-cspcvlhqs
  namespace: openebs
  resourceVersion: "358868"
  selfLink: /apis/policy/v1beta1/namespaces/openebs/poddisruptionbudgets/cstor-sparse-cspcvlhqs
  uid: 7037b472-87b1-4dc3-ae1b-808b5265c043
spec:
  minAvailable: 2
  selector:
    matchExpressions:
    - key: openebs.io/cstor-pool-instance
      operator: In
      values:
      - cstor-sparse-cspc-jdgt
      - cstor-sparse-cspc-klx6
      - cstor-sparse-cspc-tblc
    matchLabels:
      app: cstor-pool
status:
  currentHealthy: 3
  desiredHealthy: 2
  disruptionsAllowed: 1
  expectedPods: 3
  observedGeneration: 1

Sample CVC yaml after refering to PDB:

apiVersion: openebs.io/v1alpha1
kind: CStorVolumeClaim
metadata:
  annotations:
    openebs.io/volumeID: pvc-40dbc220-f1f3-45a2-b054-4d2add1450b0
  creationTimestamp: "2020-01-03T07:30:32Z"
  finalizers:
  - cvc.openebs.io/finalizer
  generation: 3
  labels:
    openebs.io/cstor-pool-cluster: cstor-sparse-cspc
    openebs.io/pod-disruption-budget: cstor-sparse-cspcq65jf 
  name: pvc-40dbc220-f1f3-45a2-b054-4d2add1450b0
  namespace: openebs
  resourceVersion: "362110"
  selfLink: /apis/openebs.io/v1alpha1/namespaces/openebs/cstorvolumeclaims/pvc-40dbc220-f1f3-45a2-b054-4d2add1450b0
  uid: 706aeac8-789d-4084-b5db-c82c353ff4ec
publish:
  nodeId: gke-shubham-test-default-pool-205971c3-122z
spec:
  capacity:
    storage: 5Gi
  cstorVolumeRef:
    apiVersion: openebs.io/v1alpha1
    kind: CStorVolume
    name: pvc-40dbc220-f1f3-45a2-b054-4d2add1450b0
    namespace: openebs
    resourceVersion: "362087"
    uid: 67171169-4b30-4167-bcc4-9e0fbcab159b
  replicaCount: 3

In above CVC openebs.io/pod-disruption-budget: cstor-sparse-cspcq65jf
label is added.

Design Doc:
https://docs.google.com/document/d/1Pq2ZDE7K1ttmqdJl4LgZW1B6JImxzJsLGrydOpjV7rs/edit?usp=sharing
OEP for PDB:
openebs/openebs#2887

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Checklist:

Fixes #
Labelled this PR & related issue with documentation tag
PR messages has document related information
Labelled this PR & related issue with breaking-changes tag
PR messages has breaking changes related information
Labelled this PR & related issue with requires-upgrade tag
PR messages has upgrade related information
Commit has unit tests
Commit has integration tests

cmd/cvc-operator/controller/cvc_controller.go

shubham14bajpai

LGTM. Just added a few nit comments.

pkg/kubernetes/poddisruptionbudget/kubernetes.go

vishnuitta · 2020-01-28T16:23:40Z

cmd/cvc-operator/controller/cstorvolumeclaim.go

+// 3. If PDB doesn't exist it creates new PDB(With CSPC hash)
+func getOrCreatePodDisruptionBudget(
+	cvObj *apis.CStorVolume, cspcName string) (*policy.PodDisruptionBudget, error) {
+	poolNames, err := cvr.GetReplicaPoolNames(cvObj)


cvr.GetReplicaPoolNames(cvobj) doesn't makes sense and can lead to circular dependency.. make it as cvobj.GetReplicaPoolNames()

It will not lead to circular dependency but passing cvObj in CVR package doesn't make sense I will update it.

updated it as cvr.GetVolumeReplicaPoolNames(pvName, namespace)

pkg/apis/openebs.io/v1alpha1/cas_keys.go

cmd/cvc-operator/controller/cstorvolumeclaim.go

vishnuitta

we need to remove Gopkg.lock file?

pkg/apis/openebs.io/v1alpha1/cas_pool.go

vishnuitta

fix the sample PDB yaml in the PR description

vishnuitta · 2020-01-28T17:27:17Z

cmd/cvc-operator/controller/cstorvolumeclaim.go

+		return nil, errors.Wrapf(err,
+			"failed to list PDB belongs to pools %v", pdbLabels)
+	}
+	if len(pdbList.Items) > 1 {


does this work even if multiple CVC controllers are running at same time?

Looks like we have to take a lease on PDB also when we take on CVC.

It will work without any changes

vishnuitta

is this PR takes care of multiple CVC controllers as well?

cmd/cvc-operator/controller/cvc_controller.go

vishnuitta · 2020-01-28T17:34:42Z

cmd/cvc-operator/controller/cvc_controller.go

@@ -263,6 +275,16 @@ func (c *CVCController) createVolumeOperation(cvc *apis.CStorVolumeClaim) (*apis
 		return nil, err
 	}

+	if cvclaim.IsHAVolume(cvc) {


this being small function and doesn't depend on package related things, cvc.IsHAVolume() looks to be good

To define cvc.IsHAVolume() we have to define in apis package i.e pkg/apis/openebs.io/v1alpha1 which is not good practice. So I will define in same cmd/cvc-operator/controller/cvc_controller.go file

cmd/cvc-operator/controller/cvc_controller.go

vishnuitta · 2020-01-28T17:43:26Z

cmd/cvc-operator/controller/cvc_controller.go

+	cvcLabelSelector := string(apis.PodDisruptionBudgetKey) + "=" + pdbName
+	cvcList, err := c.clientset.
+		OpenebsV1alpha1().
+		CStorVolumeClaims(cvc.Namespace).


can there be CVCs in different namespace using same PDB?

No!! With current design all the OpenEBS related resources will be in one namespace where OpenEBS installed.

vishnuitta · 2020-01-28T17:46:04Z

cmd/cvc-operator/controller/cstorvolumeclaim.go

+	}
+	// Create podDisruptionBudget
+	return apispdb.KubeClient().
+		WithNamespace(cvObj.Namespace).


we are creating in cvObj, but, listing in getNamespace().. can they be different?

Always both will be the same but to maintain consistency I will update to it to getNamesapce()...

pkg/apis/openebs.io/v1alpha1/cstor_volume_claim.go

pkg/apis/openebs.io/v1alpha1/cas_pool.go

vishnuitta

testcases are required

mittachaitu · 2020-01-28T18:13:04Z

testcases are required

#1574 This PR will cover one test case a few others will be rolled out one this is merged.

mittachaitu · 2020-01-28T18:38:35Z

we need to remove Gopkg.lock file?

Why do we need to remove it?

mittachaitu · 2020-01-28T18:42:58Z

is this PR takes care of multiple CVC controllers as well?

No Yes

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

cmd/cvc-operator/controller/cstorvolumeclaim.go

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

vishnuitta

changes are good

mittachaitu mentioned this pull request Jan 3, 2020

[WIP] feat(cspc, pdb): add support to create PDB for cStor pools #1569

Closed

9 tasks

mittachaitu requested review from sonasingh46 and prateekpandey14 January 3, 2020 06:47

golangcibot reviewed Jan 3, 2020

View reviewed changes

cmd/cvc-operator/controller/cvc_controller.go Outdated Show resolved Hide resolved

cmd/cvc-operator/controller/cvc_controller.go Outdated Show resolved Hide resolved

This was referenced Jan 3, 2020

chore(openebs-operator yaml): add poddisruptionbudgets to openebs clusterrole openebs/openebs#2873

Merged

[WIP] feat(PDB, BDD): add test to verify PDB creation #1574

Closed

mittachaitu added this to the 1.7.0 milestone Jan 6, 2020

mittachaitu self-assigned this Jan 6, 2020

mittachaitu added needs-review pr/release-note PR should be included in release notes labels Jan 6, 2020

mittachaitu force-pushed the combinations_pdb_cstor branch from 44644a1 to 29acf37 Compare January 14, 2020 11:29

sonasingh46 approved these changes Jan 16, 2020

View reviewed changes

mittachaitu requested a review from shubham14bajpai January 20, 2020 07:03

shubham14bajpai approved these changes Jan 28, 2020

View reviewed changes

pkg/kubernetes/poddisruptionbudget/kubernetes.go Outdated Show resolved Hide resolved

pkg/kubernetes/poddisruptionbudget/kubernetes.go Outdated Show resolved Hide resolved