feat(migration): adding support to migrate the PV to a new node #304

pawanpraka1 · 2021-04-06T06:06:44Z

Why is this PR required? What issue does it fix?:

Here we are setting the node affinity on PV using nodename, so we can not migrate the pods to the new node as k8s scheduler will try to schedule on the old node only.

What this PR does?:

Here, we can now label the nodes with the unique values using the key openebs.io/nodeid. The ZFS LocalPV driver will pick the value from the nodes and set the affinity accordingly.

Now to migrate the PV to the other node, we have to move the disks to the other node and remove the old node from the cluster and set the same label on the new node using the same key and value, which will let k8s scheduler to schedule the pods to the that node.

Refactoring?:

We are using nodename as NodeID in code, refactored the code and made it NodeName to have better code readability. Also changed the ZFSVolume display string to NodeID from the strting Node to tell that it is the NodeID.

Limitation?:

we can only move the PVs to the new node, we can not move the PVs to the node which was already used in the cluster as there is only one allowed value for the custom key for setting the node label.

akhilerm · 2021-04-08T14:42:00Z

cmd/main.go

@@ -88,7 +88,7 @@ func run(config *config.Config) {

 	klog.Infof("ZFS Driver Version :- %s - commit :- %s", version.Current(), version.GetGitCommit())
 	klog.Infof(
-		"DriverName: %s Plugin: %s EndPoint: %s NodeID: %s",
+		"DriverName: %s Plugin: %s EndPoint: %s Node Name: %s",


NodeID is used here

it is config.NodeID, which is node name only.

akhilerm · 2021-04-08T14:45:35Z

deploy/yamls/zfs-driver.yaml

@@ -805,6 +805,8 @@ spec:
              value: "zfs-operator"
            - name: OPENEBS_IO_ENABLE_ANALYTICS
              value: "true"
+            - name: NODE_AFFINITY_KEY
+              value: kubernetes.io/hostname


Is this the label key that we are using? It is added to each node by kubernetes right? So how does the labelling a new node work when another node goes down?

it is the default key used by the driver. User has to label and provide that key here. See the docs added.

If the default key is used, PV migration cannot be done right?

akhilerm · 2021-04-08T15:18:39Z

deploy/zfs-operator.yaml

@@ -2204,11 +2206,11 @@ spec:
          image: openebs/zfs-driver:ci
          imagePullPolicy: IfNotPresent
          args:
-            - "--nodeid=$(OPENEBS_NODE_ID)"
+            - "--nodeid=$(OPENEBS_NODE_NAME)"


Is it node name or node ID as the arg?

Yes, better to rename the arg here, will do that. Thanks

akhilerm

given some comments

Here we are setting the node affinity on PV using nodename, so we can not migrate the pods to the new node as k8s scheduler will try to schedule on the old node only. Here, we can now label the nodes with the unique values and provide that key in the operator yaml to use for setting the affinity via NODE_AFFINITY_KEY env. The ZFS LocalPV driver will pick the value from the nodes and set the affinity using the key provided. Now to migrate the PV to the other node, we have to move the disks to the other node and remove the old node from the cluster and set the same label on the new node using the same key, which will let k8s scheduler to schedule the pods to the that node. Signed-off-by: Pawan <pawan@mayadata.io>

Signed-off-by: Pawan <pawan@mayadata.io>

shubham14bajpai

LGTM

docs/faq.md

pkg/zfs/volume.go

akhilerm

@pawanpraka1 Given some comments

Co-authored-by: Akhil Mohan <akhilerm@gmail.com>

docs/faq.md

akhilerm

LGTM

kmova · 2021-04-30T02:34:05Z

Few high level questions.

=> Is this feature tied to a fixed label called "openebs.io/nodeid" ? Or can the user decide a custom label set by their infra tools "coolplatform.io/node-id"?

=> The PR has some changes related to replacing NODEID with NODENAME. Can you update a note in the PR description on why this refactoring was done.

pawanpraka1 · 2021-04-30T05:26:41Z

@kmova

=> Is this feature tied to a fixed label called "openebs.io/nodeid" ? Or can the user decide a custom label set by their infra tools "coolplatform.io/node-id"?

The earlier implementation was doing that only where users have to label the nodes with the custom key and then they have to mention the key the operator yaml. It seems like a lot of work for the users and does not look very user friendly. Later I made the change to fix the key (dce4b0c) so that user does not need to do anything else. We just need to label the nodes with the fixed key.

=> The PR has some changes related to replacing NODEID with NODENAME. Can you update a note in the PR description on why this refactoring was done.

done

kmova · 2021-05-01T13:20:39Z

Had a chat with @pawanpraka1 regarding the customization of the label.

In the current implementation, the driver will automatically set the node labels using node name as a unique identifier. This approach is backward compatible and users don't need to take any action. Going ahead with merging this solution.

This will be followed up by further discussions and design around - making the unique label configurable. We have to decide whether it is a good idea to make this configurable at install or storage class (if this can be done). Based on the feedback on the current implementation we will have to see if we can design a solution that can also be used for migrating volumes to an existing node.

pawanpraka1 added the pr/hold-review hold the review. label Apr 6, 2021

pawanpraka1 added this to the v1.6.0 milestone Apr 6, 2021

pawanpraka1 force-pushed the migration branch 4 times, most recently from 94011c8 to 1f693eb Compare April 6, 2021 17:46

pawanpraka1 removed the pr/hold-review hold the review. label Apr 6, 2021

pawanpraka1 requested review from kmova, prateekpandey14, akhilerm and shubham14bajpai April 6, 2021 17:50

pawanpraka1 force-pushed the migration branch from 1f693eb to d240658 Compare April 6, 2021 17:53

pawanpraka1 added the feature label Apr 6, 2021

pawanpraka1 force-pushed the migration branch 4 times, most recently from 93be005 to d8cd166 Compare April 8, 2021 14:36

akhilerm reviewed Apr 8, 2021

View reviewed changes

pawanpraka1 force-pushed the migration branch from 45cc60d to d72467b Compare April 9, 2021 05:52

pawanpraka1 modified the milestones: v1.6.0, v1.7.0 Apr 19, 2021

pawanpraka1 added 2 commits April 21, 2021 17:07

adding faq doc

725b09a

Signed-off-by: Pawan <pawan@mayadata.io>

renaming the config variable to nodename

d46ae6b

Signed-off-by: Pawan <pawan@mayadata.io>

pawanpraka1 force-pushed the migration branch 3 times, most recently from f59bd97 to a3ec2b3 Compare April 26, 2021 09:59

making migration user friendly

dce4b0c

Signed-off-by: Pawan <pawan@mayadata.io>

pawanpraka1 force-pushed the migration branch from a3ec2b3 to dce4b0c Compare April 26, 2021 10:00

shubham14bajpai approved these changes Apr 28, 2021

View reviewed changes

akhilerm reviewed Apr 28, 2021

View reviewed changes

docs/faq.md Outdated Show resolved Hide resolved

akhilerm reviewed Apr 28, 2021

View reviewed changes

docs/faq.md Show resolved Hide resolved

akhilerm reviewed Apr 28, 2021

View reviewed changes

pkg/zfs/volume.go Show resolved Hide resolved

akhilerm reviewed Apr 28, 2021

View reviewed changes

pawanpraka1 and others added 2 commits April 28, 2021 22:51

Update docs/faq.md

3ea9141

Co-authored-by: Akhil Mohan <akhilerm@gmail.com>

Update docs/faq.md

9c7d9cc

Co-authored-by: Akhil Mohan <akhilerm@gmail.com>

akhilerm reviewed Apr 29, 2021

View reviewed changes

docs/faq.md Outdated Show resolved Hide resolved

Update docs/faq.md

6ae601f

akhilerm approved these changes Apr 29, 2021

View reviewed changes

pawanpraka1 changed the title ~~feat(migration): adding support to migrate the PV to other nodes~~ feat(migration): adding support to migrate the PV to the new node Apr 30, 2021

pawanpraka1 changed the title ~~feat(migration): adding support to migrate the PV to the new node~~ feat(migration): adding support to migrate the PV to a new node Apr 30, 2021

kmova approved these changes May 1, 2021

View reviewed changes

kmova merged commit 1b30116 into openebs:master May 1, 2021

pawanpraka1 deleted the migration branch May 12, 2021 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(migration): adding support to migrate the PV to a new node #304

feat(migration): adding support to migrate the PV to a new node #304

pawanpraka1 commented Apr 6, 2021 •

edited

Loading

akhilerm Apr 8, 2021

pawanpraka1 Apr 8, 2021

akhilerm Apr 8, 2021

pawanpraka1 Apr 8, 2021

akhilerm Apr 8, 2021

pawanpraka1 Apr 8, 2021

akhilerm Apr 8, 2021

pawanpraka1 Apr 8, 2021

akhilerm left a comment

shubham14bajpai left a comment

akhilerm left a comment

akhilerm left a comment

kmova commented Apr 30, 2021 •

edited

Loading

pawanpraka1 commented Apr 30, 2021 •

edited

Loading

kmova commented May 1, 2021

feat(migration): adding support to migrate the PV to a new node #304

feat(migration): adding support to migrate the PV to a new node #304

Conversation

pawanpraka1 commented Apr 6, 2021 • 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

akhilerm left a comment

Choose a reason for hiding this comment

shubham14bajpai left a comment

Choose a reason for hiding this comment

akhilerm left a comment

Choose a reason for hiding this comment

akhilerm left a comment

Choose a reason for hiding this comment

kmova commented Apr 30, 2021 • edited Loading

pawanpraka1 commented Apr 30, 2021 • edited Loading

kmova commented May 1, 2021

pawanpraka1 commented Apr 6, 2021 •

edited

Loading

kmova commented Apr 30, 2021 •

edited

Loading

pawanpraka1 commented Apr 30, 2021 •

edited

Loading