[ovirt-devel] Making event types backwards compatible?
Oved Ourfali
oourfali at redhat.com
Tue May 30 10:11:40 UTC 2017
On May 30, 2017 11:28, "Juan Hernández" <jhernand at redhat.com> wrote:
On 05/30/2017 09:38 AM, Tomas Jelinek wrote:
>
>
> On Tue, May 30, 2017 at 9:16 AM, Juan Hernández <jhernand at redhat.com
> <mailto:jhernand at redhat.com>> wrote:
>
> On 05/30/2017 08:55 AM, Tomas Jelinek wrote:
> >
> >
> > On Tue, May 30, 2017 at 7:20 AM, Michal Skrivanek <
mskrivan at redhat.com <mailto:mskrivan at redhat.com>
> > <mailto:mskrivan at redhat.com <mailto:mskrivan at redhat.com>>> wrote:
> >
> > > On 29 May 2017, at 11:44, Juan Hernández <jhernand at redhat.com
<mailto:jhernand at redhat.com>
> <mailto:jhernand at redhat.com <mailto:jhernand at redhat.com>>> wrote:
> > >
> > >> On 05/29/2017 11:27 AM, Michal Skrivanek wrote:
> > >>
> > >>> On 29 May 2017, at 10:39, Juan Hernández <
jhernand at redhat.com <mailto:jhernand at redhat.com>
> > <mailto:jhernand at redhat.com <mailto:jhernand at redhat.com>>>
wrote:
> > >>>
> > >>> Hello,
> > >>>
> > >>> It has been recently requested that the API provides event
> types:
> > >>>
> > >>> [RFE] Expose event types to API
> > >>> https://bugzilla.redhat.com/1453170
> <https://bugzilla.redhat.com/1453170>
> > <https://bugzilla.redhat.com/1453170
> <https://bugzilla.redhat.com/1453170>>
> > >>>
> > >>> Currently the API provides the event code and description,
for
> > example:
> > >>>
> > >>> <event href="/ovirt-engine/api/events/8021" id="8021">
> > >>> <code>19</code>
> > >>> <description>Host myhost failed to recover.</description
> > >>> ...
> > >>> </event>
> > >>>
> > >>> There is no documentation of what is the meaning of codes,
> > except the
> > >>> source code of the engine itself. This forces some
> applications
> > to add
> > >>> their own code to name mapping. For example, the 'ovirt'
Ruby
> > gem used
> > >>> by older versions of ManageIQ to interact with oVirt
contains
> > the following:
> > >>>
> > >>>
> >
> https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/
event.rb#L25-L485
> <https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/
event.rb#L25-L485>
> >
> <https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/
event.rb#L25-L485
> <https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/
event.rb#L25-L485>>
> > >>>
> > >>> We could avoid this by adding to the API a new event
> attribute that
> > >>> indicates the type:
> > >>>
> > >>> <event href="/ovirt-engine/api/events/8021" id="8021">
> > >>> <code>19</code>
> > >>> <type>host_recover_failure</type>
> > >>> <description>Host myhost failed to recover.</description>
> > >>> ...
> > >>> </event>
> > >>>
> > >>> Ideally this should be defined as an enum, so that it will
be
> > >>> represented as an enum in the SDKs. Alternatively it could
> just
> > be an
> > >>> string, and we could reuse the 'name' attribute:
> > >>>
> > >>> <event href="/ovirt-engine/api/events/8021" id="8021">
> > >>> <code>19</code>
> > >>> <name>host_recover_failure</name>
> > >>> <description>Host myhost failed to recover.</description>
> > >>> ...
> > >>> </event>
> > >>>
> > >>> However, the key point to making this useful would be to
keep
> > the types
> > >>> (or names) backwards compatible, so that users of the API
can
> > rely on
> > >>> their values and meanings.
> > >>>
> > >>> So this is my question to you: can we commit to keep the
> names and
> > >>> meanings of the backend event types backwards compatible?
> > >>
> > >> Do we even have to make it bw compatible?
> > >> I guess it depends on the actual usage of those names…
> > >> The ovirt ruby gem itself doesn’t do much with it
> > >
> > > We need to make keep it backwards compatible or else tell
> users "don't
> > > rely on these values, as they may change without notice".
> > >
> > > The 'ovirt' gem doesn't do anything special, it just creates
> its own
> > > code to name mapping. But the users of the 'ovirt' gem (the
> ManageIQ
> > > oVirt provider) do rely on the name. For example:
> > >
> > >
> > >
> https://github.com/ManageIQ/manageiq-providers-ovirt/blob/
master/app/models/manageiq/providers/redhat/infra_
manager/event_parser.rb#L80-L92
> <https://github.com/ManageIQ/manageiq-providers-ovirt/blob/
master/app/models/manageiq/providers/redhat/infra_
manager/event_parser.rb#L80-L92>
> >
> <https://github.com/ManageIQ/manageiq-providers-ovirt/blob/
master/app/models/manageiq/providers/redhat/infra_
manager/event_parser.rb#L80-L92
> <https://github.com/ManageIQ/manageiq-providers-ovirt/blob/
master/app/models/manageiq/providers/redhat/infra_
manager/event_parser.rb#L80-L92>>
> >
> >
> > hmmm, while we are on topic, this pretty much looks like that
manageiq
> > does not only rely on the code but also on the actual value of it
> since
> > it is parsing it:
> >
> > # sample message: "Interface nic1 (VirtIO) was added to VM v5.
(User:
> > admin at internal-authz)" message.split(/\s/)[7][0...-1]
> >
> > Is this something we commit to maintain? Or should we commit to
> maintain it?
> >
>
> That is a good point, that isn't very future proof. We should also
find
> a way to make less fragile. Any suggestion?
>
>
> The only doable thing which comes to my mind is something like this:
> The msg is defined like this:
> USER_ADD_VM_POOL_WITH_VMS_FAILED=Failed to create VM Pool ${VmPoolName}
> (User: ${UserName}).
>
> e.g. the msg type and the variables. If we could expose in the api not
> only the substituted msg but also the variable/value binding, we could
> commit to keep the variable names backward compatible.
>
> So, something like:
>
> <event href="/ovirt-engine/api/events/8021" id="8021">
> <code>19</code>
> <type>USER_ADD_VM_POOL_WITH_VMS_FAILED</type>
> <description>the substituted msg.</description>
> <parameters>
> <parameter>
> <key>VmPoolName</key>
> <value>The Pool Name<value>
> </parameter>
> ...
> </parameters>
> </event>
>
> Not really rock solid since the variables would still be defined in the
> AuditLogMessages.properties but still better and still easier to parse
> on the client side.
>
That makes sense to me. I have opened the following bug to track that:
[RFE] Add properties to events
https://bugzilla.redhat.com/1456711
Note that for the particular case of the VM name, which is what ManageIQ
is trying to do in that caode, the current best way is to use the <vm
.../> link that is part of the event. I have opened the following
ManageIQ issue to track it:
Avoid parsing the descriptions of events
https://github.com/ManageIQ/manageiq-providers-ovirt/issues/45
I think it was introduced while working on targeted refresh, but if we have
an alternative then of course it would be better.
>
> >
> > >
> > > That means that if we ever change the meaning of a code the
ManageIQ
> > > provider, for example, will break.
> >
> > Right,then it indeed needs to stay stable.
> > But how is maintaining the enum string different from the code?
It is
> > the same information, so if MIQ doesn't use the name directly
then it
> > doesn't really matter if it's a code or string.
> > Perhaps deprecate the code and keep the name fixed?
> >
> > Thanks,
> > michal
> >
> > >
> > >>>
> > >>> Regards,
> > >>> Juan Hernandez
> > >>>
> > >>>
> > >>> _______________________________________________
> > >>> Devel mailing list
> > >>> Devel at ovirt.org <mailto:Devel at ovirt.org>
> <mailto:Devel at ovirt.org <mailto:Devel at ovirt.org>>
> > >>> http://lists.ovirt.org/mailman/listinfo/devel
> <http://lists.ovirt.org/mailman/listinfo/devel>
> > <http://lists.ovirt.org/mailman/listinfo/devel
> <http://lists.ovirt.org/mailman/listinfo/devel>>
> > >
> > _______________________________________________
> > Devel mailing list
> > Devel at ovirt.org <mailto:Devel at ovirt.org>
> <mailto:Devel at ovirt.org <mailto:Devel at ovirt.org>>
> > http://lists.ovirt.org/mailman/listinfo/devel
> <http://lists.ovirt.org/mailman/listinfo/devel>
> > <http://lists.ovirt.org/mailman/listinfo/devel
> <http://lists.ovirt.org/mailman/listinfo/devel>>
> >
> >
>
>
_______________________________________________
Devel mailing list
Devel at ovirt.org
http://lists.ovirt.org/mailman/listinfo/devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20170530/0f703891/attachment-0001.html>
More information about the Devel
mailing list