Redfish: Supporting deprecated properties

[Ed Tanous]

On Mon, Oct 5, 2020 at 1:34 PM Gunnar Mills <gmills at linux.vnet.ibm.com> wrote:
>
> Felt this needed some broader discussion. Although Redfish tries to
> avoid, it does deprecate properties. In the future, Redfish plans to
> deprecate whole schemas. One of these deprecated properties was the
> IndicatorLED property, replaced by the LocationIndicatorActive property.
> More information on this can be found at (Slide 11):
> http://www.dmtf.org/sites/default/files/Redfish_Release_2020.3_Overview.pdf
>
> On https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/36886, it was
> proposed supporting both the deprecated and new property for some time.
> This would introduce a new Redfish Validator warning about implementing
> a deprecated property. We have other warnings today so maybe not a big deal.
> How long do we need to support both properties?
> Based on releases? 6 months? That feels too long.

Based on releases feels too long?  Can you elaborate on why that's
"too long".  Supporting it for N+2 releases with a deprecation warning
return seems reasonable to me, considering the number of
implementations that we'd break if we did the cutover quickly, and
considering the alternative hasn't existed very long (a matter of
weeks it looks like).  Maybe I'm over/under thinking something here.

> Are our releases used
> broadly enough and release process mature enough?
> If it is not a replacement and just deprecating a property, can we just
> drop the deprecated property immediately when switching to a new schema
> version that deprecates?

I don't think so.  That would break clients, many of whom aren't
connected enough to the mailing list, but would be broken all the
same.

> Do we need to do the same when a schema is deprecated?

Depends on the schema, and I'd say we come up with this posture once
it happens, and depending on impact.  For example, if they deprecate
SessionService (something that every tool in the world uses) that's a
very different posture than if they deprecate the Fan schema, which
very few implementations actually implement in the client side
correctly to the spec.

> This matters
> because Redfish is targeting 2020.4 for new Power and Thermal Subsystem
> Schemas. Redfish plans to deprecate the old Power and Thermal Schemas
> and release new schemas.

In this specific case, I suspect a compile time flag would be my
recommendation, because it's not just a pure deprecation, it's
changing the meaning and intent of a lot of collections.  We already
did this with the "single chassis" flag a long time back, and it
worked quite well.  Those that wanted new behavior got it, those that
wanted old behavior, got it.  It was a nightmare to maintain, but I
suspect this changeover will be a little easier maintenance wise.

>
> I think it is reasonable we support both IndicatorLED and
> LocationIndicatorActive for some time, I'll throw out 2 months.

Not nearly enough time IMO.  It takes longer than that for the spec to
go from PR merged to a versioned API release.  Some gerrit reviews
alone take longer than that.  Also, supporting both for some time is
trivial code-wise.  Are you just worried about the warning, or are you
expecting a significant revamp of the LED stuff in the near future?

> Our
> Redfish implementation is young and I am not sure it is worth the
> developmental and support costs, at least at this time, to maintain
> deprecated properties and schemas for long.

In this case, I'm looking at what would be maybe 3 lines of code?  If
redfish starts deprecating properties every other release, then I
agree, that's not maintainable, but in this case, this seems
unimportant.  One thing I would like to understand: it looks like the
new property doesn't support Blink?  How is that handled in the new
schema?

It should be noted, OCPServerHardwareManagement v0.2.3 requires
IndicatorLED;  Are we ok with breaking our OCP compliance more?  At
this point in time, I'd rather not, so I'd like to see OCP also ratify
the deprecation, and release a new version of their profile before
anything other deprecation happens.

In this specific case, what if we did this:
Now;  Upstream backward compatible LocationIndicatorActive property to
bmcweb.  Upstream changes to OCP schema to also deprecate
IndicatorLed.  Time starts counting once both patches have been
accepted into their respective mainline branches.
N+1 release;  Implement returning a deprecation warning to the user
attempting to use the IndicatorLED PATCH API.
N+2 release; Remove the IndicatorLED property from GET requests, but
continue to accept the property for PATCH requests (we've done this in
other cases).
N+3 release; Disallow PATCH to that property entirely, and continue
with new implementation.  Ideally hold the deprecation warning, but
use judgement about technical debt.

?????

Profit!

>
> Clients can use the schema version to determine which properties are
> available. If needed companies in a fork could maintain backward
> compatibility for longer.

In practice, many clients don't check the schema at all.  I really
wish Redfish had a way for a client to say "I support X version of the
schema, give me the things compatible with that", but I'm not aware of
anything like that.

>
> Thanks,
> Gunnar