fix(plugin): get owner node id from created resource #549

jnels124 · 2024-06-25T01:15:30Z

Pull Request template

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

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

Allows restoring from snapshot when nodeid differs from the node name

What this PR does?:

gets the owner node id from the created resource in all cases

Does this PR require any upgrade changes?:
No

If the changes in this PR are manually verified, list down the scenarios covered::

Deployed with zfs nodes and custom nodeid
Ingested data
Created a snapshot from disks
Restored from snapshots
verified data present

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

Checklist:

Fixes Volume From Snapshot Fails With Custom Node Ids #541
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/website is used to track them:

PLEASE REMOVE BELOW INFORMATION BEFORE SUBMITTING

The PR title message must follow convention:
<type>(<scope>): <subject>.

Where:

type is defining if release will be triggering after merging submitted changes, details in CONTRIBUTING.md.
Most common types are:
- feat - for new features, not a new feature for build script
- fix - for bug fixes or improvements, not a fix for build script
- chore - changes not related to production code
- docs - changes related to documentation
- style - formatting, missing semi colons, linting fix etc; no significant production code changes
- test - adding missing tests, refactoring tests; no production code change
- refactor - refactoring production code, eg. renaming a variable or function name, there should not be any significant production code changes
scope is a single word that best describes where the changes fit.
Most common scopes are like:
- data engine (localpv, jiva, cstor)
- feature (provisioning, backup, restore, exporter)
- code component (api, webhook, cast, upgrade)
- test (tests, bdd)
- chores (version, build, log, travis)
subject is a single line brief description of the changes made in the pull request.

codecov-commenter · 2024-06-25T01:36:37Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.37%. Comparing base (c389127) to head (d397a29).
Report is 2 commits behind head on develop.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #549   +/-   ##
========================================
  Coverage    96.37%   96.37%           
========================================
  Files            1        1           
  Lines          496      496           
========================================
  Hits           478      478           
  Misses          14       14           
  Partials         4        4

Flag	Coverage Δ
bddtests	`96.37% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

niladrih · 2024-06-25T05:38:01Z

@jnels124 -- do you think you could help me with a test case for this nodeId changes that we've added recently?

jnels124 · 2024-06-25T14:56:27Z

@jnels124 -- do you think you could help me with a test case for this nodeId changes that we've added recently?

Yes absolutely.

niladrih · 2024-06-26T05:24:00Z

@jnels124 -- Thank you! We use the ginkgo test framework. The tests sit inside the 'tests' directory. They run on a single-node minikube ('none' driver) kubernetes cluster in our CI. Let me know if you're blocked with any of this. Again, thank you!

jnels124 · 2024-07-09T19:05:59Z

@niladrih Please review this approach. This of course doubles the test execution time but the test
cases present make sense to run with this configuration. Note, the existing tests don't verify much in many cases. Will create a follow up ticket to address that separately.

Abhinandan-Purkait · 2024-07-15T14:20:24Z

@niladrih Please review this approach. This of course doubles the test execution time but the test cases present make sense to run with this configuration. Note, the existing tests don't verify much in many cases. Will create a follow up ticket to address that separately.

cc @niladrih

Abhinandan-Purkait

The code changes look good to me. However running the ci tests two times looks a bit of an overkill to me, maybe this should be an e2e use case rather than a ci test. Doubling ci affects the PR, PR merge... all workflows.

niladrih

Thank you for the work that you've put in here @jnels124! I'm inclined to say that we should both of the sets of tests in parallel. However, the tests are taking 30 min, which isn't too bad and it is testing the core features. I'm okay with this tbh. LGTM

tiagolobocastro · 2024-07-16T09:03:13Z

The code changes look good to me. However running the ci tests two times looks a bit of an overkill to me, maybe this should be an e2e use case rather than a ci test. Doubling ci affects the PR, PR merge... all workflows.

Yeah I'd also suggest running a smaller number of tests with custom node id, at least on PR being raised. Maybe we can have extended tests running at a separate cadence.

niladrih

Looking at this again... this approach might scale badly as newer tests are added, as they'd be run twice.

It makes sense to run a sanity test set with volume provision/de-provision and snapshot creation/deletion as a compromise.

It might be a good idea to add a copy of the test set at https://github.com/openebs/zfs-localpv/blob/develop/tests/provision_test.go in a new file. The prepareCustomNodeIdEnv would be in code, and it would be run as the very first By (before By("####### Creating the storage class : " + fstype + " #######") or By("Creating default storage class", createStorageClass)) in fsVolCreationTest() and blockVolCreationTest() functions. This way what changes is, if there are new tests added they are not run twice, and nodeId is tested for the big workflows. WDYT about this approach @jnels124?

jnels124 · 2024-07-22T21:15:19Z

Looking at this again... this approach might scale badly as newer tests are added, as they'd be run twice.

It makes sense to run a sanity test set with volume provision/de-provision and snapshot creation/deletion as a compromise.

It might be a good idea to add a copy of the test set at https://github.com/openebs/zfs-localpv/blob/develop/tests/provision_test.go in a new file. The prepareCustomNodeIdEnv would be in code, and it would be run as the very first By (before By("####### Creating the storage class : " + fstype + " #######") or By("Creating default storage class", createStorageClass)) in fsVolCreationTest() and blockVolCreationTest() functions. This way what changes is, if there are new tests added they are not run twice, and nodeId is tested for the big workflows. WDYT about this approach @jnels124?

That makes sense to me I will get that in. Will let you know if I encounter difficulties around the environment config. I should be able to update node labels and delete pods to configure the environment appropriately. Will also need to ensure the test resets the environment back to the original state. This also means, this test can not be ran in parallel with other tests.

jnels124 · 2024-07-23T23:20:50Z

@niladrih I took a slightly different approach but accomplishes the goal of running only specific tests after the environment has been configured with a custom node id. I went with this approach because there is not an existing client (and other things) in the test set up to allow modification of k8s nodes. Also, this approach allows more flexibility for future testing

Abhinandan-Purkait · 2024-07-30T05:36:10Z

@jnels124 Can you please rebase the PR branch, a lot of changes went in on the tests and ci. I see we are bumping go version as well, just wanted to double check with @niladrih @tiagolobocastro @avishnu about this.

niladrih

Oh, I missed the go version bump. The go bump needs to go into this Dockerfile as well https://github.com/openebs/zfs-localpv/blob/develop/buildscripts/zfs-driver/zfs-driver.Dockerfile.

go.mod

.github/workflows/pull_request.yml

niladrih

LGTM, thanks.

niladrih · 2024-08-01T06:53:14Z

@jnels124 thank you for making the change! You'd have to rebase your PR branch. I see commits from @sinhaashish and @Abhinandan-Purkait on your branch. This is likely because of a merge, instead of a rebase. I think there might be a git rebase option --rebase-merges.

What we're trying to do is to get your commits to sit on top. Let me know if you need help with this.

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>

Signed-off-by: Jesse Nelson <jesse@hashgraph.com>

jnels124 · 2024-08-03T22:01:02Z

@niladrih I have rebased the commits and pr should only contain my feature commits. thanks

Signed-off-by: Jesse Nelson <jesse@hashgraph.com>

Abhinandan-Purkait · 2024-08-06T09:37:46Z

@jnels124 Thanks, are we good to merge now?

jnels124 · 2024-08-06T20:20:41Z

@Abhinandan-Purkait yes everything is complete and good to merge

Abhinandan-Purkait · 2024-08-07T09:06:00Z

@jnels124 Thanks. You can test the changes using the 2.7.0-develop helm chart, till the next release is cut.

jnels124 changed the title ~~fix(plugin): get owner node id consistently~~ fix(plugin): get owner node id from created resource Jun 25, 2024

sinhaashish requested review from sinhaashish and niladrih June 25, 2024 05:57

Abhinandan-Purkait reviewed Jul 15, 2024

View reviewed changes

niladrih approved these changes Jul 16, 2024

View reviewed changes

niladrih reviewed Jul 16, 2024

View reviewed changes

jnels124 requested review from niladrih and Abhinandan-Purkait July 29, 2024 16:23

niladrih reviewed Jul 30, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

niladrih reviewed Jul 30, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Abhinandan-Purkait reviewed Jul 31, 2024

View reviewed changes

.github/workflows/pull_request.yml Show resolved Hide resolved

Abhinandan-Purkait approved these changes Jul 31, 2024

View reviewed changes

Abhinandan-Purkait requested review from niladrih and tiagolobocastro July 31, 2024 16:03

niladrih approved these changes Aug 1, 2024

View reviewed changes

jnels124 added 4 commits August 3, 2024 16:36

fix(plugin): get owner node id consistently

83bc69a

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>

fix(plugin): add tests with custom nodeid

7108b90

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>

fix(plugin): bump ginkgo to v2

b8331e9

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>

fix(plugin): update namespace

253fcc5

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>

jnels124 and others added 4 commits August 3, 2024 16:43

fix(plugin): update app label

aa6a951

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>

fix(plugin): bump ci go version

a22ae9a

Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>

fix(plugin): update dependencies

83eef56

Signed-off-by: Jesse Nelson <jesse@hashgraph.com>

fix(plugin): update go version in all workflows

7a4ee0b

Signed-off-by: Jesse Nelson <jesse@hashgraph.com>

jnels124 force-pushed the 541-snapshotfix branch from c45f610 to 7a4ee0b Compare August 3, 2024 22:00

fix(plugin): correct change log location

d397a29

Signed-off-by: Jesse Nelson <jesse@hashgraph.com>

Abhinandan-Purkait merged commit fc826e1 into openebs:develop Aug 7, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(plugin): get owner node id from created resource #549

fix(plugin): get owner node id from created resource #549

jnels124 commented Jun 25, 2024 •

edited

Loading

codecov-commenter commented Jun 25, 2024 •

edited

Loading

niladrih commented Jun 25, 2024

jnels124 commented Jun 25, 2024

niladrih commented Jun 26, 2024 •

edited

Loading

jnels124 commented Jul 9, 2024

Abhinandan-Purkait commented Jul 15, 2024

Abhinandan-Purkait left a comment

niladrih left a comment

tiagolobocastro commented Jul 16, 2024

niladrih left a comment •

edited

Loading

jnels124 commented Jul 22, 2024 •

edited

Loading

jnels124 commented Jul 23, 2024 •

edited

Loading

Abhinandan-Purkait commented Jul 30, 2024 •

edited

Loading

niladrih left a comment

niladrih left a comment •

edited

Loading

niladrih commented Aug 1, 2024 •

edited

Loading

jnels124 commented Aug 3, 2024

Abhinandan-Purkait commented Aug 6, 2024

jnels124 commented Aug 6, 2024

Abhinandan-Purkait commented Aug 7, 2024

fix(plugin): get owner node id from created resource #549

fix(plugin): get owner node id from created resource #549

Conversation

jnels124 commented Jun 25, 2024 • edited Loading

Pull Request template

codecov-commenter commented Jun 25, 2024 • edited Loading

Codecov Report

niladrih commented Jun 25, 2024

jnels124 commented Jun 25, 2024

niladrih commented Jun 26, 2024 • edited Loading

jnels124 commented Jul 9, 2024

Abhinandan-Purkait commented Jul 15, 2024

Abhinandan-Purkait left a comment

Choose a reason for hiding this comment

niladrih left a comment

Choose a reason for hiding this comment

tiagolobocastro commented Jul 16, 2024

niladrih left a comment • edited Loading

Choose a reason for hiding this comment

jnels124 commented Jul 22, 2024 • edited Loading

jnels124 commented Jul 23, 2024 • edited Loading

Abhinandan-Purkait commented Jul 30, 2024 • edited Loading

niladrih left a comment

Choose a reason for hiding this comment

niladrih left a comment • edited Loading

Choose a reason for hiding this comment

niladrih commented Aug 1, 2024 • edited Loading

jnels124 commented Aug 3, 2024

Abhinandan-Purkait commented Aug 6, 2024

jnels124 commented Aug 6, 2024

Abhinandan-Purkait commented Aug 7, 2024

jnels124 commented Jun 25, 2024 •

edited

Loading

codecov-commenter commented Jun 25, 2024 •

edited

Loading

niladrih commented Jun 26, 2024 •

edited

Loading

niladrih left a comment •

edited

Loading

jnels124 commented Jul 22, 2024 •

edited

Loading

jnels124 commented Jul 23, 2024 •

edited

Loading

Abhinandan-Purkait commented Jul 30, 2024 •

edited

Loading

niladrih left a comment •

edited

Loading

niladrih commented Aug 1, 2024 •

edited

Loading