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

json implemented in zfs CLI #3938

Closed
wants to merge 3 commits into from
Closed

Conversation

goulvenriou
Copy link

Hi, in response to the pull request #3483 Alyseo's team has refactored the code to deliver single commit including all the jsonify code, the json output has been implements in all CLI commands.
Regards.
Goulven

@behlendorf
Copy link
Contributor

Awesome, thanks for squashing the patch stack it will make it much easier to review.

@ryao
Copy link
Contributor

ryao commented Oct 21, 2015

@goulvenriou @gwilson told me that you have libzfs_core extensions as part of the JSON changes. Where can I find those?

@goulvenriou
Copy link
Author

@ryao , Hi , the libzfs_core isn't affected by the JSON implementation.

@hadees
Copy link

hadees commented Nov 12, 2015

Awesome! I've really been wanting to write a web interface for my box and this will make it way easier. 👍

@yada
Copy link

yada commented Nov 12, 2015

@hadees Was one of the main goal : we think it will facilitate many new community projects based on openzfs ;-) Happy if it helps and please test, review, comment or create bug report if you find any. Thanks for your support.

@hadees
Copy link

hadees commented Dec 8, 2015

@yada is this live yet? How do I use it?

@yada
Copy link

yada commented Dec 8, 2015

@hadees The branch has been updated by our team (it is up to date now) and you check this readme file if you want to build : https://github.com/Alyseo/zfs/blob/json-0.6.5/json/install.md

@fractalram
Copy link

Is this part of the newly release zfs-0.6.5.4?

@yada
Copy link

yada commented Jan 11, 2016

@fractalram Still not, we will push again a new and fresh rebase to help core dev review and hope it will happen soon ;-) @behlendorf may have more info on timeframe ?

@yada
Copy link

yada commented Jan 18, 2016

New rebase done, hope it helps ;-)

@behlendorf behlendorf added the Type: Feature Feature request or new feature label Jan 28, 2016
@yada
Copy link

yada commented Jan 28, 2016

Hi,

We understand it is a pretty disruptive append to the code base but at least it is not IMHO introduce disruptive changes.

We are closed to the final step now, all commands have been implemented using two formats (json and ldjson), schemas descriptions are online (including orderly), we regularly push new rebase to help code review... So now we just need some love :) or in other words, we need to know if you guys (OpenZFS community) agree and get buy on the fundamental approach and give their approval for merging our code ?

For infos (output and format...), see :
https://github.com/Alyseo/zfs/tree/json-0.6.5/json

BTW, we follow and like #3907
We have different approaches but I strongly think that both can be very complementary and can co-exist since they serve slightly different roles. To help code review on these two importants pull requests, maybe we can help each others (code review) ? @ryao let me know what you think about it and if it can helps ?

PS: Also posted to [OpenZFS Developer]

Regards,
Yacine

@goulvenriou
Copy link
Author

new rebase and squash Done , your comments and reviews are welcome . the diff is available here : https://github.com/zfsonlinux/zfs/pull/3938/files
Regards,
Goulven
edited on 02/09/2016

@goulvenriou goulvenriou force-pushed the json-0.6.5 branch 2 times, most recently from 70b96eb to b3c8794 Compare February 9, 2016 16:34
@yada
Copy link

yada commented Feb 11, 2016

The usual tests failed I guess, can you guys confirm we have nothing to fix on our side to improve this : @behlendorf ?

@behlendorf
Copy link
Contributor

@yada all the tests normally pass. It looks like your changes have accidentally introduced at least one change in behavior. It seems filebench is failing because the zfs set compression command is failing, see the CentOS 7 test log as an example.

sudo -E zfs set compression=lz4 tank/filebench
missing property=value argument(s)
usage:
    set [-jJ] <property=value> ... <filesystem|volume|snapshot> ...

The following properties are supported:
...

@yada
Copy link

yada commented Feb 12, 2016

@behlendorf : thanks for the info and issue fixed, all passed now and branch is up to date so happy review ;-)

@goulvenriou goulvenriou force-pushed the json-0.6.5 branch 4 times, most recently from fcb2dd7 to 3fa363c Compare March 11, 2016 14:55
@goulvenriou goulvenriou force-pushed the json-0.6.5 branch 5 times, most recently from 104942e to 70eb170 Compare April 7, 2016 11:03
@goulvenriou goulvenriou force-pushed the json-0.6.5 branch 2 times, most recently from d176b47 to 080f5ee Compare April 15, 2016 13:42
@behlendorf
Copy link
Contributor

@yada how goes the process of adding test cases to verify the json output?

@yada
Copy link

yada commented May 9, 2016

@behlendorf Sorry, team been busy on big project... will find some spare time to work on it asap.

@yada
Copy link

yada commented May 28, 2016

Can you describe and explain your major issue !?
All schema are complete for ages... See :
https://github.com/Alyseo/zfs/blob/json-0.6.5/json/STATUS.md

What is missing from your point of view ?
Will love to understand your latest sentence and why current work done is not useful...

@denji
Copy link

denji commented May 28, 2016

@goulvenriou
Copy link
Author

@denji
the choice of the library was made ​​during exchange on the mailing list zfs before implementation, you can see the archive here : https://www.mail-archive.com/developer@open-zfs.org/msg00880.html ,
we have indeed chosen to use the libnvpair , modified by Joyent

FYI : 
- Long term, we'd like the kernel to do more of the property processing, so this could eventually turn into simply:
     - do an ioctl to get the requested properties of the requested datasets, returning an nvlist
     - print nvlist as json (which has already been implemented, e.g. by Joyent)

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.

Test cases needed.

Copy link

@wspeirs wspeirs left a comment

Choose a reason for hiding this comment

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

I really only looked at the schema files as the plan is to add something to the tests to verify that that JSON output validates against the schema files. However, the errors in the files need to be fixed first... most of them appear to be copy-paste issues.

@yada any thoughts on a good tool to use for validating the JSON output against the schema files? There are a few tools (http://json-schema.org/implementations.html) but mileage varies greatly.

"items":
{
"type":"object",
"type": "type of volume",
Copy link

Choose a reason for hiding this comment

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

type seems to be a duplicate key here, and overrides the schema restriction. Should "type of volume" be a property instead?

"items":
{
"type":"object",
"type": "type of volume",
Copy link

Choose a reason for hiding this comment

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

type seems to be a duplicate key here, and overrides the schema restriction. Should "type of volume" be a property instead?

}
}
}
```
Copy link

Choose a reason for hiding this comment

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

Erroneous markdown

"type":"object",
"name": "zpool split -J",
"version": "1.0",
"description": "create a new pool by splitting a mirrored zfs storage poo"l,
Copy link

Choose a reason for hiding this comment

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

Invalid JSON, need to move the l inside the quotes

"health": {
"type":"string",
"name": "health",
"description": " The current health of the pool. Health can be "ONLINE", "DEGRADED", "FAULTED", " OFF‐LINE", "REMOVED", or "UNAVAIL"",
Copy link

Choose a reason for hiding this comment

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

Cannot have double-quotes in values without escaping them with a backslash

}


```
Copy link

Choose a reason for hiding this comment

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

Erroneous markdown

"items":
{
"type":"object",
"type": "type of volume",
Copy link

Choose a reason for hiding this comment

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

type seems to be a duplicate key here, and overrides the schema restriction. Should "type of volume" be a property instead?

"items":
{
"type":"object",
"type": "type of volume",
Copy link

Choose a reason for hiding this comment

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

type seems to be a duplicate key here, and overrides the schema restriction. Should "type of volume" be a property instead?

@@ -0,0 +1,27 @@
zfs destroy -J :
Copy link

Choose a reason for hiding this comment

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

Erroneous sring

}


```
Copy link

Choose a reason for hiding this comment

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

Erroneous markdown

@wspeirs
Copy link

wspeirs commented Sep 27, 2016

@yada I also believe there are errors in your schema files compared to what is generated. Using zfs list -J on a system with no ZFS volumes yields the following output:

{"cmd":"zfs list","schema_version":"1.0","stdout":[],"stderr":{"error":"no datasets available"}}

However, according to the schema, stderr should be a string:

        "stderr": {
            "type":"string",
            "title": "stderr",
            "description": "error output of command",
            "required":true
        },

I imagine this is broken in a few places...

Also, it appears you're using Draft3 of the JSON Schema, should this be updated to Draft4? One thing I noticed that has changed is the required restriction.

Finally, why did you use Orderly (the orderly-json.org site seems to be down) instead of straight JSON schema? I think the next step to creating tests would be to run commands and verify the output against the schema files, but currently too much is broken for this to work. Happy to help, but I need to better understand the design decisions here.

@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label Oct 7, 2016
@alek-p
Copy link
Contributor

alek-p commented Jun 22, 2017

Looks like this PR has been inactive for a while, @goulvenriou, @yada what's the status on this?
We'd like to see this integrated so I can offer assistance to move this along.

@yada
Copy link

yada commented Jun 29, 2017

@alek-p : Yep sorry guys but new job and no ressources anymore to move forward on this project.
I think this feature will be great for the openzfs community and hope some folks can take over...

@sevagh
Copy link

sevagh commented Feb 13, 2018

Hello. I've had to write some ugly Python to parse the output of zfs status, and searched for JSON output capabilities.

I'm interested in continuing the work on this PR. Should I continue from the last commit on this PR, or is it better to start over?

@behlendorf
Copy link
Contributor

There was work done by ClusterHQ https://github.com/ClusterHQ/pyzfs which is worth looking at.

@alek-p
Copy link
Contributor

alek-p commented Feb 13, 2018

We've made some progress on JSON output support for zfs cmd at datto. What we've done is 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()).
Of course, this only adds the ability to get JSON output for zfs commands, we're still thinking about how to cleanly do this for zpool commands and welcome collaboration on that.

Since channel programmes got merged recently, I've dug out the JSON patch and am rebasing it, plus adding tests. Once that's done it will be put out for review in ZoL.

@yzgyyang
Copy link

We've made some progress on JSON output support for zfs cmd at datto. What we've done is 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()).
Of course, this only adds the ability to get JSON output for zfs commands, we're still thinking about how to cleanly do this for zpool commands and welcome collaboration on that.

Since channel programmes got merged recently, I've dug out the JSON patch and am rebasing it, plus adding tests. Once that's done it will be put out for review in ZoL.

Any news regarding zpool channel programs?

@alek-p
Copy link
Contributor

alek-p commented Jan 27, 2021

We've made some progress on JSON output support for zfs cmd at datto. What we've done is 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()).
Of course, this only adds the ability to get JSON output for zfs commands, we're still thinking about how to cleanly do this for zpool commands and welcome collaboration on that.
Since channel programmes got merged recently, I've dug out the JSON patch and am rebasing it, plus adding tests. Once that's done it will be put out for review in ZoL.

Any news regarding zpool channel programs?

I think you're asking about json output support for channel programs - that was implemented here #7281

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Work in Progress Not yet ready for general review Type: Feature Feature request or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.