feat(upgrade): add support to migrate jiva volumes resources to openebs namespace #1646

shubham14bajpai · 2020-04-02T12:04:02Z

Signed-off-by: shubham shubham.bajpai@mayadata.io

What this PR does / why we need it:
This PR adds support to migrate the old jiva volume resources to openebs namespace.

Test job logs:

mayadata:upgrade$ k logs -f jiva-vol-170180-pvc-fc34fbbd-6661-4f40-9-39a3-qjgtt
I0402 11:54:14.024428       1 jiva_volume.go:83] Started upgrading jivaVolume{pvc-be1d95bf-205d-49e2-8bc6-83f4c8ba42af} from 1.6.0 to 1.9.0-RC1
I0402 11:54:14.024504       1 jiva_volume.go:90] Upgrading to 1.9.0-RC1
I0402 11:54:14.117842       1 jiva_upgrade.go:413] Scaling down old deployments
I0402 11:54:14.117872       1 helper.go:325] Scaling deploy pvc-be1d95bf-205d-49e2-8bc6-83f4c8ba42af-ctrl in default namespace to 0
I0402 11:54:29.171796       1 helper.go:325] Scaling deploy pvc-be1d95bf-205d-49e2-8bc6-83f4c8ba42af-rep in default namespace to 0
I0402 11:54:44.241182       1 jiva_upgrade.go:695] splitting replica deployment
I0402 11:54:44.244362       1 jiva_upgrade.go:717] creating replica deployment pvc-be1d95bf-205d-49e2-8bc6-83f4c8ba42af-rep-1 in openebs namespace
I0402 11:54:49.270392       1 jiva_upgrade.go:717] creating replica deployment pvc-be1d95bf-205d-49e2-8bc6-83f4c8ba42af-rep-2 in openebs namespace
I0402 11:54:54.305663       1 jiva_upgrade.go:717] creating replica deployment pvc-be1d95bf-205d-49e2-8bc6-83f4c8ba42af-rep-3 in openebs namespace
I0402 11:54:59.352312       1 jiva_upgrade.go:762] creating controller deployment pvc-be1d95bf-205d-49e2-8bc6-83f4c8ba42af-ctrl in openebs namespace
I0402 11:55:04.389390       1 jiva_upgrade.go:781] removing controller service pvc-be1d95bf-205d-49e2-8bc6-83f4c8ba42af-ctrl-svc in default namespace
I0402 11:55:04.425101       1 jiva_upgrade.go:786] creating controller service pvc-be1d95bf-205d-49e2-8bc6-83f4c8ba42af-ctrl-svc in openebs namespace
I0402 11:55:09.522279       1 jiva_upgrade.go:318] Verifying replica sync
I0402 11:55:15.686415       1 jiva_upgrade.go:350] Synced replica quorum is reached
I0402 11:55:43.949692       1 jiva_upgrade.go:354] Replica syncing complete
I0402 11:55:43.991904       1 jiva_upgrade.go:654] Upgrade Successful forpvc-be1d95bf-205d-49e2-8bc6-83f4c8ba42af
I0402 11:55:44.002157       1 jiva_volume.go:106] Upgraded successfully

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

Signed-off-by: shubham <shubham.bajpai@mayadata.io>

kmova · 2020-04-02T17:45:11Z

How does the code handle the following cases:

The previous version jiva volume was deployed with "DeployInOpenEBSNamespace=true"
If there is a failure after the previous target service is deleted.
And in general if during migrate, the job fails and on subsequent restarts it doesn't come backup up and user wants to revert to old version. This case need not be fixed, but at least the steps to recover from this state should be documented.

vishnuitta · 2020-04-02T18:52:13Z

pkg/upgrade/upgrader/jiva_upgrade.go

@@ -411,6 +409,20 @@ func (j *jivaVolumeOptions) preupgrade(pvName, openebsNamespace string) error {
 		return err
 	}

+	if currentVersion < "1.9.0" {
+		klog.Infof("Scaling down old deployments")
+		err = j.preMigration(openebsNamespace)


name preMigration as something to say if less than 1.9.0

vishnuitta · 2020-04-02T19:01:31Z

pkg/upgrade/upgrader/jiva_upgrade.go

+	if currentVersion < "1.9.0" {
+		klog.Infof("Scaling down old deployments")
+		err = j.preMigration(openebsNamespace)
+		if err != nil {


in case of failures, do we need to revert the partial upgrade that has been done so that operations go fine? or would that be out of scope of upgrade jobs?
Did we handled such things like reverting partial upgrade in any previous failures?
is this scope for higher level operator (or) need to follow blue-green deployments?

vishnuitta · 2020-04-02T19:10:07Z

pkg/upgrade/upgrader/jiva_upgrade.go

+	return nil
+}
+
+func (j *jivaVolumeOptions) migrateReplica(openebsNamespace string) error {


change this function name as well to reflect version < 1.9.0

vishnuitta · 2020-04-02T19:12:17Z

pkg/upgrade/upgrader/jiva_upgrade.go

+		// Scaling down controller deployment before patching replica deployment
+		// if the replica is not upgraded already.
+		if j.replicaObj.version == currentVersion {
+			err = scaleDeploy(j.controllerObj.name, j.ns, ctrlDeployLabel, 0)


can this be moved to part of preUpgarde rather than being part of replicaUpgrade?

vishnuitta · 2020-04-02T19:18:22Z

pkg/upgrade/upgrader/jiva_upgrade.go

 func jivaUpgrade(pvName, openebsNamespace string, utaskObj *utask.UpgradeTask) (*utask.UpgradeTask, error) {

 	var (
-		pvLabel = "openebs.io/persistent-volume=" + pvName
-		err     error
+		pvLabel      = "openebs.io/persistent-volume=" + pvName


convert these hardcoded strings as constants like apis.PersistentVolumeCPK

vishnuitta · 2020-04-02T19:21:00Z

pkg/upgrade/upgrader/jiva_upgrade.go

+			return err
+		}
+		nodeNames := deployObj.Spec.Template.Spec.Affinity.NodeAffinity.
+			RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions[0].Values


can it be sure that first element is the one that we always want?

I see if-else in CASTemplates

vishnuitta · 2020-04-02T19:35:02Z

pkg/upgrade/upgrader/jiva_upgrade.go

+			RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions[0].Values
+		var one int32 = 1
+		for i, node := range nodeNames {
+			replicaDeploy := deployObj.DeepCopy()


add validation for each nodeName to verify that it exists

vishnuitta · 2020-04-02T19:38:18Z

pkg/upgrade/upgrader/jiva_upgrade.go

+			// updating the replica deployments image and version before
+			// creating them in openebs namespace.
+			lastIndex := strings.LastIndex(replicaDeploy.Spec.Template.Spec.Containers[0].Image, ":")
+			replicaDeploy.Spec.Template.Spec.Containers[0].Image = replicaDeploy.Spec.


can this be avoided and let regular patch takes care of updating image name, version etc?

vishnuitta · 2020-04-02T19:42:43Z

pkg/upgrade/upgrader/jiva_upgrade.go

+			replicaDeploy.Name = replicaDeploy.Name + "-" + strconv.Itoa(i+1)
+			replicaDeploy.Namespace = openebsNamespace
+			replicaDeploy.ResourceVersion = ""
+			replicaDeploy.Spec.Replicas = &one


as part of postMigration i.e., after replicaPatch is done, scale up the deployments

vishnuitta · 2020-04-02T19:43:43Z

pkg/upgrade/upgrader/jiva_upgrade.go

+			return err
+		}
+		var one int32 = 1
+		deployObj.Spec.Replicas = &one


same comments as in migrateReplica applies here

vishnuitta · 2020-04-02T19:44:07Z

pkg/upgrade/upgrader/jiva_upgrade.go

+		if err != nil {
+			return err
+		}
+		svcList, err := serviceClient.WithNamespace(j.ns).List(metav1.ListOptions{


move this to migrateService

vishnuitta · 2020-04-02T19:46:45Z

pkg/upgrade/upgrader/jiva_upgrade.go

+		if err != nil {
+			return err
+		}
+		klog.Infof("creating controller service %s in %s namespace", svcObj.Name, openebsNamespace)


what if upgrade job dies after deleting service? we will be losing the IP..
is it possible to create service with same IP but in disabled mode?
If so, first create in disabled mode, delete old one and then enable new one?

vishnuitta · 2020-04-02T19:49:59Z

pkg/upgrade/upgrader/jiva_upgrade.go

 		}
-	}
+	} else {


this code structure doesn't look neat..
better to allow old code to get executed by reducing code in new path as provided in comments there
Same applies for target also

its also required to allow executing existing code path as 'patch' may keep changing as we make new releases..
By combining upgrade code into migration path, we will miss the patches that need to be done in new releases.

vishnuitta

My other suggestion is:

As backward compatibility is already there, document the steps for migration and let user perform the steps

- segregate migration from upgarde Signed-off-by: shubham <shubham.bajpai@mayadata.io>

kmova · 2020-04-04T05:32:04Z

pkg/upgrade/upgrader/jiva_upgrade.go

+		return nil, err
+	}
+	svcObj := &corev1.Service{}
+	svcObj.ObjectMeta = metav1.ObjectMeta{


how about getting the existing service object and then doing a deep copy, patching the namespace. I guess this logic is written to account for the case where the previous service object is already deleted and then possible job also is restarted and will have to now go ahead and create the new service.

Can there be an alternate solution like:

create a temporary service - with a new name and deleting the older cluster ip.

delete orig service

create new service, by copying from temp service and using the target ip (from PV) as the cluster ip

as part of cleanup, delete the temporary service.

…er verifying upgrade Signed-off-by: shubham <shubham.bajpai@mayadata.io>

vishnuitta · 2020-04-05T16:05:29Z

pkg/upgrade/upgrader/jiva_upgrade.go

+		}
+		if err == nil {
+			klog.Info("getting old deployment")
+			if deployObj.Name == "" {


this can never happen

vishnuitta · 2020-04-05T17:18:44Z

pkg/upgrade/upgrader/jiva_upgrade.go

+	if j.ns == openebsNamespace {
+		replicasLeft = replicasLeft - 1
+	}
+	if len(deployList.Items) != replicaCount {


is there a chance where old replica deployment got scaled down and upgrade process got restarted without completing upgrade?
If such thing happens, replicaCount will be 0 here.

instead of depending on spec.Replicas to get replicationFactor, better to depend on len of nodeNames in NodeAffinity..

vishnuitta · 2020-04-05T17:47:39Z

pkg/upgrade/upgrader/jiva_upgrade.go

+				RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms[0].MatchExpressions[0].Values = []string{nodeNames[i]}
+			klog.Infof("creating replica deployment %s in %s namespace", replicaDeploy.Name, openebsNamespace)
+			replicaDeploy, err := deployClient.WithNamespace(openebsNamespace).Create(replicaDeploy)
+			if err != nil {


by handling 'IsAlreadyExists' error, we can just keep creating on every iteration without much logic in getting the count.
Current code of getting count of replicas to be created is also fine.

vishnuitta · 2020-04-05T17:47:57Z

pkg/upgrade/upgrader/jiva_upgrade.go

+		return err
+	}
+	replicaCount := int(*oldDeployObj.Spec.Replicas)
+	replicasLeft := len(deployList.Items)


variable name should be replicasCreated?

vishnuitta · 2020-04-05T17:50:50Z

pkg/upgrade/upgrader/jiva_upgrade.go

+
+func (j *jivaVolumeOptions) migrateTarget(pvName, openebsNamespace string) error {
+	// get the controller deployment in openebs namespace
+	deployObj, err := deployClient.WithNamespace(openebsNamespace).Get(j.controllerObj.name)


instead of doing Get, by just checking 'j.ns' for openebsNamespace and returning should be fine

ok.. this Get call helps in case of upgrade restart also..

vishnuitta · 2020-04-05T17:53:25Z

pkg/upgrade/upgrader/jiva_upgrade.go

+func (j *jivaVolumeOptions) migrateTarget(pvName, openebsNamespace string) error {
+	// get the controller deployment in openebs namespace
+	deployObj, err := deployClient.WithNamespace(openebsNamespace).Get(j.controllerObj.name)
+	if err != nil && !k8serror.IsNotFound(err) {


you can return for err as nil also, and can avoid next 'if' condition

vishnuitta · 2020-04-05T18:00:36Z

pkg/upgrade/upgrader/jiva_upgrade.go

-	if j.replicaObj.version == currentVersion {
-		err = scaleTargetDeploy(j.controllerObj.name, j.ns, 0)
+	if currentVersion < "1.9.0" {
+		err = scaleDeploy(j.controllerObj.name, j.ns, ctrlDeployLabel, 0)


don't we want to scale down controller post 1.9 also?

vishnuitta · 2020-04-05T18:08:33Z

pkg/upgrade/upgrader/jiva_upgrade.go

+
+func (j *jivaVolumeOptions) migrateTargetSVC(pvName, openebsNamespace string) error {
+	// migrate service only if service not in openebs namespace
+	if j.ns != openebsNamespace {


do reversing the condition

vishnuitta · 2020-04-05T18:30:52Z

pkg/upgrade/upgrader/jiva_upgrade.go

+	return nil
+}
+
+func getTargetSVC(pvName, openebsNamespace string) (*corev1.Service, error) {


needs validation with CASTemplate..
@utkarshmani1997 can you do this review?

vishnuitta · 2020-04-05T18:33:52Z

pkg/upgrade/upgrader/jiva_upgrade.go

+	if j.replicaObj.version == currentVersion && j.replicaObj.name != "" {
+		klog.Info("cleaning old replica deployment")
+		err = deployClient.WithNamespace(j.ns).Delete(j.replicaObj.name, &metav1.DeleteOptions{})
+		if err != nil {


handle isNotFound error as not error

If the job restarts and the replica deployment is missing the getReplica function will not populate replicaObj.name and the check j.replicaObj.name != "" will skip the deletion.

vishnuitta · 2020-04-05T18:37:13Z

pkg/upgrade/upgrader/jiva_upgrade.go

+			return err
+		}
+	}
+	if j.controllerObj.version == currentVersion && j.ns != openebsNamespace {


if upgrade job gets restarted at this point, replicaObj.version will not be set in getReplica call.. and all the upgrade process is dependent on this field replicaObj.version

same problem with replicaObj.name also

Signed-off-by: shubham <shubham.bajpai@mayadata.io>

utkarshmani1997

LGTM

vishnuitta · 2020-04-06T13:30:02Z

pkg/upgrade/upgrader/jiva_upgrade.go

+			if err != nil {
+				return nil, err
+			}
+			replicaObj.replicas[deployObj.Name] = version


consider adding logs at required places

logs like those in patchReplica would be good to be there in new functions

kmova · 2020-04-06T13:40:27Z

pkg/upgrade/upgrader/helper.go

@@ -59,6 +60,23 @@ func getOpenEBSVersion(d *appsv1.Deployment) (string, error) {
 	return d.Labels["openebs.io/version"], nil
 }

+func checkOpenEBSVersion(d *appsv1.Deployment) (string, error) {


Should this function be called isValidVersion? Also there is a message in this function about replica.

kmova · 2020-04-06T13:44:18Z

pkg/upgrade/upgrader/jiva_upgrade.go

+	return patchDetails, nil
+}
+
+func getReplica(pvName, replicaLabel, namespace, openebsNamespace string) (*replicaDetails, error) {


n_: just to be clear, the variable namespace can be renamed as volumeNamespace

vishnuitta · 2020-04-06T13:48:16Z

pkg/upgrade/upgrader/jiva_upgrade.go

-			metav1.ListOptions{
-				LabelSelector: controllerLabel,
-			})
+func (j *jivaVolumeOptions) preReplicaUpgradeLessThan190(pvName, openebsNamespace string) (string, error) {


comments for the functions

vishnuitta · 2020-04-06T13:49:10Z

pkg/upgrade/upgrader/jiva_upgrade.go

+			"openebs.io/persistent-volume":       pvName,
+			"openebs.io/persistent-volume-claim": pvObj.Spec.ClaimRef.Name,
+			"pvc":                                pvObj.Spec.ClaimRef.Name,
+			"openebs.io/version":                 currentVersion,


is this supposed to be UpgradeVersion?

ok.. we are updating this in patchService fn

vishnuitta · 2020-04-06T13:51:11Z

pkg/upgrade/upgrader/jiva_upgrade.go

-			})
+func (j *jivaVolumeOptions) preReplicaUpgradeLessThan190(pvName, openebsNamespace string) (string, error) {
+	if currentVersion < "1.9.0" {
+		err := scaleDeploy(j.replicaObj.name, j.ns, replicaDeployLabel, 0)


can replicaObj.name be nil here?

Yes. Added appropriate checks.

kmova · 2020-04-06T14:04:52Z

pkg/upgrade/upgrader/jiva_upgrade.go

-	}
-	replicaObj.name = deployObj.Name
-	version, err := getOpenEBSVersion(deployObj)
+func getPatchDetailsForDeploy(pvName string, deployObj *appsv1.Deployment) (*replicaPatchDetails, error) {


n_: As this is specific to Replica Patch. Can we change this to getPatchDetailsForReplicaDeploy

vishnuitta · 2020-04-06T14:09:40Z

pkg/upgrade/upgrader/jiva_upgrade.go

+	// check if old replica is present for currentVersion < 1.9.0
+	// if present then migration is not complete and store the old
+	// replica details
+	if currentVersion < "1.9.0" {


Above comments are good..
add few more like
'replicaObj.name and replicaObj.version would be empty if old replica got deleted as part of upgrade.
So, later on code uses replicaObj.name to perform replica related migration.'

kmova · 2020-04-06T14:47:50Z

pkg/upgrade/upgrader/jiva_upgrade.go

+
+	klog.Infof("splitting replica deployment")
+	var zero int32
+	for i := replicasCreated; i < replicaCount; i++ {


One comment above this for loop to say that:
replica deployment pv-name-rep will be split into multiple replicas like pv-name-rep-1, pv-name-rep-2,... pv-name-rep-n, where n is the replica count for this volume.

kmova · 2020-04-06T14:57:49Z

pkg/upgrade/upgrader/jiva_upgrade.go

+
+func (j *jivaVolumeOptions) cleanup(openebsNamespace string) error {
+	var err error
+	if j.replicaObj.version == currentVersion && j.replicaObj.name != "" {


can we also add a check on the namespace here.

Namespace check is not required as irrespective of namespace either openebs or pvc, the old deployment pvName-rep is deleted after splitting it.

kmova · 2020-04-06T14:58:29Z

pkg/upgrade/upgrader/jiva_upgrade.go

+		}
+	}
+	if j.controllerObj.version == currentVersion && j.ns != openebsNamespace {
+		klog.Info("cleaning old replica deployment")


n_: this should be deleting controller(or target) deployment.

kmova · 2020-04-06T15:00:53Z

pkg/upgrade/upgrader/jiva_upgrade.go

@@ -584,6 +611,18 @@ func (j *jivaVolumeOptions) verify(pvLabel, openebsNamespace string) error {
 		}
 		return err
 	}
+	if currentVersion < "1.9.0" {


It may be possible that the job dies after cleaning up old replica deployments. So when the job restarts, will it still know that it has to go and cleanup the pending target deployment. One thought is -- just call the j.cleanup anyway without version check, and that cleanup will only delete stale entries.

If job dies after cleanup is successful it will not attempt to delete the deployment as it will not be able to find old deployment and the replicaObj.name variable will be empty and we check on that before deleting the deployment.

Signed-off-by: shubham <shubham.bajpai@mayadata.io>

vishnuitta

changes are good

feat(upgrade): add support to migrate old jiva volumes

c85749c

Signed-off-by: shubham <shubham.bajpai@mayadata.io>

shubham14bajpai requested review from vishnuitta, kmova and sagarkrsd April 2, 2020 12:04

kmova added this to the 1.9 milestone Apr 2, 2020

vishnuitta reviewed Apr 2, 2020

View reviewed changes

- make the job more fault resistant

56499e9

- segregate migration from upgarde Signed-off-by: shubham <shubham.bajpai@mayadata.io>

kmova reviewed Apr 4, 2020

View reviewed changes

move migration code together and cleanup of old resources is done aft…

b4de773

…er verifying upgrade Signed-off-by: shubham <shubham.bajpai@mayadata.io>

vishnuitta reviewed Apr 5, 2020

View reviewed changes

adressed review comments

b8e8909

Signed-off-by: shubham <shubham.bajpai@mayadata.io>

shubham14bajpai requested a review from utkarshmani1997 April 6, 2020 09:56

utkarshmani1997 approved these changes Apr 6, 2020

View reviewed changes

vishnuitta reviewed Apr 6, 2020

View reviewed changes

kmova reviewed Apr 6, 2020

View reviewed changes

vishnuitta reviewed Apr 6, 2020

View reviewed changes

kmova reviewed Apr 6, 2020

View reviewed changes

vishnuitta reviewed Apr 6, 2020

View reviewed changes

kmova reviewed Apr 6, 2020

View reviewed changes

update function comments and names of variables & functions

607c732

Signed-off-by: shubham <shubham.bajpai@mayadata.io>

kmova approved these changes Apr 6, 2020

View reviewed changes

vishnuitta approved these changes Apr 6, 2020

View reviewed changes

vishnuitta merged commit d1c471e into openebs-archive:master Apr 6, 2020

feat(upgrade): add support to migrate jiva volumes resources to openebs namespace #1646

feat(upgrade): add support to migrate jiva volumes resources to openebs namespace #1646

Conversation

shubham14bajpai commented Apr 2, 2020

kmova commented Apr 2, 2020

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

vishnuitta 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

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

utkarshmani1997 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

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

vishnuitta left a comment

Choose a reason for hiding this comment