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

feat(list_snap): zfs listsnap CLI #295

Merged
merged 8 commits into from
Mar 30, 2020
Merged

Conversation

vishnuitta
Copy link

@vishnuitta vishnuitta commented Mar 20, 2020

This PR adds the zfs listsnap <dataset> CLI.
For this command, output will be in json format as:

vitta@vitta-laptop:~/openebs/lzfs$ ./cmd/zfs/zfs listsnap pool1/ds0 | jq
{
  "name": "pool1/ds0",
  "snaplist": {
    "istgt1": null,
    "snap4": null,
    "istgt2": null,
    "snap1": null,
    "snap2": null,
    "istgt3": null,
    "snap3": null
  }
}

This will NOT list the snapshots created through 'zfs snapshot' until the zrepl is restarted. However, it lists the snapshots created through istgt iscsi target.

Even though existing zfs list -t snapshot gives the list of snapshots, difference with this CLI is just NOT about printing in json format. Main difference is in caching the response. This command implementation will NOT read the contents from disk for every trigger of the command. It does read from disk one time, and keeps updating it with the new snapshots name whenever istgt sends SNAP_CREATE command.

Working on testcases.

Signed-off-by: Vitta vitta@mayadata.io

Signed-off-by: Vitta <vitta@mayadata.io>
Copy link

@richardelling richardelling left a comment

Choose a reason for hiding this comment

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

It isn't clear to me why we're adding a new command, when zfs list -t snapshot already exists

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
pawanpraka1
pawanpraka1 previously approved these changes Mar 24, 2020
Copy link
Member

@pawanpraka1 pawanpraka1 left a comment

Choose a reason for hiding this comment

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

looks good. one minor comment.

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
Copy link
Member

@mynktl mynktl left a comment

Choose a reason for hiding this comment

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

looks good.

module/zfs/zvol.c Outdated Show resolved Hide resolved
Signed-off-by: Vitta <vitta@mayadata.io>
@vishnuitta
Copy link
Author

incorporated review comments

Signed-off-by: Vitta <vitta@mayadata.io>
Signed-off-by: Vitta <vitta@mayadata.io>
Signed-off-by: Vitta <vitta@mayadata.io>
Signed-off-by: Vitta <vitta@mayadata.io>
Signed-off-by: Vitta <vitta@mayadata.io>
@pawanpraka1
Copy link
Member

@vishnuitta can you update the PR description with the modified output?

@vishnuitta
Copy link
Author

@vishnuitta can you update the PR description with the modified output?

PR description already have modified output

@vishnuitta
Copy link
Author

It isn't clear to me why we're adding a new command, when zfs list -t snapshot already exists

This command caches the response. As the control plane triggers the request very frequently (say every 30 seconds), reading from disk doesn't make sense. So, this command is introduced to cache the response and keep the in-memory data up-to-date.

@pawanpraka1
Copy link
Member

@vishnuitta can you update the PR description with the modified output?

PR description already have modified output

I see in the PR you have change the o/p to fprintf(stdout, "%s\n", str_val); from fprintf(stdout, "%s: %s\n", nvpair_name(elem),str_val)?

@vishnuitta
Copy link
Author

@vishnuitta can you update the PR description with the modified output?

PR description already have modified output

I see in the PR you have change the o/p to fprintf(stdout, "%s\n", str_val); from fprintf(stdout, "%s: %s\n", nvpair_name(elem),str_val)?

yes.. I updated the output after changing this print in the code

mittachaitu
mittachaitu previously approved these changes Mar 30, 2020
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.

changes are good

tests/cstor/gtest/test_zrepl_prot.cc Show resolved Hide resolved
mynktl
mynktl previously approved these changes Mar 30, 2020
tests/cstor/gtest/test_zrepl_prot.cc Outdated Show resolved Hide resolved
@vishnuitta vishnuitta dismissed stale reviews from mynktl and mittachaitu via be6a9bf March 30, 2020 11:42
Signed-off-by: Vitta <vitta@mayadata.io>
@mynktl mynktl merged commit 850861a into mayadata-io:develop Mar 30, 2020
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.

5 participants