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

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

[Mine]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

I’d like to discuss the design of phosphor-time-manager with the new
sdbusplus interfaces.

Please see below background and new design, let me know if you have any
comments.
Thanks!
Background

Legacy time-manager expose below interfaces and methods:

   - curr_time_mode
   - curr_time_owner
   - requested_time_mode
   - requested_time_owner
   - GetTime(target)
   - SetTime(target, time)

The implementation of Get/SetTime() will check the current time mode and
owner, and do below:

   - For GetTime(<target>):
      - If target is “bmc”, return BMC time;
      - If target is “host”, return Host time;
      - Else, return error
   - For SetTime(target, time):
      - If target is “bmc”:
         - If mode is NTP, return error
         - If owner is “HOST”, return error
         - Set BMC time
      - If target is “host”:
         - If owner is “BMC”, return error
         - Set Host time
         - If owner is “SPLIT”, save the diff between BMC time and Host
         time;

New Design

Now with sdbusplus, we have EpochTime interface, with elapsed property.

After discussion with Li Yi and Vishwa, we decided to implement as below:

Create two objects:

   - BmcEpoch
   - HostEpoch

They both implements EpochTime interface.

For BmcEpoch:

   - When elapsed() is called, return BMC time;
   - When elapsed(us) is called, use above SetTime(“bmc”) logic

For HostEpoch:

   - When elapsed() is called, return HOST time;
   - When elapsed(us) is called, use above SetTime(“host”) logic.

And there will be no “curr_time_mode/owner” or “requested_time_mode/owner”
properties on DBUS.

--
BRs,
Lei YU

[-- Attachment #2: Type: text/html, Size: 8747 bytes --]

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

[Deepak Kodihalli]

Hello Lei,


On 13/01/17 1:12 pm, Mine wrote:
> I’d like to discuss the design of |phosphor-time-manager| with the new
> sdbusplus interfaces.
>
> Please see below background and new design, let me know if you have any
> comments.
> Thanks!
>
>
>       Background
>
> Legacy time-manager expose below interfaces and methods:
>
>   * curr_time_mode
>   * curr_time_owner
>   * requested_time_mode
>   * requested_time_owner
>   * GetTime(target)
>   * SetTime(target, time)
>
> The implementation of |Get/SetTime()| will check the current time mode
> and owner, and do below:
>
>   * For |GetTime(<target>)|:
>       o If target is “bmc”, return BMC time;
>       o If target is “host”, return Host time;
>       o Else, return error
>   * For |SetTime(target, time)|:
>       o If target is “bmc”:
>           + If mode is NTP, return error
>           + If owner is “HOST”, return error
>           + Set BMC time
>       o If target is “host”:
>           + If owner is “BMC”, return error
>           + Set Host time
>           + If owner is “SPLIT”, save the diff between BMC time and Host
>             time;
>
>
>       New Design
>
> Now with sdbusplus, we have EpochTime interface, with |elapsed| property.
>
> After discussion with Li Yi and Vishwa, we decided to implement as below:
>
> Create two objects:
>
>   * |BmcEpoch|
>   * |HostEpoch|


Could these be just associations (via the association interface) to a 
single object? I'm not sure if the user the Epoch interface knows Host 
vs BMC? Shouldn't that be picked from the settings, or do we really have 
explicit calls to fetch/set bmc and host time?

> They both implements EpochTime interface.
>
> For |BmcEpoch|:
>
>   * When |elapsed()| is called, return BMC time;
>   * When |elapsed(us)| is called, use above SetTime(“bmc”) logic
>
> For |HostEpoch|:
>
>   * When |elapsed()| is called, return HOST time;
>   * When |elapsed(us)| is called, use above SetTime(“host”) logic.
>
> And there will be no “curr_time_mode/owner” or
> “requested_time_mode/owner” properties on DBUS.
>
> --
> BRs,
> Lei YU

Regards,
Deepak

>
>
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
>

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

[Mine]

Hi Deepak

On Fri, Jan 13, 2017 at 5:11 PM, Deepak Kodihalli
<dkodihal@linux.vnet.ibm.com> wrote:
> Hello Lei,
>
>
> On 13/01/17 1:12 pm, Mine wrote:
>>
>> I’d like to discuss the design of |phosphor-time-manager| with the new
>> sdbusplus interfaces.
>>
>> Please see below background and new design, let me know if you have any
>> comments.
>> Thanks!
>>
>>
>>       Background
>>
>> Legacy time-manager expose below interfaces and methods:
>>
>>   * curr_time_mode
>>   * curr_time_owner
>>   * requested_time_mode
>>   * requested_time_owner
>>   * GetTime(target)
>>   * SetTime(target, time)
>>
>> The implementation of |Get/SetTime()| will check the current time mode
>> and owner, and do below:
>>
>>   * For |GetTime(<target>)|:
>>       o If target is “bmc”, return BMC time;
>>       o If target is “host”, return Host time;
>>       o Else, return error
>>   * For |SetTime(target, time)|:
>>       o If target is “bmc”:
>>           + If mode is NTP, return error
>>           + If owner is “HOST”, return error
>>           + Set BMC time
>>       o If target is “host”:
>>           + If owner is “BMC”, return error
>>           + Set Host time
>>           + If owner is “SPLIT”, save the diff between BMC time and Host
>>             time;
>>
>>
>>       New Design
>>
>> Now with sdbusplus, we have EpochTime interface, with |elapsed| property.
>>
>> After discussion with Li Yi and Vishwa, we decided to implement as below:
>>
>> Create two objects:
>>
>>   * |BmcEpoch|
>>   * |HostEpoch|
>
>
>
> Could these be just associations (via the association interface) to a single
> object? I'm not sure if the user the Epoch interface knows Host vs BMC?
> Shouldn't that be picked from the settings, or do we really have explicit
> calls to fetch/set bmc and host time?
>
According to Vishwa, it is a requirement for time manager to keep both
BMC and HOST time.

And from previous design, whether the settings' time_owner is BMC or
HOST or else,
the user of time-manager will get/set the BMC or HOST time.
E.g.
1. Host's IPMI SEL_SET_TIME will set HOST time.
    In this case, with Epoch interface, hostipmid will set HostEpoch
service's "elasped" property.
2. User is able to see both BmcEpoch and HostEpoch service on DBus,
and he is free to
get/set the "elasped" property on each service.
    The implementation will handle if the set should succeed or not.

>> They both implements EpochTime interface.
>>
>> For |BmcEpoch|:
>>
>>   * When |elapsed()| is called, return BMC time;
>>   * When |elapsed(us)| is called, use above SetTime(“bmc”) logic
>>
>> For |HostEpoch|:
>>
>>   * When |elapsed()| is called, return HOST time;
>>   * When |elapsed(us)| is called, use above SetTime(“host”) logic.
>>
>> And there will be no “curr_time_mode/owner” or
>> “requested_time_mode/owner” properties on DBUS.
>>
>> --
>> BRs,
>> Lei YU
>
>
> Regards,
> Deepak
>
>>
>>
>>
>> _______________________________________________
>> openbmc mailing list
>> openbmc@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/openbmc
>>
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

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

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

On Fri, Jan 13, 2017 at 03:42:50PM +0800, Mine wrote:
> New Design
> 
> Create two objects:

You mean 'classes' or 'objects'?  I think you mean two classes, which
initially we will only have 1 instance of each.  Please make sure the
HostEpoch implementation can relatively-easily be enhanced to have
multiple instances.

>    - BmcEpoch
>    - HostEpoch
> 
> They both implements EpochTime interface.
> 
> For BmcEpoch:
> 
>    - When elapsed() is called, return BMC time;
>    - When elapsed(us) is called, use above SetTime(“bmc”) logic
> 
> For HostEpoch:
> 
>    - When elapsed() is called, return HOST time;
>    - When elapsed(us) is called, use above SetTime(“host”) logic.

Seems ok.

The errors also need to be converted to dbus-defined errors, right?

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

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Mine]

Hi Patrick,

On Tue, Jan 17, 2017 at 3:44 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
>
> On Fri, Jan 13, 2017 at 03:42:50PM +0800, Mine wrote:
> > New Design
> >
> > Create two objects:
>
> You mean 'classes' or 'objects'?  I think you mean two classes, which
> initially we will only have 1 instance of each.  Please make sure the
> HostEpoch implementation can relatively-easily be enhanced to have
> multiple instances.

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

```
The service's BUSNAME is still `xyz.openbmc_project.Time.Manager`, and
it creates two objects, one for BmcEpoch and the other for HostEpoch, who
both implement xyz.openbmc_project.Time.EpochTime interface.

>
> >    - BmcEpoch
> >    - HostEpoch
> >
> > They both implements EpochTime interface.
> >
> > For BmcEpoch:
> >
> >    - When elapsed() is called, return BMC time;
> >    - When elapsed(us) is called, use above SetTime(“bmc”) logic
> >
> > For HostEpoch:
> >
> >    - When elapsed() is called, return HOST time;
> >    - When elapsed(us) is called, use above SetTime(“host”) logic.
>
> Seems ok.
>
> The errors also need to be converted to dbus-defined errors, right?
>
> > 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.

What do you mean by "add them to the journal"?

>
> --
> Patrick Williams

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

[vishwa]

On 01/17/2017 01:21 PM, Mine wrote:
> Hi Patrick,
>
> On Tue, Jan 17, 2017 at 3:44 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
>> On Fri, Jan 13, 2017 at 03:42:50PM +0800, Mine wrote:
>>> New Design
>>>
>>> Create two objects:
>> You mean 'classes' or 'objects'?  I think you mean two classes, which
>> initially we will only have 1 instance of each.  Please make sure the
>> HostEpoch implementation can relatively-easily be enhanced to have
>> multiple instances.
> 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
>
> ```
> The service's BUSNAME is still `xyz.openbmc_project.Time.Manager`, and
> it creates two objects, one for BmcEpoch and the other for HostEpoch, who
> both implement xyz.openbmc_project.Time.EpochTime interface.
So TimeManager still owns the responsibility of maintaining offsets etc 
right ?
>>>     - BmcEpoch
>>>     - HostEpoch
>>>
>>> They both implements EpochTime interface.
>>>
>>> For BmcEpoch:
>>>
>>>     - When elapsed() is called, return BMC time;
>>>     - When elapsed(us) is called, use above SetTime(“bmc”) logic
>>>
>>> For HostEpoch:
>>>
>>>     - When elapsed() is called, return HOST time;
>>>     - When elapsed(us) is called, use above SetTime(“host”) logic.
>> Seems ok.
>>
>> The errors also need to be converted to dbus-defined errors, right?
>>
>>> 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.
>
> What do you mean by "add them to the journal"?
I believe he is asking you to put that information in the journal traces 
so that we can use them for debug. I think you don't need anything more 
to cater to that request as the original implementation already puts 
those as journal traces.
>
>> --
>> Patrick Williams
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

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

[Mine]

[-- Attachment #1: Type: text/plain, Size: 3555 bytes --]

Hi Vishwa,

On Wed, Jan 18, 2017 at 7:07 PM, vishwa <vishwa@linux.vnet.ibm.com> wrote:
> On 01/17/2017 01:21 PM, Mine wrote:
>>
>> Hi Patrick,
>>
>> On Tue, Jan 17, 2017 at 3:44 AM, Patrick Williams <patrick@stwcx.xyz>
>> wrote:
>>>
>>> On Fri, Jan 13, 2017 at 03:42:50PM +0800, Mine wrote:
>>>>
>>>> New Design
>>>>
>>>> Create two objects:
>>>
>>> You mean 'classes' or 'objects'?  I think you mean two classes, which
>>> initially we will only have 1 instance of each.  Please make sure the
>>> HostEpoch implementation can relatively-easily be enhanced to have
>>> multiple instances.
>>
>> 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
>>
>> ```
>> The service's BUSNAME is still `xyz.openbmc_project.Time.Manager`, and
>> it creates two objects, one for BmcEpoch and the other for HostEpoch, who
>> both implement xyz.openbmc_project.Time.EpochTime interface.
>
> So TimeManager still owns the responsibility of maintaining offsets etc
> right ?

Not exactly, HostEpoch will own the offset and maintain it.
But yes, overall HostEpoch is part of TimeManager, so you can say
TimeManager owns the offset as well.

>
>>>>     - BmcEpoch
>>>>     - HostEpoch
>>>>
>>>> They both implements EpochTime interface.
>>>>
>>>> For BmcEpoch:
>>>>
>>>>     - When elapsed() is called, return BMC time;
>>>>     - When elapsed(us) is called, use above SetTime(“bmc”) logic
>>>>
>>>> For HostEpoch:
>>>>
>>>>     - When elapsed() is called, return HOST time;
>>>>     - When elapsed(us) is called, use above SetTime(“host”) logic.
>>>
>>> Seems ok.
>>>
>>> The errors also need to be converted to dbus-defined errors, right?
>>>
>>>> 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.
>>
>> What do you mean by "add them to the journal"?
>
> I believe he is asking you to put that information in the journal traces
so
> that we can use them for debug. I think you don't need anything more to
> cater to that request as the original implementation already puts those as
> journal traces.

Sure.
The code will use sdbusplus's logging to send journal logs.

>>
>>
>>> --
>>> Patrick Williams
>>
>> _______________________________________________
>> openbmc mailing list
>> openbmc@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/openbmc
>
>

[-- Attachment #2: Type: text/html, Size: 4925 bytes --]

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

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

On Tue, Jan 17, 2017 at 03:51:39PM +0800, Mine wrote:
> On Tue, Jan 17, 2017 at 3:44 AM, Patrick Williams <patrick@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.

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

> What do you mean by "add them to the journal"?

Use phosphor-logging.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Mine]

On Wed, Jan 18, 2017 at 10:44 PM, Patrick Williams <patrick@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@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

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

[vishwa]

hey Lei,

I will add some here. Hopefully that helps clarify.

On 01/19/2017 09:18 AM, Mine wrote:
> On Wed, Jan 18, 2017 at 10:44 PM, Patrick Williams <patrick@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@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?

'Settingsd' provides the system level settings that are settable by 
users at any point of time. This
should be treated as "Customer request". This does not mean that 
whatever that is written to settings
is readily acceptable by the consumers. Putting a value in 'settings' 
indicates that 'I want this
value to be used whenever the system condition allows it'. If some 
settings are related to changing
the IP address, then fair enough, we don't really want to defer applying 
that until some system
condition is reached. However, the time owner is something that can only 
be set when the host is OFF
and this is done to make sure that we start clean and not get into 
managing on the fly offsets.

So the values that get updated in 'settings' are *Totally* controlled by 
settingsd and no other daemon updates the values.
> 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:

That is not true. Its the other way.
>     * 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;

When the Host is OFF, time-manager consumes all the values readily as 
and when the updates are made to
settings since we are still at standby and we are safe.
> 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 am sorry if you feel that this logic is complicated. But let me try to 
help you on this. It just
follows one thumb rule and that is:

* When timemanager receives the signal from settingsd telling that some 
of the settings have changed,
  then it will *

1/. Apply the changes into timemanager internal config immediately if 
the HOST is OFF ( actually if
  pgood is off )

2/. If PGOOD is [On], then the changes are *not* applied into current_* 
and are kept as *requested*.
When the Pgood turns back to [Off] state again ( because time manager 
receives a signal telling so ),
that the updates in *request* are consumed into *current* and the 
necessary house keeping is done.
> 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.

This is exactly what time manager is doing today. I am not sure why you 
are thinking that time manager
is updating the settings. Settingsd is in *complete* control of owning 
'settings' and time manager
just receives the signal broadcasted by 'settings' telling about a 
change in the property.

The curr_time_mode and requested_time_mode are totally *private* to time 
manager and they are there just to cater to the usecase that I mentioned 
above. Those *private* settings are *not* settable by an outside 
application. They are read-only dbus properties just to help in debug.
> 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?

I hope that helped ?


>
>>> 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
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

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

[Mine]

Hi Vishwa,

Thanks for the detailed explanation, it helps a lot.
Please see my comments inline.

On Thu, Jan 19, 2017 at 2:11 PM, vishwa <vishwa@linux.vnet.ibm.com> wrote:
> hey Lei,
>
> I will add some here. Hopefully that helps clarify.
>
> On 01/19/2017 09:18 AM, Mine wrote:
>>
>> On Wed, Jan 18, 2017 at 10:44 PM, Patrick Williams <patrick@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@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?
>
>
> 'Settingsd' provides the system level settings that are settable by users at
> any point of time. This
> should be treated as "Customer request". This does not mean that whatever
> that is written to settings
> is readily acceptable by the consumers. Putting a value in 'settings'
> indicates that 'I want this
> value to be used whenever the system condition allows it'. If some settings
> are related to changing
> the IP address, then fair enough, we don't really want to defer applying
> that until some system
> condition is reached. However, the time owner is something that can only be
> set when the host is OFF
> and this is done to make sure that we start clean and not get into managing
> on the fly offsets.
>
> So the values that get updated in 'settings' are *Totally* controlled by
> settingsd and no other daemon updates the values.
>>
>> 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:
>
>
> That is not true. Its the other way.
>>
>>     * 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;
>
>
> When the Host is OFF, time-manager consumes all the values readily as and
> when the updates are made to
> settings since we are still at standby and we are safe.

OK, I did not notice it happens on when host is off.

>>
>> 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 am sorry if you feel that this logic is complicated. But let me try to
> help you on this. It just
> follows one thumb rule and that is:
>
> * When timemanager receives the signal from settingsd telling that some of
> the settings have changed,
>  then it will *
>
> 1/. Apply the changes into timemanager internal config immediately if the
> HOST is OFF ( actually if
>  pgood is off )
>
> 2/. If PGOOD is [On], then the changes are *not* applied into current_* and
> are kept as *requested*.
> When the Pgood turns back to [Off] state again ( because time manager
> receives a signal telling so ),
> that the updates in *request* are consumed into *current* and the necessary
> house keeping is done.

OK, this is crystal clear :)
But this is not the whole story of the current code. See below example that
timemanager also set settings to settingsd or disagrees with settingsd.

>>
>> 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.
>
>
> This is exactly what time manager is doing today. I am not sure why you are
> thinking that time manager
> is updating the settings. Settingsd is in *complete* control of owning
> 'settings' and time manager
> just receives the signal broadcasted by 'settings' telling about a change in
> the property.

In current timemanager, it **does** set settings to settingsd, as
describe above:
0. Settingsd's time owner is BMC, mode is NTP;
1. Someone changes owner to HOST in settingsd;
2. timemanager receives the callback, set the owner to HOST,
   and because HOST and NTP are exclusive, it also set the mode to MANUAL
3. settingsd's time_mode is now **changed** to MANUAL.

Another case the timemanager disagree with settingsd is:
0. Settingsd's time owner is HOST, mode is MANUAL;
1. Someone changes mode to NTP;
2. timemanager receives the callback, and check it's not allowed, it
prints a log and do nothing;
3. Now settingsd's time owner is HOST, mode is NTP;
    while timemanager refuses this settings, its behavior is still HOST/MANUAL.

>
> The curr_time_mode and requested_time_mode are totally *private* to time
> manager and they are there just to cater to the usecase that I mentioned
> above. Those *private* settings are *not* settable by an outside
> application. They are read-only dbus properties just to help in debug.
>>
>> 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?
>
>
> I hope that helped ?

Currently the time_mode/owner has 8 combinations, some of them are not valid.
Could you help to verify what's the expected behavior in each combinations?

Mode      | Owner | Set BMC Time  | Set Host Time
------------- | --------- | ----------------------- |
-------------------------------------
NTP        | BMC   | Not allowed      | Not allowed
NTP        | HOST | Invalid case     | Invalid case
NTP        | SPLIT | Not allowed      | OK, and just save offset
NTP        | BOTH | Now allowed     | OK, and set time to BMC
MANUAL | BMC   | OK                  | Not allowed
MANUAL | HOST | Not allowed      | OK, and set time to BMC
MANUAL | SPLIT | OK                  | OK, and just save offset
MANUAL | BOTH | OK                  | OK, and set time to BMC

If my understanding is correct, then we can see:
* NTP/HOST is invalid case, such case shall not exist, the current
implementation
  either hacks settingsd to change the mode to MANUAL, or just behaves
like MANUAL;
* NTP/BOTH has the same behavior as MANULA/HOST, they can be merged.

That's why I propose to combine the settings to:
* NTP-BMC
* NTP-SPLIT
* NTP-BOTH (or MANUAL-HOST)
* MANUAL-BMC
* MANUAL-SPLIT
* MANUAL-BOTH

Then we don't have to "hack" or "disagree" with settingsd, and the
logic could be
a little simpler.

>
>
>>
>>>> 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
>>
>> _______________________________________________
>> openbmc mailing list
>> openbmc@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/openbmc
>
>

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

[vishwa]

hey Lei,

:), I need the data if you see that 'time manager' is updating settings. 
There is no way it can do that since there is no instance of 'Set' being 
called on settings from timemanager.

Its one of these only:

1/. setting::Get on the first boot

2/. Wait on the signal from settings

On 01/19/2017 01:07 PM, Mine wrote:
> Hi Vishwa,
>
> Thanks for the detailed explanation, it helps a lot.
> Please see my comments inline.
>
> On Thu, Jan 19, 2017 at 2:11 PM, vishwa <vishwa@linux.vnet.ibm.com> wrote:
>> hey Lei,
>>
>> I will add some here. Hopefully that helps clarify.
>>
>> On 01/19/2017 09:18 AM, Mine wrote:
>>> On Wed, Jan 18, 2017 at 10:44 PM, Patrick Williams <patrick@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@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?
>>
>> 'Settingsd' provides the system level settings that are settable by users at
>> any point of time. This
>> should be treated as "Customer request". This does not mean that whatever
>> that is written to settings
>> is readily acceptable by the consumers. Putting a value in 'settings'
>> indicates that 'I want this
>> value to be used whenever the system condition allows it'. If some settings
>> are related to changing
>> the IP address, then fair enough, we don't really want to defer applying
>> that until some system
>> condition is reached. However, the time owner is something that can only be
>> set when the host is OFF
>> and this is done to make sure that we start clean and not get into managing
>> on the fly offsets.
>>
>> So the values that get updated in 'settings' are *Totally* controlled by
>> settingsd and no other daemon updates the values.
>>> 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:
>>
>> That is not true. Its the other way.
>>>      * 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;
>>
>> When the Host is OFF, time-manager consumes all the values readily as and
>> when the updates are made to
>> settings since we are still at standby and we are safe.
> OK, I did not notice it happens on when host is off.
>
>>> 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 am sorry if you feel that this logic is complicated. But let me try to
>> help you on this. It just
>> follows one thumb rule and that is:
>>
>> * When timemanager receives the signal from settingsd telling that some of
>> the settings have changed,
>>   then it will *
>>
>> 1/. Apply the changes into timemanager internal config immediately if the
>> HOST is OFF ( actually if
>>   pgood is off )
>>
>> 2/. If PGOOD is [On], then the changes are *not* applied into current_* and
>> are kept as *requested*.
>> When the Pgood turns back to [Off] state again ( because time manager
>> receives a signal telling so ),
>> that the updates in *request* are consumed into *current* and the necessary
>> house keeping is done.
> OK, this is crystal clear :)
> But this is not the whole story of the current code. See below example that
> timemanager also set settings to settingsd or disagrees with settingsd.
>
>>> 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.
>>
>> This is exactly what time manager is doing today. I am not sure why you are
>> thinking that time manager
>> is updating the settings. Settingsd is in *complete* control of owning
>> 'settings' and time manager
>> just receives the signal broadcasted by 'settings' telling about a change in
>> the property.
> In current timemanager, it **does** set settings to settingsd, as
> describe above:
> 0. Settingsd's time owner is BMC, mode is NTP;
> 1. Someone changes owner to HOST in settingsd;
> 2. timemanager receives the callback, set the owner to HOST,
>     and because HOST and NTP are exclusive, it also set the mode to MANUAL
> 3. settingsd's time_mode is now **changed** to MANUAL.
>
> Another case the timemanager disagree with settingsd is:
> 0. Settingsd's time owner is HOST, mode is MANUAL;
> 1. Someone changes mode to NTP;
> 2. timemanager receives the callback, and check it's not allowed, it
> prints a log and do nothing;
> 3. Now settingsd's time owner is HOST, mode is NTP;
>      while timemanager refuses this settings, its behavior is still HOST/MANUAL.
>
>> The curr_time_mode and requested_time_mode are totally *private* to time
>> manager and they are there just to cater to the usecase that I mentioned
>> above. Those *private* settings are *not* settable by an outside
>> application. They are read-only dbus properties just to help in debug.
>>> 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?
>>
>> I hope that helped ?
> Currently the time_mode/owner has 8 combinations, some of them are not valid.
> Could you help to verify what's the expected behavior in each combinations?
>
> Mode      | Owner | Set BMC Time  | Set Host Time
> ------------- | --------- | ----------------------- |
> -------------------------------------
> NTP        | BMC   | Not allowed      | Not allowed
> NTP        | HOST | Invalid case     | Invalid case
> NTP        | SPLIT | Not allowed      | OK, and just save offset
> NTP        | BOTH | Now allowed     | OK, and set time to BMC
> MANUAL | BMC   | OK                  | Not allowed
> MANUAL | HOST | Not allowed      | OK, and set time to BMC
> MANUAL | SPLIT | OK                  | OK, and just save offset
> MANUAL | BOTH | OK                  | OK, and set time to BMC
>
> If my understanding is correct, then we can see:
> * NTP/HOST is invalid case, such case shall not exist, the current
> implementation
>    either hacks settingsd to change the mode to MANUAL, or just behaves
> like MANUAL;
> * NTP/BOTH has the same behavior as MANULA/HOST, they can be merged.
>
> That's why I propose to combine the settings to:
> * NTP-BMC
> * NTP-SPLIT
> * NTP-BOTH (or MANUAL-HOST)
> * MANUAL-BMC
> * MANUAL-SPLIT
> * MANUAL-BOTH
>
> Then we don't have to "hack" or "disagree" with settingsd, and the
> logic could be
> a little simpler.
>
>>
>>>>> 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
>>> _______________________________________________
>>> openbmc mailing list
>>> openbmc@lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/openbmc
>>

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

[Mine]

Hi Vishwa,

I checked the code again, and find out it does not set settings back,
but only set it internally.
That's good :)

So the table is updated accordingly:

Mode      | Owner | Set BMC Time  | Set Host Time
------------- | --------- | ----------------------- |
-------------------------------------
NTP        | BMC   | Not allowed      | Not allowed
NTP        | HOST | Not allowed      | OK, and set time to BMC
NTP        | SPLIT | Not allowed      | OK, and just save offset
NTP        | BOTH | Now allowed     | OK, and set time to BMC
MANUAL | BMC   | OK                  | Not allowed
MANUAL | HOST | Not allowed      | OK, and set time to BMC
MANUAL | SPLIT | OK                  | OK, and just save offset
MANUAL | BOTH | OK                  | OK, and set time to BMC

NPT/HOST is the same as NTP/BOTH, which has the same
behavior as MANULA/HOST, they can be merged.

So the updated proposals:
* NTP-BMC
* NTP-SPLIT
* NTP-BOTH (or MANUAL-HOST, or NTP-HOST)
* MANUAL-BMC
* MANUAL-SPLIT
* MANUAL-BOTH

Anyway, please confirm the above logic is correct, and I can follow
the logic in the new code, no matter if my proposal is accepted or not.

Btw, is there any specific reason why the time mode/owner is only changed
when host is off?

Thanks!

--
BRs,
Lei YU

On Thu, Jan 19, 2017 at 4:39 PM, vishwa <vishwa@linux.vnet.ibm.com> wrote:
> hey Lei,
>
> :), I need the data if you see that 'time manager' is updating settings.
> There is no way it can do that since there is no instance of 'Set' being
> called on settings from timemanager.
>
> Its one of these only:
>
> 1/. setting::Get on the first boot
>
> 2/. Wait on the signal from settings
>
>
> On 01/19/2017 01:07 PM, Mine wrote:
>>
>> Hi Vishwa,
>>
>> Thanks for the detailed explanation, it helps a lot.
>> Please see my comments inline.
>>
>> On Thu, Jan 19, 2017 at 2:11 PM, vishwa <vishwa@linux.vnet.ibm.com> wrote:
>>>
>>> hey Lei,
>>>
>>> I will add some here. Hopefully that helps clarify.
>>>
>>> On 01/19/2017 09:18 AM, Mine wrote:
>>>>
>>>> On Wed, Jan 18, 2017 at 10:44 PM, Patrick Williams <patrick@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@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?
>>>
>>>
>>> 'Settingsd' provides the system level settings that are settable by users
>>> at
>>> any point of time. This
>>> should be treated as "Customer request". This does not mean that whatever
>>> that is written to settings
>>> is readily acceptable by the consumers. Putting a value in 'settings'
>>> indicates that 'I want this
>>> value to be used whenever the system condition allows it'. If some
>>> settings
>>> are related to changing
>>> the IP address, then fair enough, we don't really want to defer applying
>>> that until some system
>>> condition is reached. However, the time owner is something that can only
>>> be
>>> set when the host is OFF
>>> and this is done to make sure that we start clean and not get into
>>> managing
>>> on the fly offsets.
>>>
>>> So the values that get updated in 'settings' are *Totally* controlled by
>>> settingsd and no other daemon updates the values.
>>>>
>>>> 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:
>>>
>>>
>>> That is not true. Its the other way.
>>>>
>>>>      * 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;
>>>
>>>
>>> When the Host is OFF, time-manager consumes all the values readily as and
>>> when the updates are made to
>>> settings since we are still at standby and we are safe.
>>
>> OK, I did not notice it happens on when host is off.
>>
>>>> 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 am sorry if you feel that this logic is complicated. But let me try to
>>> help you on this. It just
>>> follows one thumb rule and that is:
>>>
>>> * When timemanager receives the signal from settingsd telling that some
>>> of
>>> the settings have changed,
>>>   then it will *
>>>
>>> 1/. Apply the changes into timemanager internal config immediately if the
>>> HOST is OFF ( actually if
>>>   pgood is off )
>>>
>>> 2/. If PGOOD is [On], then the changes are *not* applied into current_*
>>> and
>>> are kept as *requested*.
>>> When the Pgood turns back to [Off] state again ( because time manager
>>> receives a signal telling so ),
>>> that the updates in *request* are consumed into *current* and the
>>> necessary
>>> house keeping is done.
>>
>> OK, this is crystal clear :)
>> But this is not the whole story of the current code. See below example
>> that
>> timemanager also set settings to settingsd or disagrees with settingsd.
>>
>>>> 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.
>>>
>>>
>>> This is exactly what time manager is doing today. I am not sure why you
>>> are
>>> thinking that time manager
>>> is updating the settings. Settingsd is in *complete* control of owning
>>> 'settings' and time manager
>>> just receives the signal broadcasted by 'settings' telling about a change
>>> in
>>> the property.
>>
>> In current timemanager, it **does** set settings to settingsd, as
>> describe above:
>> 0. Settingsd's time owner is BMC, mode is NTP;
>> 1. Someone changes owner to HOST in settingsd;
>> 2. timemanager receives the callback, set the owner to HOST,
>>     and because HOST and NTP are exclusive, it also set the mode to MANUAL
>> 3. settingsd's time_mode is now **changed** to MANUAL.
>>
>> Another case the timemanager disagree with settingsd is:
>> 0. Settingsd's time owner is HOST, mode is MANUAL;
>> 1. Someone changes mode to NTP;
>> 2. timemanager receives the callback, and check it's not allowed, it
>> prints a log and do nothing;
>> 3. Now settingsd's time owner is HOST, mode is NTP;
>>      while timemanager refuses this settings, its behavior is still
>> HOST/MANUAL.
>>
>>> The curr_time_mode and requested_time_mode are totally *private* to time
>>> manager and they are there just to cater to the usecase that I mentioned
>>> above. Those *private* settings are *not* settable by an outside
>>> application. They are read-only dbus properties just to help in debug.
>>>>
>>>> 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?
>>>
>>>
>>> I hope that helped ?
>>
>> Currently the time_mode/owner has 8 combinations, some of them are not
>> valid.
>> Could you help to verify what's the expected behavior in each
>> combinations?
>>
>> Mode      | Owner | Set BMC Time  | Set Host Time
>> ------------- | --------- | ----------------------- |
>> -------------------------------------
>> NTP        | BMC   | Not allowed      | Not allowed
>> NTP        | HOST | Invalid case     | Invalid case
>> NTP        | SPLIT | Not allowed      | OK, and just save offset
>> NTP        | BOTH | Now allowed     | OK, and set time to BMC
>> MANUAL | BMC   | OK                  | Not allowed
>> MANUAL | HOST | Not allowed      | OK, and set time to BMC
>> MANUAL | SPLIT | OK                  | OK, and just save offset
>> MANUAL | BOTH | OK                  | OK, and set time to BMC
>>
>> If my understanding is correct, then we can see:
>> * NTP/HOST is invalid case, such case shall not exist, the current
>> implementation
>>    either hacks settingsd to change the mode to MANUAL, or just behaves
>> like MANUAL;
>> * NTP/BOTH has the same behavior as MANULA/HOST, they can be merged.
>>
>> That's why I propose to combine the settings to:
>> * NTP-BMC
>> * NTP-SPLIT
>> * NTP-BOTH (or MANUAL-HOST)
>> * MANUAL-BMC
>> * MANUAL-SPLIT
>> * MANUAL-BOTH
>>
>> Then we don't have to "hack" or "disagree" with settingsd, and the
>> logic could be
>> a little simpler.
>>
>>>
>>>>>> 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
>>>>
>>>> _______________________________________________
>>>> openbmc mailing list
>>>> openbmc@lists.ozlabs.org
>>>> https://lists.ozlabs.org/listinfo/openbmc
>>>
>>>
>

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

[vishwa]

[-- Attachment #1: Type: text/plain, Size: 2997 bytes --]

On 01/18/2017 08:14 PM, Patrick Williams 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@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.
>
>>>> 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.
There was a concern from test team on 'why do we need to defer applying 
any settings that are used by time manager'.

I thought and partially agree to their concern. For changing the owner 
*away from SPLIT*, we can actually consume that readily. Its only the 
change * TO SPLIT* that needs to delayed until off.

Also, the time_mode changes can be applied readily ( although time 
manager may not like HOST and NTP and in that case, its in pending state 
anyway ).


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


The current implementation only sets the values based on what was set 
prior to reset.
It does that by consuming the saved data in /var/lib/obmc/saved_*. So 
there is not an issue there.


>
>> What do you mean by "add them to the journal"?
> Use phosphor-logging.
>
>
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc


[-- Attachment #2: Type: text/html, Size: 4549 bytes --]

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

[vishwa]

[-- Attachment #1: Type: text/plain, Size: 2993 bytes --]

On 01/18/2017 08:14 PM, Patrick Williams 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@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.
>
>>>> 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.
There was a concern from test team on 'why do we need to defer applying 
any settings that are used by time manager'.

I thought and partially agree to their concern. For changing the owner 
*away from SPLIT*, we can actually consume that readily. Its only the 
change * TO SPLIT* that needs to delayed until off.

Also, the time_mode changes can be applied readily ( although time 
manager may not like HOST and NTP and in that case, its in pending state 
anyway ).


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


The current implementation only sets the values based on what was set 
prior to reset.
It does that by consuming the saved data in /var/lib/obmc/saved_*. So 
there is not an issue there.


>> What do you mean by "add them to the journal"?
> Use phosphor-logging.
>
>
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc


[-- Attachment #2: Type: text/html, Size: 4515 bytes --]

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

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

On Thu, Jan 19, 2017 at 03:37:34PM +0800, Mine wrote:
> Currently the time_mode/owner has 8 combinations, some of them are not valid.
> Could you help to verify what's the expected behavior in each combinations?
> 
> Mode      | Owner | Set BMC Time  | Set Host Time
> ------------- | --------- | ----------------------- |
> -------------------------------------
> NTP        | BMC   | Not allowed      | Not allowed
> NTP        | HOST | Invalid case     | Invalid case
> NTP        | SPLIT | Not allowed      | OK, and just save offset
> NTP        | BOTH | Not allowed     | OK, and set time to BMC
> MANUAL | BMC   | OK                  | Not allowed
> MANUAL | HOST | Not allowed      | OK, and set time to BMC
> MANUAL | SPLIT | OK                  | OK, and just save offset
> MANUAL | BOTH | OK                  | OK, and set time to BMC

Why is 'Set BMC Time' not allowed in any NTP mode in your table?  There
is no reason to stop 'Set BMC Time' except in the NTP/HOST case.

> If my understanding is correct, then we can see:
> * NTP/HOST is invalid case, such case shall not exist, the current
> implementation
>   either hacks settingsd to change the mode to MANUAL, or just behaves
> like MANUAL;

When the 'host' owns the clock, 'ntp' vs 'manual' is irrelevant.

> * NTP/BOTH has the same behavior as MANULA/HOST, they can be merged.

No, these are significantly different.  NTP/BOTH means that we run the
NTP client on the BMC but allow either side to set the time.
MANUAL/HOST means we do not run the NTP client and the BMC is prohibited
from setting the time in any way.

> That's why I propose to combine the settings to:
> * NTP-BMC
> * NTP-SPLIT
> * NTP-BOTH (or MANUAL-HOST)
> * MANUAL-BMC
> * MANUAL-SPLIT
> * MANUAL-BOTH
> 
> Then we don't have to "hack" or "disagree" with settingsd, and the
> logic could be
> a little simpler.

1. NTP vs Manual controls if the NTP-client is running.
2. Owner controls who is allowed to set the time.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2163 bytes --]

On Thu, Jan 19, 2017 at 05:48:23PM +0800, Mine wrote:
> Btw, is there any specific reason why the time mode/owner is only changed
> when host is off?

Yes, I think you're missing the point of having a split clock at all.

Typically you think of a machine as being "owned" by a single party.
They decide if they want to run NTP on the host or run NTP on the BMC
and they point at an NTP server they trust and all is fine.

There is another case of a machine being "owned" by one party and "used"
(leased) by another party.  Typically the owner maintains access to the
BMC and the lessee maintains access to the Host.  Neither side necessary
trusts the other side to keep the time correct, so we have the "split"
mode.

(There are potential security issues with having an incorrect timebase.
A clear example is that your OS will accept expired SSL certificates if
you tell it the wrong year.)

If the machine owner sets the clock to "NTP/SPLIT", they no longer care
what the time of the host is.  They point the NTP config at their own
NTP server and time, from a BMC perspective, is "correct".  At that
point the machine lessee can:

    1. Ask for 3rd party attestation records from the BMC to confirm what
       level of code the BMC is running.  (TPM support, not implemented
       now).
    2. Audit the code on Github to understand how the modes / models are
       implemented and what the system will do as a result.
    3. Query the BMC on boot to determine what mode it is currently
       operating in.

At this point the lessee:
 
    * Can trust that the machine is running a non-tampered version of
      code that behaves like our reference implementation.
    * Knows from our reference implementation that the 'host time' is
      maintained in a secure manner so that if the "owner's" NTP server
      were compromised, the 'host time' is still correct.

If the BMC were allowed to change the mode while the host is running (#3
is no longer accurate), then it is impossible for the host to trust the
time.  An attacker could simply change the mode after the host as
queried.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

On Thu, Jan 19, 2017 at 05:54:02PM +0530, vishwa wrote:
 
> I thought and partially agree to their concern. For changing the owner 
> *away from SPLIT*, we can actually consume that readily. Its only the 
> change * TO SPLIT* that needs to delayed until off.

Actually the opposite.  You can safely switch to split while running,
but switch from split or host needs to be deferred.

> >      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.
> 
> 
> The current implementation only sets the values based on what was set 
> prior to reset.
> It does that by consuming the saved data in /var/lib/obmc/saved_*. So 
> there is not an issue there.

Sounds good.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Mine]

On Sat, Jan 21, 2017 at 3:08 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
> On Thu, Jan 19, 2017 at 03:37:34PM +0800, Mine wrote:
>> Currently the time_mode/owner has 8 combinations, some of them are not valid.
>> Could you help to verify what's the expected behavior in each combinations?
>>
>> Mode      | Owner | Set BMC Time  | Set Host Time
>> ------------- | --------- | ----------------------- |
>> -------------------------------------
>> NTP        | BMC   | Not allowed      | Not allowed
>> NTP        | HOST | Invalid case     | Invalid case
>> NTP        | SPLIT | Not allowed      | OK, and just save offset
>> NTP        | BOTH | Not allowed     | OK, and set time to BMC
>> MANUAL | BMC   | OK                  | Not allowed
>> MANUAL | HOST | Not allowed      | OK, and set time to BMC
>> MANUAL | SPLIT | OK                  | OK, and just save offset
>> MANUAL | BOTH | OK                  | OK, and set time to BMC
>
> Why is 'Set BMC Time' not allowed in any NTP mode in your table?  There
> is no reason to stop 'Set BMC Time' except in the NTP/HOST case.

The current implementation uses this logic. (time-manager.cpp:300)
Actually the above table is the summary of the current implementation's logic.

>
>> If my understanding is correct, then we can see:
>> * NTP/HOST is invalid case, such case shall not exist, the current
>> implementation
>>   either hacks settingsd to change the mode to MANUAL, or just behaves
>> like MANUAL;
>
> When the 'host' owns the clock, 'ntp' vs 'manual' is irrelevant.

OK

>
>> * NTP/BOTH has the same behavior as MANULA/HOST, they can be merged.
>
> No, these are significantly different.  NTP/BOTH means that we run the
> NTP client on the BMC but allow either side to set the time.
> MANUAL/HOST means we do not run the NTP client and the BMC is prohibited
> from setting the time in any way.

Yup, with or without NTP means BMC will run or disable NTP client.
But the "same behavior" I mean here is to refer that if set BMC/HOST
time is allowed
or not, the current logic:
NTP/BOTH: setting BMC time is not allowed (you mentioned above that this shall
be allowed, but the current implementation does not allow it); setting
HOST time is allowed.
MANUAL/HOST: setting BMC time is not allowed, setting HOST time is allowed.
They are the same.

But if your suggestion that in NTP/BOTH setting BMC is allowed, then
the behavior will be
different.

>
>> That's why I propose to combine the settings to:
>> * NTP-BMC
>> * NTP-SPLIT
>> * NTP-BOTH (or MANUAL-HOST)
>> * MANUAL-BMC
>> * MANUAL-SPLIT
>> * MANUAL-BOTH
>>
>> Then we don't have to "hack" or "disagree" with settingsd, and the
>> logic could be
>> a little simpler.
>
> 1. NTP vs Manual controls if the NTP-client is running.
> 2. Owner controls who is allowed to set the time.

Sure. Let's keep the current mode and time separately.
But please confirm the behavior of setting BCM/HOST time in each
combination (the table)

>
> --
> Patrick Williams