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
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 at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/openbmc
>
>
More information about the openbmc
mailing list