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

Mine mine260309 at gmail.com
Thu Jan 19 18:37:34 AEDT 2017

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

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

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

More information about the openbmc mailing list