Markus Armbruster wrote:
1. QError feels overengineered, particularly for a first iteration of a
protocol.
We go to great lengths to build highly structured error objects.
There is only one sane reason for doing that: a demonstrated client
need. I can't see that need.
A GUI wants to present the user a mechanism to reduce the memory
consumption of a VM. There are three reasons why this would not work.
The first is that ballooning was not configured for this guest. The
second is that ballooning was configured but the version of the KVM
kernel module does not have a sync mmu. The third is that an
appropriate guest driver has not been loaded for the device.
If we don't specify a type of error, all the GUI can do is fail right
before trying to balloon the guest. That offers the user no insight why
it failed other than popping up a message box with an error message
(that may not even be in their locale). For a non-english speaker, it's
gibberish.
If you use three types of errors, then a GUI can differentiate these
cases and presents the user different feedback. For the first case, it
can simply gray out the menu item. For the second can, it can also gray
out the menu item but it can provide a one time indication (in say, a
status bar), that ballooning is unavailable because the kernel version
does not support it. For the third error, you would want to prompt the
user to indicate the driver is not loaded in the guest.
It's highly likely that for this last case, you'd want generic code to
generate this error. Further more, in order to generate the error
message for a user, you need to know what device does not have a
functioning driver. You may say it's obvious for something like info
balloon but it's not so obvious if you assume we support things other
than just virtio-pci-balloon. For instance, with s390, it's a different
balloon driver. For xenner/xenpv, it's a different driver. It really
suggests we should send the qdev device name for the current balloon driver.
2. It falls short of the requirement that clients can reasonably
classify errors they don't know.
I think this is wishful thinking. I strongly suspect that you can't
find a reasonably popular web browser that doesn't know all of the error
codes that web servers generate.
We should document all of the errors that each QMP function can return.
That said, if you thought there was 4-5 common classes of errors, adding
an additional 'base_class' field could be a completely reasonable thing
to do. As long as you're adding information vs. taking it away, I'm
quite happy.
3. It falls short of the requirement that clients can easily present a
human-readable error description to their human users, regardless of
whether they know the error or not.
That's just incorrect. We provide an example conversion table that's
akin to strerror() or a __repr__ for an exception in Python.
In more detail[*]:
There are more than one way to screw up here. One is certainly to fall
short of client requirements in a way that is costly to fix (think
incompatible protocol revision). Another is to overshoot them in a way
that is costly to maintain. A third one is to spend too much time on
figuring out the perfect solution.
I believe our true problem is that we're still confused and/or
disagreeing on client requirements, and this has led to a design that
tries to cover all the conceivable bases, and feels overengineered to
me.
I've described my requirements for what a client can do. I'd like to
understand how you disagree.
There are only so many complexity credits you can spend in a program,
both globally and individually. I'm very, very wary of making error
reporting more complex than absolutely, desperately necessary.
We're following a very well established model of error reporting
(object-based exceptions). This is well established and well understood.
Reporting errors should remain as easy as we can make it. The more
cumbersome it is to report an error, the less it is done, and the more
vaguely it is done. If you have to edit more than the error site to
report an error accurately, then chances skyrocket that it won't be
reported, or it'll be reported inaccurately.
I don't buy these sort of arguments. We are not designing a library
that is open to abuse. If our developers are fall victim to this, then
we need to retrain them to be less lazy by not accepting patches that do
a poor job reporting errors.
And not because coders are
stupid or lazy, but because they make sensible use of their very limited
complexity credits: if you can either get the job done with lousy error
messages, or not get it done at all, guess what the smart choice is.
Error reporting is extremely important in a management scenario. You
are severely limited by what actions you can take based on the amount of
error information returned. This is particularly when dealing with a
multi-client management mechanism. IMHO, this is a sensible place to
spend complexity credits.
If I understand Dan correctly, machine-readable error code +
human-readable description is just fine, as long as the error code is
reasonably specific and the description is precise and complete. Have
we heard anything else from client developers?
There are no client developers because QMP doesn't exist. Historically,
we haven't provided high quality error messages so of course libvirt
makes due without them today.
But I can answer on behalf of the management work we've done based on
libvirt.
If you attempt to do a virDomainSetMemory() to a qemu domain, you only
get an error if you're using a version of qemu that doesn't support the
balloon command. You do not get an error at all for the case where the
kernel module doesn't support ballooning or a guest driver isn't loaded.
The concrete use case here is building an autoballoon mechanism part of
a larger management suite. If you're in an environment where memory
overcommit is important, then you really do want to know whether
ballooning isn't functioning because appropriate guest drivers aren't
loaded.
I'm not a client developer, but let me make a few propositions on client
needs anyway:
* Clients are just as complexity-bound as the server. They prefer their
errors as simple as possible, but no simpler.
I agree but the problem is that every client has a different definition
of "simple as possible". My client doesn't necessarily care about PCI
hotplug error messages because it's not a function that we use.
However, ballooning is tremendously important and I want to know
everything I can about why it failed.
* The crucial question for the client isn't "what exactly went wrong".
It is "how should I handle this error". Answering that question
should be easy (say, check the error code). Figuring out what went
wrong should still be possible for a human operator of the client.
I disagree. The client needs to know what went wrong if they are going
to take a corrective action.
* Clients don't want to be tightly coupled to the server.
I don't follow this.
* No matter how smart and up-to-date the client is, there will always be
errors it doesn't know. And it needs to answer the "how to handle"
question whether it knows the error code or not! That's why protocols
like HTTP have simple rules to classify error codes.
HTTP is the exception and the HTTP error classes really leave an awful
lot to be desired. If you feel it's important to add error classes, I'd
be happy to consider those patches.
Likewise, it needs to be able to give a human operator enough
information to figure out what went wrong whether it knows the error
or not. How do you expect clients to format a structured error object
for an error they don't know into human-readable text? Isn't it much
easier and more robust to cut out the formatting middle-man and send
the text along with the error?
We do that by providing a conversion table. The fundamental problem
here is localization.
* Clients need to present a human-readable error description to their
human users, regardless of whether they know the error or not.
I would never pass an error string from a third party directly to a
user. I doubt you'll find a lot of good applications that do. From a
usability perspective, you need to be in control of your interactions
with the user. They grammar needs to be consistent and it has to be
localized. The best you would do with a third party string is log it to
some log file for later examination by support. In that case, dumping
the class code and the supporting information in JSON form is just as
good as a text description.
There's a general rule of programming that I've found quite hard to
learn and quite painful to disobey: always try the stupidest solution
that could possibly work first.
Based on what I've learned about client requirements so far, I figure
that solution is "easily classified error code + human-readable
description".
I appreciate you feel strongly that this is the right thing to do. That
said, I disagree and I have specific use cases in mind that are not
satisfied by the above mechanism. In a situation like this, I tend to
think the best thing to do is provide more information such that
everyone can get the function they want.
If we add a base_class member to the error structure and we have the
table for formatting error messages, then that should satisfy your
requirements. If you're concerned about a client having to have an
up-to-date version of the table, we can introduce a monitor command to
pretty print a json error object which would address that concern.
Then I think your argument boils down to, you think the remaining
infrastructure is unnecessary for what you anticipate the uses to be.
But hopefully, you'll at least concede that there are valid use cases
beyond what you anticipate to be the normal case that do need this
information.
If we later realize that this solution was *too* stupid, we can simply
add a data member to the error object.
It's never that easy because a management tool has to support a least
common denominator.
--
Regards,
Anthony Liguori
