Openbmc archive at lore.kernel.org
 help / color / Atom feed
* Redfish: Supporting deprecated properties
@ 2020-10-05 20:34 Gunnar Mills
  2020-10-05 21:35 ` Ed Tanous
  0 siblings, 1 reply; 5+ messages in thread
From: Gunnar Mills @ 2020-10-05 20:34 UTC (permalink / raw)
  To: openbmc; +Cc: ed

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. 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?
Do we need to do the same when a schema is deprecated? 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.

I think it is reasonable we support both IndicatorLED and 
LocationIndicatorActive for some time, I'll throw out 2 months. 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.

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

Thanks,
Gunnar

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Redfish: Supporting deprecated properties
  2020-10-05 20:34 Redfish: Supporting deprecated properties Gunnar Mills
@ 2020-10-05 21:35 ` Ed Tanous
  2020-10-06  0:52   ` Joseph Reynolds
                     ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Ed Tanous @ 2020-10-05 21:35 UTC (permalink / raw)
  To: Gunnar Mills; +Cc: OpenBMC Maillist

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

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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 20:34 Redfish: Supporting deprecated properties Gunnar Mills
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

Openbmc archive at lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/openbmc/0 openbmc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 openbmc openbmc/ https://lore.kernel.org/openbmc \
		openbmc@lists.ozlabs.org openbmc@ozlabs.org
	public-inbox-index openbmc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.openbmc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git