zpool_influxdb: rewire under zpool command

[snajpa]

The original zpool_influxdb name has polluted the usual command namespace
available via common $PATH settings. This is really inconvenient for users
who have things like zpo<tab>status in muscle memory.

This change rewires the logic of said command under traditional zpool command,
which already links with libzfs (and nothing else) + it doesn't do any calls to
libzfs automatically.

Signed-off-by: Pavel Snajdr snajpa@snajpa.net

Motivation and Context

See #11156

How Has This Been Tested?

Locally, modified test is also passing.

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 OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[snajpa]

Ah, the failing test is because the new help line exceeds 80 chars, going to fix that in a bit.

[richardelling]

I think this project misses the point of why zpool_influxdb exists. It also violates the UNIX philosophy https://en.wikipedia.org/wiki/Unix_philosophy
The zpool command has far too many options and its output is clearly targeted at humans, not automation. Metrics systems are core parts of automation and don't need or want human-friendly formatted outputs.

Finally, zpool_influxdb nominally runs as a daemon. The zpool command is expressly intended to run interactively.

The follow-on to zpool_influxdb is zpool_prometheus that runs as a daemon and provides an HTTP endpoint. This will be an even further divergence from the zpool command and keeping it as a daemon-only is a simpler path while following the UNIX philosophy.

These are the reasons I did not provide a "influxdb-compatible" output extension for the zpool command.

History lesson: several attempts have been made to generate easily parsable (JSON or similar) output for the zpool command. This is non-trivial because of the way the pool configuration is used internally in ZFS. I suppose if we had a fresh start, knowing what we know now, we'd use less C structs and more nvlists because the latter are easy to print as JSON. But going back and retrofitting the current ZFS to do so would be a massive undertaking, with little benefit. The simpler approach is to use /dev/zfs as the interface and provide small, purpose-built applications to use that interface.

So the problem to be solved here, according to the OP, is one of fixing the shell completion rules.

[snajpa]

Well and I think that this argumentation for having a separate command (in a global command namespace no less) is rather weak, to say the least.

If we're to follow the Unix philosophy, then I would love to see the following argumentation:

1. why we already haven't split the subcommands in zfs/zpool, as the Unix philosophy states 'do one thing and do it well'
2. why is this even in the OpenZFS repository in the first place, if we followed the spirit of above argumentation, this should be in a separate repository. If I'm not interested in this functionality at all, why am I forced to have it installed by make install in the OpenZFS repo directly?
3. do you have any numbers proving the advantage of a separate binary? Anything at all, rather than just your general feeling?
4. do you have any numbers why the Prometheus code should be merged in the similar spirit? Why is that even needed with all the community code floating around? (I believe node-exporter is just OK and have no problem with using it).
5. I'd love to know why you choose to disregard long term standing traditions in the code and now are at arms when someone challenges your doing...

And finally, I would love to see the shell completion rule, which will enable this exact sequence again: zpo<tab>status, I'm really genuinely interested in how to do that, when there's another binary called zpool_influxdb. I could see how that woud work if all the zpool commands were to be split in separate binaries, then all of the commands would have a common zpool_ prefix. But how do I autocomplete a space in this case?

Can I ask you to actually argue to the point and with concrete arguments? General philosophical comments don't seem to do much to help me understand a single bit of the motivation where are you actually coming from.

Also, is it too much to ask for a constructive approach? I don't see you trying at all. Trying would be at least something along the lines of "okay, then lets rename it so it doesn't begin with zpool_"... but look at your response. So many words just to disagree and make sure that I know you disagree.

[richardelling]

I'm sure there is an equitable solution. Some comments,

why we already haven't split the subcommands in zfs/zpool, as the Unix philosophy states 'do one thing and do it well'

This makes sense because zfs operates on datasets and zpool operates on pools. One of the original goals was to have a simple set of commands for humans. A ton of features has since been added, so one could argue, like the "ls has too many options", these have become too complicated.

why is this even in the OpenZFS repository in the first place, if we followed the spirit of above argumentation, this should be in a separate repository. If I'm not interested in this functionality at all, why am I forced to have it installed by make install in the OpenZFS repo directly?

Ah, for that there is a very good reason. Such a tool cannot be separated from the OpenZFS source because it relies on internals that are not externally available in the header files. It also uses unstable interfaces (libzfs) that are subject to change with every release -- and they do. So it is not possible to deliver such a tool separately from OpenZFS and have it actually work. Believe me, I tried that first. Corollary, the zfs and zpool commands compiled for one release are not expected to work on any other release.

do you have any numbers proving the advantage of a separate binary? Anything at all, rather than just your general feeling?

Why do we have editors like vim, when all we need is cat? :-)

do you have any numbers why the Prometheus code should be merged in the similar spirit? Why is that even needed with all the community code floating around? (I believe node-exporter is just OK and have no problem with using it).

node_exporter (to which I also contribute) does not deliver zpool information, with a minor exception of a small subset on FreeBSD.

You'll be happy to know that to date, the metrics information available from datasets is obtainable from node_exporter and telegraf (et.al.), so there is not plan AFAIK for a zfs_influxdb.

I'd love to know why you choose to disregard long term standing traditions in the code and now are at arms when someone challenges your doing...

Where are these traditions documented? I see nothing in the contributing docs. Perhaps you can document them and add them?

Meanwhile, back to the real problem. It seems your completion does not permit anyone to ever develop another command that begins with the letters "zpo" Am I correct? Is it really a simple task of renaming "zpool_influxdb" to something else?

[snajpa]

Is it really a simple task of renaming "zpool_influxdb" to something else?

At the absolute core, yes. If we can agree to, say, zmonitor_*, I'll gladly shut up about the rest of the points 😜

It's not that there can't ever be another new binary with zpo prefix, we could however avoid doing that directly in OpenZFS repo. Would you agree?

[richardelling]

This is silly, but surely it is ok to rename. However, monitor is not quite right, I'd recommend zmetrics_influxdb

Also, this implies you'll watch pull requests like a hawk in the future to make sure such things are caught early in review.

[snajpa]

This is silly, but surely it is ok to rename. However, monitor is not quite right, I'd recommend zmetrics_influxdb

Well... I thought the same thing about the current name and I'd bet if this got onto production systems, others would too. IMHO it shows, that we really need to get more people onto the -rc train, we need more testers :)

The other idea I've had is, that we could install this into /usr/libexec, which is not included in $PATH on any systems that I know of - that dir is supposed to be for programs, which are called by other programs, which would fit here nicely, I think. Then we wouldn't have to rename anything ;)

Also, this implies you'll watch pull requests like a hawk in the future to make sure such things are caught early in review.

I'm trying! But... I also have zillion of other duties... OpenZFS is just one of many components we need to look after to put together our vpsAdminOS - and it's still a community effort of 2.5 guys, so my time budget dedicatable to OpenZFS is very limited :(

[richardelling]

relocating to /usr/libexec has merit and would be the easiest solution

[behlendorf]

Relocating it makes good sense to me, arguably that's where it always should have been installed. We already populate a /usr/libexec/zfs/ directory for various zed and zpool support scripts, take a look at the cmd/zpool/Makefile.am for an example of this.

[behlendorf]

It sounds like we've decided the way to handle this is to move it to /usr/libexec/. Given that, I'm going to close out this PR.