Re: BMC and Host State Management Refactor

[Andrew Geissler <geissonator@gmail.com>]

On Sun, Nov 20, 2016 at 11:55 PM, Joel Stanley <joel@jms.id.au> wrote:
> Hi Andrew and Josh,
>
> On Sat, Nov 19, 2016 at 7:01 AM, Andrew Geissler <geissonator@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 :)  What would this look like from an implementation perspective
and DBUS/REST interfaces?

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

>>
>> BOOTED implies petitboot has reached it's "ready" state.
>>
>>
>> Valid power states to request for DesiredPowerState
>> - OFF, ON
>> Validate states to be in for CurrentPowerState
>> - OFF, TRANSITION, ON
>
> Does TRANSITION need to distinguish between coming up and going down?
>

I'd prefer to keep simple and let the user determine this by looking
at the DesiredPowerState if it's needed.

>>
>> Patrick and I had some discussion on providing more details on the
>> host booted state (petitboot vs. os booted) but that can be an
>> extension in the future if needed.
>
> I think baking this in from the start is a worthy goal. This then
> feeds into the user-facing APIs, weather that is an IPMI sensor or a
> REST endpoint, that can be queried to ask "what is that damn computer
> up to". If we can make it informative enough that no one ever feels
> the need to watch the console output when the machine is booting then
> we have done a good job.
>

Agree but please see above comment about the scope of this story.  I'm
concerned this opens up a can of worms on extra stuff we'll need from
hostboot and petitboot.  I'll def make sure to write this down for
future stories and there wont be anything in this base design that
will limit that later.

> Cheers,
>
> Joel

Thanks for the feedback Joel!
Andrew