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

add JSON output support to channel programs #7281

Merged
merged 1 commit into from Mar 19, 2018

Conversation

alek-p
Copy link
Contributor

@alek-p alek-p commented Mar 7, 2018

This patch adds JSON output support to channel programs.

Description

The changes piggyback JSON output support on top of channel programs (#6558). This way the JSON output support is targeted to scripting use cases and is easily maintainable since it really only touches one function (zfs_do_channel_program()).

This patch ports Joyent's JSON nvlist library from illumos to enable easy JSON printing of channel program output nvlist.
To keep the delta small I also took advantage of the fact that printing in zfs_do_channel_program() was almost always done before exiting the program.

Motivation and Context

There have been requests to add JSON output support for a while, since as far back as 2012. See #3938, #740, and #6967

How Has This Been Tested?

Unit testing along with the newly added test case.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@alek-p
Copy link
Contributor Author

alek-p commented Mar 7, 2018

should probably post some sample output:

# cat zfs_rlist.zcp
           succeeded = {}
           failed = {}

           function list_recursive(root, prop)
               for child in zfs.list.children(root) do
                   list_recursive(child, prop)
               end
               val, src  = zfs.get_prop(root, prop)
               if (val == nil) then
                   failed[root] = val
               else
                   succeeded[root] = val
               end
           end

           args = ...
           argv = args["argv"]

           list_recursive(argv[1], argv[2])

           results = {}
           results["succeeded"] = succeeded
           results["failed"] = failed
           return results
# zfs program data zfs_rlist.zcp data recordsize
Channel program fully executed and produced output:
    return:
        succeeded:
            data/fs: 131072
            data/8k: 8192
            data: 131072
            data/128k: 131072
        failed:
# zfs program -j data zfs_rlist.zcp data recordsize
{"return_value":0,"truncated":false,"stdout":"Channel program fully executed and produced output:\n","output":{"return":{"succeeded":{"data/128k":131072,"data":131072,"data/fs":131072,"data/8k":8192},"failed":{}}}}
# zfs program -j data zfs_rlist.zcp data recordsize | python -m json.tool
{
    "output": {
        "return": {
            "failed": {},
            "succeeded": {
                "data": 131072,
                "data/128k": 131072,
                "data/8k": 8192,
                "data/fs": 131072
            }
        }
    },
    "return_value": 0,
    "stdout": "Channel program fully executed and produced output:\n",
    "truncated": false
}

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved
man/man8/zfs.8 Outdated Show resolved Hide resolved
@ahrens
Copy link
Member

ahrens commented Mar 7, 2018

This is a wonderful idea, and so much simpler than json-ifying every command!

@alek-p
Copy link
Contributor Author

alek-p commented Mar 8, 2018

Thanks for the reviews! I think I've addressed all the feedback in comments or in the code, let me know if there is anything else.
New sample output:

# zfs program data -j zfs_rlist.zcp data recordsize | python -m json.tool
{
    "return": {
        "failed": {},
        "succeeded": {
            "data": 131072,
            "data/128k": 131072,
            "data/8k": 8192,
            "data/fs": 131072
        }
    }
}
# zfs program data -j zfs_rlist.zcp data recordsize
{"return":{"succeeded":{"data/fs":131072,"data/128k":131072,"data/8k":8192,"data":131072},"failed":{}}}

@behlendorf
Copy link
Contributor

@alek-p when it's convenient a rebase on master will sort out the Rawhide/Kernel.org build failures. There were some ABI changes in the 4.16-rc3 kernel.

@behlendorf
Copy link
Contributor

It looks like we had one hiccup in testing on CentOS 6 where the new test case failed. I've resubmitted that build to see how reproducible it is. Here's the link:

http://build.zfsonlinux.org/builders/CentOS%206%20x86_64%20%28TEST%29/builds/2119

@richardelling
Copy link
Contributor

Just one comment. You can believe me now, or believe me later.
JSON numbers are neither ints nor floats. They are numbers. So when you take an int and transform it to a number, you must expect the consumer to convert that to float, because many do. The only way to ensure consumers don't screw themselves, is to force them to understand the data: by writing as a string and forcing them to do string-to-int, string-to-uint, or string-to-float. While this might seem simple enough for many numbers, it is hugely suckful for enums and bitmasks (ints).
My suggestion is to always print strings or booleans. The nice side-effect is you can print hex.

@alek-p
Copy link
Contributor Author

alek-p commented Mar 12, 2018

seems like the test failure has to do with how python on centos 6 outputs json. I'll add some debug print into the test so that I can look into this more

Signed-off-by: Alek Pinchuk <apinchuk@datto.com>
@codecov
Copy link

codecov bot commented Mar 16, 2018

Codecov Report

Merging #7281 into master will decrease coverage by 0.33%.
The diff coverage is 25.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7281      +/-   ##
==========================================
- Coverage   76.62%   76.29%   -0.34%     
==========================================
  Files         328      329       +1     
  Lines      104034   104188     +154     
==========================================
- Hits        79716    79489     -227     
- Misses      24318    24699     +381
Flag Coverage Δ
#kernel 76.03% <ø> (-0.43%) ⬇️
#user 65.49% <25.46%> (-0.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cec3a0a...4790320. Read the comment docs.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Functionally, this looks good to me but I'm no expert on the pitfalls of using JSON formatted output. Since we've had quite a lot of interest in this particular functionality it would be great if a few more people could test and approve this PR before it's merged.

@richardelling regarding you comment on formatting numbers are there specific changes you're looking for?

@richardelling
Copy link
Contributor

I'll be happy to do a -J option that implements numbers as strings, after this gets integrated. The changes are needed in libnvpair_json.c and I understand a desire to import that as-is.

@behlendorf behlendorf merged commit 272b5d7 into openzfs:master Mar 19, 2018
@behlendorf
Copy link
Contributor

Merged to master. Please open new PRs to extend this functionality as needed.

@RLovelett
Copy link

In what version of ZFS does zfs program appear (I'm not even specifically asking about the JSON stuff)? I'm getting unrecognized command 'program' in ZFS: Loaded module v0.7.8-1, ZFS pool version 5000, ZFS filesystem version 5.

@tonyhutter
Copy link
Contributor

@RLovelett it's currently only in master. We haven't pushed it to a point release yet since it's such a big patch. We may push it to a future point release if we're comfortable enough with it. Otherwise, it will be supported in 0.8.0.

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.

None yet

8 participants