Re: [Qemu-devel] [PATCH 00/10]: QError v4

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> 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.

So far, this is a convincing argument for reasonably specific error
codes, and on that we're in violent agreement.  What you didn't
demonstrate here is a need for *structured* error reporting,
i.e. machine-readable data beyond a simple error code.

> 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.

This is your argument for structured error reporting.  It boils down to
client localization.

I challenge the idea that we should even try to get that covered now.
We need to get the basics right, and the only way to do that is to use
them in anger.  The more baggage we design into the thing before we use
it, the higher the risk that we screw it up.

With our JSON encoding we can easily extend the error object without
breaking existing users.  It's designed for growth.  One more reason not
to try to anticipate everything.

Iteration beats the committee.  Let's iterate.

>> 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.

I'd argue that that wasn't the case while HTTP was still evolving, and I
bet it won't be the case while QMP is evolving.

> We should document all of the errors that each QMP function can return.

Yes, we should.

> 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.

Fair enough.

HTTP and FTP encode the error class into the error code.  Clever, but
since we decided to use textual error codes, a separate textual error
class probably makes the most sense.

>> 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.

As Jamie already said, that won't work unless we transfer the table over
the wire, or require client to match server perfectly.  I doubt you want
the latter.  Did you mean to suggest the former?

>> 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.

I have essentially two complaints:

* I'm afraid we've made QError much more complex than it need be (item 1
  above), at least for a first iteration of the protocol.

* The design has shortcomings that effectively require clients to know
  all errors (items 2 & 3 above).

>> 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.

It's a well-established, well-understood mechanism within a single
program, where coupling between the parts is checked and supported by
the programming language(s).

But we're designing a (hopefully!) simple networking protocol.  Got
precedence for object-based exceptions in simple networking protocols?
Or are we breaking new ground here?

>> 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.

I'm afraid I'm not as sanguine as you on the topic of patch review.

>>  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.

I don't disagree with this as a general sentiment, I do disagree with
the idea that we need to spend them *now*, before we even tried to use
the thing in a management scenario.

>> 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.

Again, this is a convincing argument for reasonably specific error
codes, not for highly structured errors.

>> 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.

For errors the client doesn't know, it can't possibly answer "what
exactly went wrong".  It still has to answer the crucial question "how
should I handle this error".

For errors the client knows, I'd expect a (reasonably specific!) error
code to be sufficient information, except perhaps for a few particularly
interesting cases.  Your ballooning example isn't such a case as far as
I can see.

>> * Clients don't want to be tightly coupled to the server.
>>   
>
> I don't follow this.

We want to be able to use an old client with a new server and vice
versa.

>> * 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.

How should the application report an error it doesn't know to the user?

>> 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.

At least in the first iteration.

> 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.

I don't like the extra round-trip to pretty-print the error much (what
if my connection fails just then?), but I figure I could live with that.

> 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.

I gladly concede that I can't anticipate all uses.  That's my point,
actually.  I don't trust my anticipating, I want uses demonstrated
before building complex solutions for them.

>> 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.

If we build complex solutions for anticipated needs, we run a high risk
of missing real needs.  And then we'll evolve the protocol "sideways",
which is even less easy for management tools than evolving "upwards".

We'll iterate anyway, so why not embrace it?  Start with something
simple and functional, then extend it to address what's found lacking.