Issue 312 making ss cache volumes optional #342

Prabhaker24 · 2020-03-16T11:12:01Z

Signed-off-by: prabhaker24 [email protected]

Change log description

From Pravega 0.7, the Segment Store cache is completely in-memory. This means that the PVC that the operator creates is not necessary anymore. We need to make the deployment of this volume optional, depending on whether the version of Pravega is older or newer than 0.7, thus these changes will deploy as well as upgrade the version accordingly so as to not include pvc's incase we are deploying or upgrading to a Pravega version 0.7 or above and remove the old ones deployed by a version below 0.7 in case of an upgrade.
In Case of a rollback when an upgrade from a version above 0.7 to a version below 0.7 fails, it should remove the pods of the segment store above version 0.7 and should deploy all segment store pods of version below 0.7 along with their pvc's

Purpose of the change

Fixes #312

What the code does

These changes will deploy the segment store with the pvc's for a version below 0.7 and will deploy the segment store without pvc's for a version 0.7 or above, it will also take care of upgrades in case the upgrades are from a version below 0.7 to a version above or equal to 0.7 and in this case it will remove the pvc's created by the older version while deploying the segment store for the version above 0.7.
It will also handle the rollback in case of an upgrade failure from a version below 0.7 to version above 0.7 and will display appropriate upgrade failed message and when the user changes the version to the last version(below 0.7 from which he was upgrading) it will rollback it to this below 0.7 version and remove the old pods of the segment store having version above 0.7.
It will also display appropriate user message incase the above rollback also fails.

How to verify it

Deploy Pravega with a version 0.7 or above and it should be deployed without any pvc's
Upgrade Pravega with a version blow 0.7 to a version above 0.7 it should do two this deploy the new segment store without pvc's and remove the old segment store as well as the pvc's attached to it.
Upgrade Pravega from a version below 0.7 to a version above 0.7
Upgrade Pravega from a version above 0.7 to a version above 0.7
Upgrade from 0.6.0 to 0.6.2 .
I/o should be working in case of all the above four processes both while the upgrade or deployment is in the process as well as when it's completed to check that Benchmark can be run.
Rollback in case the upgrade fails while upgrading from a version below 0.7 to a version below 0.7
Rollback in case the upgrade fails while upgrading from a version below 0.7 to a version above 0.7
Rollback in case the upgrade fails while upgrading from a version above 0.7 to a version above 0.7
Displays appropriate message in case rollback fails while doing a rollback if an upgrade fails from a version below 0.7 to a version above 0.7.

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

pkg/controller/pravega/pravega_segmentstore.go

pkg/controller/pravegacluster/pravegacluster_controller.go

pbelgundi · 2020-03-18T05:16:22Z

pkg/controller/pravegacluster/pravegacluster_controller.go

-	err = r.syncSegmentStoreSize(p)
-	if err != nil {
-		return err
+	/*this condition is to stop syncSegmentstore version from running when we are updapting the ss from version below 07


Could we just say in the comment " We skip calling syncSegmentStoreSize() during upgrade/rollback from version 07"

pbelgundi · 2020-03-18T05:17:12Z

pkg/controller/pravegacluster/pravegacluster_controller.go

-		err = r.syncStatefulSetPvc(sts)
-		if err != nil {
-			return fmt.Errorf("failed to sync pvcs of stateful-set (%s): %v", sts.Name, err)
+		// this check is to avoid calling syncStatefulSetPvc() over ss with version above 07


Same as above

pbelgundi · 2020-03-18T05:19:52Z

pkg/controller/pravegacluster/upgrade.go

+	stsAbove07 := &appsv1.StatefulSet{}
+	name := util.StatefulSetNameForSegmentstoreAbove07(p.Name)
+	err := r.client.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: p.Namespace}, stsAbove07)
+	if err != nil && errors.IsNotFound(err) {


What if client.Get() returns error, but it is not a "NotFound" error? Please add a check similar to :
https://github.com/pravega/pravega-operator/blob/master/pkg/controller/pravegacluster/pravegacluster_controller.go#L92

All errors must be logged with log level "Error". We're not logging anything here.

i have changed it

pbelgundi · 2020-03-18T05:26:47Z

pkg/controller/pravegacluster/upgrade.go

+	if (r.IsClusterRollbackingFrom07(p) && newsts.Status.ReadyReplicas == *newsts.Spec.Replicas) || (oldsts.Status.ReadyReplicas+newsts.Status.ReadyReplicas == p.Spec.Pravega.SegmentStoreReplicas && newsts.Status.ReadyReplicas == *newsts.Spec.Replicas) {
+		//this check is run till the value of old sts replicas is greater than 0 and will increase two replicas of the new sts and delete 2 replicas of the old sts
+		if *oldsts.Spec.Replicas > 2 {
+			*newsts.Spec.Replicas = *newsts.Spec.Replicas + 2


Encapsulate the changes to newSTS in a function and invoke that here.
Ditto for old STS, so its easier to follow what is happening.

pbelgundi · 2020-03-18T05:29:34Z

pkg/controller/pravegacluster/upgrade.go

+			}
+		} else {
+			//here we remove the pvc's attached with the old sts and deleted it when old sts replicas have become 0
+			*newsts.Spec.Replicas = p.Spec.Pravega.SegmentStoreReplicas


Again encapsulate changes to old STS in a method and invoke the same....
One more thing, we need to increment the newSTS first and then do stuff for deleting the old sts, so that we err on side of caution.... and can have more redundancy than less....

pbelgundi · 2020-03-18T05:32:00Z

pkg/util/pravegacluster.go

+	if ver == "" {
+		return true
+	}
+	first3 := strings.Trim(ver, "\t \n")[0:3]


Why can't we use CompareVersions here? Lets try to reuse existing methods wherever possible, Its is not good to use only way of doing things here and another way in the webhook.

pbelgundi · 2020-03-18T05:32:47Z

pkg/util/pravegacluster.go

+	return false
+}
+
+//this function will return true only in case of upgrading from a version below 0.7 to a version above 0.7


Fix the comment, "to pravega version 0.7 or later"

pkg/util/pravegacluster.go

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

pbelgundi

Please address all review comments. I still see some earlier comments open and unaddressed.

pkg/controller/pravegacluster/upgrade.go

pbelgundi · 2020-03-18T11:46:19Z

pkg/controller/pravegacluster/upgrade.go

+	}
+
+	//TO detect upgade/rolback faiure
+	if oldsts.Status.ReadyReplicas+newsts.Status.ReadyReplicas != p.Spec.Pravega.SegmentStoreReplicas {


!= check should be changed to < as discussed.

i have changed it

pbelgundi · 2020-03-18T11:47:05Z

pkg/controller/pravegacluster/upgrade.go

+	}
+
+	//checking if sts below07 exsists
+	stsBelow07 := &appsv1.StatefulSet{}


How are we dealing with situations where one STS is deleted and other is present?

that I have dealt with as one the above07 sts is there and below07 sts is not there I am checking the version, so in case the below07sts is not there and the version is below 07, in that case, I am calling the syncSegmentStoreVersionTo07() which will remove the above07 pods and add the below07 sts

in case the below07sts is there and above07 sts is not there I am returning false and syncSegmentStoreVersion() will get called which will handle this situation

pbelgundi · 2020-03-18T11:49:44Z

pkg/controller/pravegacluster/upgrade.go

+}
+
+//this function will increase two replicas of the new sts and delete 2 replicas of the old sts everytime it's called
+func (r *ReconcilePravegaCluster) incrementWhenReplicasMoreThan2(p *pravegav1alpha1.PravegaCluster, newsts *appsv1.StatefulSet, oldsts *appsv1.StatefulSet) error {


Could we call this method scaleSegmentStoreSTS()

pbelgundi · 2020-03-18T11:52:24Z

pkg/controller/pravegacluster/upgrade.go

+}
+
+//This function will remove the pvc's attached with the old sts and deleted it when old sts replicas have become 0
+func (r *ReconcilePravegaCluster) incrementWhenReplicasLessThan2(p *pravegav1alpha1.PravegaCluster, newsts *appsv1.StatefulSet, oldsts *appsv1.StatefulSet) error {


Please use a better method name that is indicative of what the method does..... Perhaps transitionToNewSTS() ???

pkg/util/pravegacluster.go

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

…are versions Signed-off-by: prabhaker24 <[email protected]>

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

pbelgundi

Some of the earlier comments are not addressed still and I have provided new comments.

pbelgundi · 2020-03-19T04:31:19Z

pkg/controller/pravegacluster/pravegacluster_controller.go

-		if err != nil {
-			return fmt.Errorf("failed to sync pvcs of stateful-set (%s): %v", sts.Name, err)
+		/*We skip calling syncStatefulSetPvc() during upgrade/rollback from version 07*/
+		if !util.IsClusterUpgradingTo07(p) && !r.isRollbackTriggered(p) {


Same as above. Why can't we use "IsClusterRollbackingFrom07()"?

I have changed it

pbelgundi · 2020-03-19T04:33:28Z

pkg/controller/pravegacluster/pravegacluster_controller.go

-	if err != nil {
-		return err
+	/*We skip calling syncSegmentStoreSize() during upgrade/rollback from version 07*/
+	if !util.IsClusterUpgradingTo07(p) && !r.isRollbackTriggered(p) {


We should be checking if rollback from 07 is triggered. Here we're checking if any rollback is triggered. Why can't we use "IsClusterRollbackingFrom07()"?

I have changed it

pkg/controller/pravegacluster/upgrade.go

pbelgundi · 2020-03-19T04:38:25Z

pkg/controller/pravegacluster/upgrade.go

+	return true
+}
+
+//To handle upgrade/rollback from Pravega version < 0.7 to Pravega Version >= 0.7


Wrong comment on method

pbelgundi · 2020-03-19T04:38:49Z

pkg/controller/pravegacluster/upgrade.go

+	if util.IsClusterUpgradingTo07(p) || r.IsClusterRollbackingFrom07(p) {
+		return r.syncSegmentStoreVersionTo07(p)
+	}
+	//for all other cases of upgrades and rollback this function is called


comment not removed yet.

pbelgundi · 2020-03-19T04:39:19Z

pkg/controller/pravegacluster/upgrade.go

+	controllerutil.SetControllerReference(p, newsts, r.scheme)
+	err = r.client.Get(context.TODO(), types.NamespacedName{Name: newsts.Name, Namespace: p.Namespace}, newsts)
+	//this check is to see if the newsts is present or not if it's not present it will be created here
+	if err != nil && errors.IsNotFound(err) {


I don;t see the error being logged????

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

pbelgundi · 2020-03-19T12:13:50Z

pkg/controller/pravegacluster/upgrade.go

+				return false, err
+			}
+		}
+		if !errors.IsNotFound(err) {


This if is not needed, please remove.

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

…into issue-312-making-ss-cache-volume-optional

pbelgundi

LGTM

jkhalack · 2020-03-31T19:09:36Z

pkg/util/pravegacluster.go

+
+//if version is above or equals to 0.7 this name will be assigned
+func StatefulSetNameForSegmentstoreAbove07(name string) string {
+	return fmt.Sprintf("%s-pravega-segment-store", name)


This change just broke the upgrade.

0.7.0 pravega clusters historically were deployed with 0.4.x p-operator and had the stateful sets named using the pravega-segmentstore substring.

prabhaker24 added 3 commits March 16, 2020 15:47

util functions to return names of ss based on version included

3fcde06

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

update and rollback code added

90aa3d1

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

fixed format fail issue

61f6afe

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

Prabhaker24 requested a review from pbelgundi March 16, 2020 11:41

small fix added

1ac0810

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

Prabhaker24 marked this pull request as ready for review March 16, 2020 15:07

ut's added

268cb58

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

pbelgundi requested changes Mar 18, 2020

View reviewed changes

prabhaker24 added 3 commits March 18, 2020 17:02

made the code more modular and fixed typos

33daee3

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

small fix

8592bb6

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

fixed typo

2c510c5

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

pbelgundi requested changes Mar 18, 2020

View reviewed changes

prabhaker24 added 4 commits March 18, 2020 17:49

changed names of some functions

dbc4708

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

code for handling corner case added as well as switched to using comp…

5b73d94

…are versions Signed-off-by: prabhaker24 <[email protected]>

canged the util function to use compare version

66c9285

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

removed unnecessary comments

07455ed

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

pbelgundi requested changes Mar 19, 2020

View reviewed changes

prabhaker24 and others added 2 commits March 19, 2020 13:33

changed rollback logic and fixed comments

6a607c5

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

updated upgrade.go

7ade426

pbelgundi requested changes Mar 19, 2020

View reviewed changes

prabhaker24 added 2 commits March 19, 2020 18:36

removed unnecessary if

3c360a5

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

Merge branch 'master' of https://github.com/pravega/pravega-operator …

10a302c

…into issue-312-making-ss-cache-volume-optional

pbelgundi approved these changes Mar 19, 2020

View reviewed changes

pbelgundi merged commit 835e7e6 into master Mar 20, 2020

pbelgundi mentioned this pull request Mar 20, 2020

Issue 312 making ss cache volumes optional #335

Closed

jkhalack reviewed Mar 31, 2020

View reviewed changes

Issue 312 making ss cache volumes optional #342

Issue 312 making ss cache volumes optional #342

Conversation

Prabhaker24 commented Mar 16, 2020

Change log description

Purpose of the change

What the code does

How to verify it

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

pbelgundi 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

Prabhaker24 Mar 18, 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

pbelgundi 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

pbelgundi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prabhaker24 Mar 18, 2020 •

edited

Loading