[ovirt-devel] Making event types backwards compatible?
Juan Hernández
jhernand at redhat.com
Tue May 30 10:17:25 UTC 2017
On 05/30/2017 12:11 PM, Oved Ourfali wrote:
>
>
> On May 30, 2017 11:28, "Juan Hernández" <jhernand at redhat.com
> <mailto: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>
> > <mailto: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>>
> > > <mailto: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>>
> > <mailto: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>>
> > > <mailto: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>>
> > > <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>>
> > >
> >
> <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>>
> > >
> >
> <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
> <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
> <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.
>
Piotr and Boris explained to me that we need to parse the description
text because the VM may have been already removed when the event is
processed. In that case the link to the VM won't work, so we don't have
an alternative today. But if we implement the above RFE, then we could
use the properties, which should be more reliable.
>
> >
> > >
> > > >
> > > > 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>>
> > <mailto: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>>
> > > <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>>
> > <mailto: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>>
> > > <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>
> http://lists.ovirt.org/mailman/listinfo/devel
> <http://lists.ovirt.org/mailman/listinfo/devel>
>
>
More information about the Devel
mailing list