RFC: new design of phosphor-time-manager on sdbusplus

[Mine]

On Wed, Jan 18, 2017 at 10:44 PM, Patrick Williams <patrick at stwcx.xyz> wrote:
>
> On Tue, Jan 17, 2017 at 03:51:39PM +0800, Mine wrote:
> > On Tue, Jan 17, 2017 at 3:44 AM, Patrick Williams <patrick at stwcx.xyz> wrote:
> > > On Fri, Jan 13, 2017 at 03:42:50PM +0800, Mine wrote:
>
> > I should be meant to create two "instances" on the bus, eventually it looks
> > something like:
> > ```
> > xyz.openbmc_project.Time.Manager
> >     /xyz/openbmc_project/time/host_epoch
> >         org.freedesktop.DBus.Peer
> >         org.freedesktop.DBus.Introspectable
> >         org.freedesktop.DBus.Properties
> >         xyz.openbmc_project.Time.EpochTime
> >     /xyz/openbmc_project/time/bmc_epoch
> >         org.freedesktop.DBus.Peer
> >         org.freedesktop.DBus.Introspectable
> >         org.freedesktop.DBus.Properties
> >         xyz.openbmc_project.Time.EpochTime
>
> There isn't a reason to name the object path "epoch".  /time/bmc and
> /time/host0 is probably more appropriate.

OK.

>
> > > > And there will be no “curr_time_mode/owner” or “requested_time_mode/owner”
> > > > properties on DBUS.
> > >
> > > So these are only stored in phosphor-settingsd now or are they also used
> > > internally for decisions?  I believe the previous implementation had
> > > them exposed more for debug purposes.  Are you going to add them to the
> > > journal at least?
> >
> > Yes, the time_mode and time_owner are still stored in
> > phosphor-settingsd, and they
> > are used internally by phosphor-time-manager, which will register
> > callback for the
> > settings' change and handled accordingly.
> > The difference from the previous implementation is that we do not expose these
> > settings in time-manager's DBus now.
>
> There are two aspects for consideration here:
>
>     1. The current time manager defers changing the host policies until
>        the next boot.  We need to continue this behavior.
>
>     2. If the process restarts we need it to go back into the "current
>        state" and not the "requested state".  How do we make this
>        happen?  The current implementation might also have a flaw here
>        so maybe we log it as an issue for follow up.
>

This is the part I did not understand well on the previous "requested"
and "current" mode.
My consideration/question (for previous code) is:
It has two settings of time mode/owner, one in settingsd, the other in
timemanager, then
which service is *really* responsible for the setting?
E.g. if current time owner is BMC, and mode is NTP,. when settingsd
set time owner to HOST:
1. timemanager save the owner to requested_owner;
2. If host is off, it defers the change, then:
   * In settingsd, owner is HOST, mode is NTP;
   * In timemanager, current owner is BMC, requested owner is HOST, mode is NTP
   It's kindly of complicated and confusing;
3. If host is on, it sets the owner to HOST, and change the mode to
MANUAL (it has specific
comments to indicates that HOST and NTP are exclusive), then:
   * In settingsd, owner is HOST, mode is MANUAL;
   * In timemanager, current owner is HOST, requested owner is HOST,
mode is MANUAL
   So changing settingsd time owner also affects settingsd mode.
   It looks as if timemanager is responsible for the setting.

I feel this piece of logic is too complicated, it's better that we
simplifies it:
1. Enforce that settingsd is the owner of settings, timemanager shall not modify
    settingsd's settings, but only register callbacks on changes.
2. Combine the time mode/owner into:
   * BMC_NTP  ==> Both Bmc/Host Epoch return the same time as BMC's NTP time;
                              Setting epoch is not allowed
   * BMC_MANUAL ==> Both Bmc/Host Epoch return the same BMC time;
                                    Setting BMC epoch is allowed,
setting HOST epoch is not allowed
   * HOST_MANUAL ==> Both Bmc/Host Epoch return the same HOST time;
                                      Settings BMC epoch is not
allowed, settings HOST epoch is allowed
                                      (It looks like BMC_MANUAL and
HOST_MANUAL can be merged as MANUAL?
                                       since they are actually the same)
   * SPLIT_NTP  ==> Bmc and Host Epoch are separated;
                                BMC uses NTP time, and setting is not allowed;
                                Host epoch can be get and set;
   * SPLIT_MANUAL ==> Bmc and Host Epoch are separated
                                      Both can be get and set.

What's your opinion?

> > What do you mean by "add them to the journal"?
>
> Use phosphor-logging.

Yes, the code uses phosphor-logging to send log to journal.

>
> --
> Patrick Williams