<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 19, 2022 at 8:14 PM Lei Yu <<a href="mailto:yulei.sh@bytedance.com">yulei.sh@bytedance.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This email is to describe an issue in mapper that the<br>
GetSubTree/GetSubTreePaths could return incomplete results when it's<br>
doing introspect.<br>
<br>
Steps to reproduce the issue:<br>
1. Configure phosphor-logging to get max 1000 entries. (with<br>
-Derror_info_cap=1000 meson option)<br>
2. Create 1000 logging entries.<br>
3. Call GetSubTreePaths and make sure it gets the correct 1000 entries:<br>
    # busctl call "xyz.openbmc_project.ObjectMapper"<br>
"/xyz/openbmc_project/object_mapper"<br>
"xyz.openbmc_project.ObjectMapper" GetSubTreePaths sias<br>
/xyz/openbmc_project/logging/entry 0 1<br>
xyz.openbmc_project.Logging.Entry | awk '{print $2;}'<br>
    1000<br>
4. Restart logging service<br>
    # systemctl restart xyz.openbmc_project.Logging.service<br>
5. After the service is restarted, call GetSubTreePaths for multiple<br>
times in the short time (e.g. within 10 seconds)<br>
    # busctl call "xyz.openbmc_project.ObjectMapper"<br>
"/xyz/openbmc_project/object_mapper"<br>
"xyz.openbmc_project.ObjectMapper" GetSubTreePaths sias<br>
/xyz/openbmc_project/loggiz.openbmc_project.Logging.Entry | awk<br>
'{print $2;}'<br>
    47<br>
    # busctl call "xyz.openbmc_project.ObjectMapper"<br>
"/xyz/openbmc_project/object_mapper"<br>
"xyz.openbmc_project.ObjectMapper" GetSubTreePaths sias<br>
/xyz/openbmc_project/loggiz.openbmc_project.Logging.Entry | awk<br>
'{print $2;}'<br>
    375<br>
    # busctl call "xyz.openbmc_project.ObjectMapper"<br>
"/xyz/openbmc_project/object_mapper"<br>
"xyz.openbmc_project.ObjectMapper" GetSubTreePaths sias<br>
/xyz/openbmc_project/loggiz.openbmc_project.Logging.Entry | awk<br>
'{print $2;}'<br>
    851<br>
    # busctl call "xyz.openbmc_project.ObjectMapper"<br>
"/xyz/openbmc_project/object_mapper"<br>
"xyz.openbmc_project.ObjectMapper" GetSubTreePaths sias<br>
/xyz/openbmc_project/loggiz.openbmc_project.Logging.Entry | awk<br>
'{print $2;}'<br>
    1000<br>
<br>
We can see that the result of GetSubTreePaths is increasing until it gets 1000.<br>
This actually happens when mapper is doing introspect of the logging<br>
service, and getting more and more objects.</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The above "incomplete" behavior will impact the logic that depends on<br>
the result of GetSubTreePaths.<br>
E.g. in ipmid, the "cached SEL" feature depends on the reliable result<br>
of GetSubTreePath, to get the number of current logging entries.</blockquote><div><br></div><div>I'm not really following why this is a problem, and sounds like a bug in the cached SEL feature.  Dbus objects can get added or removed at any time, including logging objects, if the cached sel feature isn't handling all the added/removed signals as it should, that really should be fixed.  Can you go into more detail about what this actually causes?  Is it that logging objects are getting added out of order?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> If<br>
it's not correct, ipmid will not know the "missed" entries.<br></blockquote><div><br></div><div>What is a "missed" entry in this context?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
The question is, should we make sure mapper returns the "stable"<br>
result in the above case?<br></blockquote><div><br></div><div>I don't think so, at least, when it was built, it wasn't designed to return atomically created results, but given that ANY object on dbus can be added or removed at any time, it's not clear why this is a problem in the mapper itself.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
When it's doing introspect of a service (e.g. nameOwnerChanged), it<br>
could throw if the service is not fully introspected, and only return<br>
the "correct" result after the service is fully introspected. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
If mapper could not guarantee the stable result, the service calling<br>
mapper will have to add more complex logic to make sure it gets the<br>
"full and correct" result.<br></blockquote><div><br></div><div><div>I left comments on your code review as well, but please don't do this.  This is going to force retry code into EVERY service that relies on the mapper, at EVERY mapper call site.  If the current behavior is truly a problem, I would much rather the mapper support multi-versioned atomically create copies of the dbus-map, and have the mapper expose the "reliable" interface, rather than spreading that throughout the system.  For context, bmcweb alone has dozens of call sites to the mapper that would need to be fixed if it were made to be possible for it to fail in this way.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-- <br>
BRs,<br>
Lei YU<br>
</blockquote></div></div>