BMC and Host State Management Refactor

[Andrew Geissler]

On Mon, Nov 21, 2016 at 9:40 PM, Andrew Jeffery <andrew at aj.id.au> wrote:
> On Mon, 2016-11-21 at 20:28 -0600, Andrew Geissler wrote:
>> > On Sun, Nov 20, 2016 at 11:55 PM, Joel Stanley <joel at jms.id.au> wrote:
>> > Hi Andrew and Josh,
>> >
>> > On Sat, Nov 19, 2016 at 7:01 AM, Andrew Geissler <geissonator at gmail.com> wrote:
>> > > Josh and I are working two stories this sprint that deal with
>> > > refactoring the bmc and host state management code out of skeleton
>> > > (#772/#783).  Here’s the proposal on this work.
>> >
>> > Thanks for sending out your plan, this is great. I have a few comments
>> > that came up as I was reading.
>> >
>> > > The overall design for both state management objects is that they will
>> > > provide a set of properties on which to operate.
>> > > - DesiredState
>> > > - CurrentState
>> > >
>> > > CurrentState will be a read only property.
>> >
>> > You've chosen to make the desired and current states be separate,
>> > which works. Another option would be to have them be the same list of
>> > states, so you know that when current==desired you're not waiting on
>> > anything to happen. What do you think?
>> >
>>
>> Hmmm, I'm thinking from a DBUS/REST api perspective here.  2 seems
>> more intuitive, but also I don't think I understand your proposal
>> fully :)
>
> I think you might be misinterpreting. I don't think Joel was suggesting
> you eliminate one of the DesiredState or CurrentState "variables",
> rather that the /types/ of the CurrentState and DesiredState variables
> be equal. That is, that the same set of states can be assigned to both.
>

I see now.  I'm still not seeing any huge advantages on either
proposal over my original.  I think I'm just going to stick with it
for now since there are times where the valid states associated with
each (Desired vs. Current) are different and I think having the two as
I've defined is a bit more user friendly.

>>  What would this look like from an implementation perspective
>> and DBUS/REST interfaces?
>
> No different, just the set of possible states would be consistent
> between the two variables.
>
> Alternatively you could define transitions on the states and rename
> DesiredState to Transition, or something similar. This might better
> align with what you're proposing (a state machine).
>
>>
>> > >
>> > > The host control will have these additional properties:
>> > > - DesiredPowerState
>> > > - CurrentPowerState
>> > >
>> > > Valid states to request for OpenBMC (DesiredState)
>> > > - READY, REBOOT
>> > > Valid states to be in for OpenBMC (CurrentState)
>> > > - NOT_READY, READY (note this proposal removes BASE_APPS and BMC_STARTING).
>
> What does it mean to request a READY state? When can you do this? Can
> you request it whilst the BMC is in NOT_READY? Requesting READY in
> READY doesn't seem productive.
>
> If there are restrictions on combinations of source and target state it
> might be better to make use of the transition concept to spell it out
> (i.e. describe a formal state machine).
>

Yeah good point, READY is not a valid state to request.  We get to
that via systemd, so the only valid DesiredState would be REBOOT.  I'm
trying to keep things as simple as possible right now, and to utilize
as much of systemd's built in function for state transitions.

>> >
>> > Does this need to consider states where the device is updating? That
>> > is, it is not attempting to be ready to control the host, but it is
>> > turned on and able to accept new firmware?
>> >
>>
>> I think READY implies available for code update?  But good point on
>> code updates, when a code update is being performed, a FW_UPDATE state
>> seems reasonable.
>>
>> > >
>> > > READY implies all services started and running successfully (i.e. we
>> > > reached obmc-standby.target)
>> > >
>> > >
>> > > Valid states to request for Host (DesiredState)
>> > > - OFF, ON, REBOOT
>> >
>> > This might need to distinguish between soft-off/soft-reboot and
>> > printer-on-fire OFF requests.
>> >
>>
>> Yeah, the focus of this story was to keep similar function as what's
>> in place now, but refactor into c++ and the new sdbusplus interfaces.
>> We do probably need more work done with things like you say here.
>> We've tended to call the printer-on-fire off's EMERGENCY_ type event
>> requests.  At a minimum, I'll be sure these are tracked in future
>> stories.
>>
>> > > Validate states to be in for Host (CurrentState)
>> > > - OFF, BOOTING, BOOTED
>> >
>> > STANDBY, BOOTING, RUNNING?
>> >
>> > The bikeshed should be blue.
>> >
>>
>> Agree
>
> Okay so the bikeshed is blue, but what about the state names?? ;)

I finally googled this... http://bikeshed.org/  nice

>
> Andrew