Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(design): adding pv migration proposal #336

Merged
merged 7 commits into from
Feb 20, 2024

Conversation

pawanpraka1
Copy link
Contributor

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

@pawanpraka1 pawanpraka1 added the documentation Improvements or additions to documentation label May 20, 2021
Signed-off-by: Pawan <pawan@mayadata.io>
design/pv-migration.md Outdated Show resolved Hide resolved
design/pv-migration.md Outdated Show resolved Hide resolved
design/pv-migration.md Outdated Show resolved Hide resolved
Signed-off-by: Pawan <pawan@mayadata.io>
Signed-off-by: Pawan <pawan@mayadata.io>
@pawanpraka1
Copy link
Contributor Author

cc: @iyashu Please take a look.

Copy link

@mittachaitu mittachaitu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

design/pv-migration.md Outdated Show resolved Hide resolved
Signed-off-by: Pawan <pawan@mayadata.io>
design/pv-migration.md Outdated Show resolved Hide resolved
Copy link
Member

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pawanpraka1 Have a few questions on the design

Copy link
Member

@prateekpandey14 prateekpandey14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, although one small query on pod reschedule.

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

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

### Workflow

- user will setup all the nodes and setup the ZFS pool on each of those nodes.
- the ZFS-LocalPV CSI driver will look for all the pools on the node and will set the `guid.openebs.io/<pool-guid>=true` label for all ZFS POOLs that is present on that node. Let's say node-1 has two pools(say pool1 with guid as 14820954593456176137 and pool2 with guid as 16291571091328403547) present then the labels will be like this :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q_: Does k8s put a limit on the number of labels that can be attached to the node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are cloud providers which have some limit like gcp has limit of 64.


- we can simply import the pool and restart the ZFS-LocalPV driver to make it aware of that pool to set the corresponding node topology
- the ZFS-LocalPV driver will look for `guid.openebs.io/14820954593456176137=true` and will remove the label from the nodes where pool is not present
- the ZFS-LocalPV driver will update the new node with `guid.openebs.io/14820954593456176137=true` label
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q_: How to avoid a race conditions where two nodes in the cluster will have the same label? Also consider the cases like:

  • Node is shutdown or in not ready state and the pools have been moved to new node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous step handles that, it has to make sure there are no node with the same label.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the previous steps - ZFS - Local PV driver - is it the node driver running on the old node or new node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the node driver will remove/set the label.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in uninstall, cases - will the labels be cleared from nodes.

Copy link
Contributor Author

@pawanpraka1 pawanpraka1 Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need that, at the installation time, node label will be handle/moved accordingly. Btw CSI does not provide any framework to unset the topology. We can only set it at the registration time only. But can be done out of the box.

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

Is there still something missing from this proposal, which blocks it from being merged?

Else it could be a first step towards an implementation of #291. Merging it would signal that this feature is still considered, while the discussion is stalled, that could be continued in pair with a reference implementation.

@sinhaashish
Copy link
Member

@pawanpraka1 It would be great if you contribute this feature(code) to this repo. We are our hands full of other openebs engines work at the moment, and i don't see we will be able to code for this improvement.
Please contribute.

@orville-wright orville-wright merged commit b236788 into openebs:develop Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Development

Successfully merging this pull request may close these issues.

8 participants