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

fix(plugin): Fix ability to have custom value for openebs.io/nodeid #451

Merged
merged 7 commits into from
Dec 20, 2023

Conversation

jnels124
Copy link
Contributor

@jnels124 jnels124 commented Jun 8, 2023

Pull Request template

Please, go through these steps before you submit a PR.

Why is this PR required? What issue does it fix?:
This PR is required in order to allow a ZFS volume to be moved to another node
What this PR does?:
This pr fixes existing logic that attempts to retrieve a node using the name and instead retrieves the node using the nodeid label
Does this PR require any upgrade changes?:
No
If the changes in this PR are manually verified, list down the scenarios covered::
These changes were manually verified by doing the following:

  1. Labeled all nodes via kubectl label node <Node> openebs.io/nodeid=value-1
  2. Created persistent volumes
  3. Ensured that the ZFSNode resources were created correctly.
  4. Performed manual upgrade of the cluster
  5. Attached the label with distinct value to each of the new nodes
  6. Started test application to ensure that the volumes were mounted and accessible

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

  • Fixes Setting NodeId label doesn't work #450
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

@jnels124 jnels124 force-pushed the node-id-fix branch 2 times, most recently from 9ff5475 to c854109 Compare June 8, 2023 15:08
@jnels124 jnels124 marked this pull request as ready for review June 8, 2023 15:09
@jnels124 jnels124 marked this pull request as draft June 8, 2023 19:59
@jnels124 jnels124 marked this pull request as ready for review June 16, 2023 18:49
@jnels124
Copy link
Contributor Author

@niladrih when you have a moment would you mind taking a peak at this PR. It is a critical fix in order for our deployment. Without this change, we will not be able to perform node upgrades within the cluster.

@sinhaashish
Copy link
Member

sinhaashish commented Aug 11, 2023

@jnels124 sorry to keep you waiting, but @niladrih is having some higher priority jobs at the moment and is stuck in them . For sure i will look into this PR. Just give us some more time.

Choose a reason for hiding this comment

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

Should have some tests for the changes to both go classes

@hrudaya21
Copy link
Contributor

hrudaya21 commented Oct 25, 2023

@jnels124 Can you please rebase your code. Also please add the testing steps in the Documentation file as part of this PR.

@jnels124
Copy link
Contributor Author

jnels124 commented Nov 1, 2023

@hrudaya21 I have rebased and updated install documentation to reference existing documentation. Please let me know if anything else is needed to get this merged.

@DeprecatedLuke
Copy link

See #489. Might still be broken.

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
@hrudaya21 hrudaya21 merged commit 6b0942c into openebs:develop Dec 20, 2023
5 checks passed
@jnels124 jnels124 deleted the node-id-fix branch December 28, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting NodeId label doesn't work
5 participants