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

'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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc