* Re: Redfish: Supporting deprecated properties
2020-10-05 21:35 ` Ed Tanous
@ 2020-10-06 0:52 ` Joseph Reynolds
2020-10-06 0:52 ` Joseph Reynolds
2020-10-12 20:24 ` Gunnar Mills
2 siblings, 0 replies; 5+ messages in thread
From: Joseph Reynolds @ 2020-10-06 0:52 UTC (permalink / raw)
To: Ed Tanous, Gunnar Mills; +Cc: OpenBMC Maillist
On 10/5/20 4:35 PM, Ed Tanous wrote:
> On Mon, Oct 5, 2020 at 1:34 PM Gunnar Mills <gmills@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.
...snip...
>
> 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!
Can we discuss this without referencing South Park, season 2, episode
17, please?
- Joseph
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Redfish: Supporting deprecated properties
2020-10-05 21:35 ` Ed Tanous
2020-10-06 0:52 ` Joseph Reynolds
@ 2020-10-06 0:52 ` Joseph Reynolds
2020-10-12 20:24 ` Gunnar Mills
2 siblings, 0 replies; 5+ messages in thread
From: Joseph Reynolds @ 2020-10-06 0:52 UTC (permalink / raw)
To: Ed Tanous, Gunnar Mills; +Cc: OpenBMC Maillist
On 10/5/20 4:35 PM, Ed Tanous wrote:
> On Mon, Oct 5, 2020 at 1:34 PM Gunnar Mills <gmills@linux.vnet.ibm.com> wrote:
>> Felt this needed some broader discussion. Although Redfish tries to
>> avoid, it does deprecate properties.
>>
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Redfish: Supporting deprecated properties
2020-10-05 21:35 ` Ed Tanous
2020-10-06 0:52 ` Joseph Reynolds
2020-10-06 0:52 ` Joseph Reynolds
@ 2020-10-12 20:24 ` Gunnar Mills
2 siblings, 0 replies; 5+ messages in thread
From: Gunnar Mills @ 2020-10-12 20:24 UTC (permalink / raw)
To: Ed Tanous; +Cc: OpenBMC Maillist, kurt.r.taylor
On 10/5/2020 3:35 PM, Ed Tanous wrote:
> On Mon, Oct 5, 2020 at 1:34 PM Gunnar Mills <gmills@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".
It is additional work. I feel it could slow picking up new Redfish
schemas and implementing new properties.
Also, are we planning on continuing our release process? Roughly every 6
months following a Yocto Release?
I could get behind something like support both the deprecated and new
property for one release. See below.
> Supporting it for N+2 releases with a deprecation warning
> return seems reasonable to me,
PATCH deprecation warning only? Not sure how you would return a
deprecation warning on a GET within the spec.
If the client checks the schema or runs the validator they will also
know it is deprecated.
It seems reasonable to me to return a PATCH deprecation warning.
> 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.
"single chassis" flag was a lot of work maintaining. I studied the new
Power / Thermal schemas a bit more, they can coexist.
The existing Power and Thermal will be at .../Thermal .../Power while
the new schemas will be at .../ThermalSubsystem and .../PowerSubsystem.
I think this is better than a flag but can discuss later after.
>> 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.
Not always, Redfish does 4 releases a year now and most of their merging
is the last few weeks of their cycle.
Redfish also has a 30 day IP review which is prescribed by DMTF which is
why it does take a month after they approve a release for it to be public.
> 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?
>
Supporting both LED properties, sure, probably isn't a huge amount of
work (I would not call it trivial though) but supporting both the new
thermal/power and the old thermal/power schemas I think would be, either
via compile flag or just both in /chassis. I am worried about the
developmental and support costs of any "support deprecated properties"
rule so would be more in favor of something more limited like support
both new and deprecated for 1 release.
>> 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?
Disagree it is 3 lines of code. IndicatorLED is used in multiple places,
you suggested a PATCH deprecation warning, and have to make both
properties work.
> 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?
Redfish moved away from IndicatorLED (3 states) to
LocationIndicatorActive (boolean).
More detail in the 2020.3 overview and several issues in the Redfish
repo. The highlights are:
IndicatorLED improperly exposed hardware implementation details
- Redfish attempts to model usage semantics rather than physical
characteristics
This caused interoperability issues discovered during Redfish Plugfests
- Some vendors use “On” as the active state, others use “Blinking”
- Some vendors use “Off” as the inactive state, other use “On”
>
> It should be noted, OCPServerHardwareManagement v0.2.3 requires
> IndicatorLED; Are we ok with breaking our OCP compliance more?
Where is it stated we need to support the OCP profiles?
The OCP profiles are the most well known but there are other profiles
out there. Not all systems in OpenBMC are OCP. I don't think we have
100% compliance today with the OCP profiles either?
> 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.
The first seems reasonable. The second does not for the reasons stated
above. I think before dropping a property that is required by an OCP
profile we can have more discussion but I don't think getting upstream
changes to the OCP profile accepted should be a requirement to starting
the count down. If supporting OCP Baseline Profile / OCP Server Profile
is a requirement, we should document that and probably test in CI.
You mentioned OCPServerHardwareManagement v0.2.3 which was released
April 18, 2018. It is not clear to me if OCP plans to release new
versions or when. Does anyone have insight into this?
Looking at
https://www.opencompute.org/wiki/Hardware_Management/SpecsAndDesigns
> 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.
>
This feels too cumbersome.
I could get behind something like support for 1 release, ~6 months as a
compromise:
Release N-1 - Only the (yet to be) deprecated property (e.g.
IndicatorLED) is supported
Release N - Both the deprecated and new property (e.g.
LocationIndicatorActive) are supported
Release N+1 - Support for the deprecated property is dropped
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread