Stop creating the serialization code for the non inventory interfaces

[ratagupt]

As part of compilation the inventory manager, There is a build script named pimgen.py which generates the serialization code for the inventory manager.

Presently there is not filtering for the interface so that it generates the serialization code for other D-Bus interfaces which is not inventory related.

Till now this issue was not popped up as we had the interfaces which has the properties of the simple type, so silently inventory manager was producing this code and there was no impact as this code was not used.

Following gerrit commit introduces dict type and the inventory manager starts failing.

https://gerrit.openbmc-project.xyz/#/c/openbmc/phosphor-dbus-interfaces/+/15177/5/xyz/openbmc_project/Certs/Certificate.interface.yaml.

It was discussed in the design meeting that we can put the filter for the desired inventory interfaces.

[ratagupt]

    def get_interfaces(targetdir):
        '''Scan the interfaces directory for interfaces that PIM can create.'''

The above function in the pimgen.py fetches all the interfaces.

[ratagupt]

Assign it to "Jinu Thomas"

[jinuthomas]

started to work on a white list for inventory but seems the list is not correct, will need to look deeper on the required list

[bradbishop]

On Tue, Feb 05, 2019 at 06:54:20PM -0800, Jinu Joy Thomas wrote:

started to work on a white list for inventory but seems the list is not correct, will need to look deeper on the required list

A white list? Wouldn't it just be checking that xyz.openbmc_project.Inventory.Item is a substring of the full interface name?

[jinuthomas]

I see three options

Option 1: add a blacklist assume to bypass Cert for the time being. Pros:

• Doesn't break anything currently working.
• Risk free fix Cons
• each time someone creates a new interface, it has to put here to bypass, but if they do not know that they have to so this, then it would duplicate code.
• bad design as everyone needs to know this design
• Old duplicates remain

Option 2: add a whitelist assume to only use ones that are needed. Pros:

• New interface developers need not add anything, unless required.
• Inventory developer if needed adds the extra yaml outside Inventory into the list and it will be used.
• All duplicates are removed.
• Better design Cons
• To use interface need to have the extra yamls mentioned in code if need which is outside Inventory Path.
• Not sure if it will ripple the change

Option 3: Need to have the path of the yaml used be fixed and the Makefile only pulls the required yaml files into the directory. Pros:

• New interface developers need not add anything, unless required.
• No code to add into the base code to use the required yaml.
• All duplicates are removed.
• Best design. Cons
• Need the code design expert knowledge to do it.

@bradbishop : I also have gone by the Option 2. similar to what you have mentioned and have the code done , it is not just Inventory but we need some extra yamls and the Example interface. There may be more Cons and Pros for each as i am new to this is all that comes immediately to mind.

The issue i have is

• I do not know if the change will have any effect on something else like openpower ?
• Would you think if Option 3 will be a better approach so that the makefile /recipes handles this.
• How do i know that this change will not have a ripple effect, ask the test team to test it ?

@bradbishop could you give me your opinion on this , and can i push the code for review, also who all should be added for the review, how do i reach this conclusion.

fyi @ojayanth : i am planning to use the Option 2 currently and propose to have Option 3 in future

[bradbishop]

I do not know if the change will have any effect on something else like openpower ?

It won't. These lines are in the API spec for xyz.openbmc_project.Inventory.Item: description: > Implement to provide basic item attributes. Required by all objects within the inventory namespace. So you can't have objects in the inventory namespace that don't implement xyz.openbmc_project.Inventory.Item.

Would you think if Option 3 will be a better approach so that the makefile /recipes handles this.

Yes. Maintaining a list...it won't get maintained.

How do i know that this change will not have a ripple effect, ask the test team to test it ?

Unfortunately we don't have any unit tests for phosphor-inventory-manager. We do have openbmc-test-automation - you could run that test suite. @gkeishin can help you.

A manual test would be good too: A manual test would be:

1 - boot the system without your code 2 - dump the inventory namespace using the REST API. 3 - load your code 4 - repeat 1 & 2 with your code 5 - check that there are no differences in the inventory namespace dumps

fyi @ojayanth : i am planning to use the Option 2 currently and propose to have Option 3 in future

This suggests that we are going to add technical debt. We don't do that in OpenBMC - we just identify the correct way from the start, and implement that.

[jinuthomas]

@bradbishop thanks will go ahead and push the code for review

[msbarth]

May I ask why inventory mgr was not enhanced to handle dict type?

I dont think looking at the Inventory.Item interface should strictly be used and instead have some sort of config for what is persisted to what is not (i.e. the white list mentioned?). One example is the CoolingType must be persisted for inventory manager to properly remove or keep the fan1 object in inventory between water and air cooled wspoons. If this is not persisted, the startup of inventory mgr will not be able to correctly keep or remove fan1 since the CoolingType would not be set in inventory until after the cooling-type app starts (which inventory has to be started already).

[bradbishop]

On Wed, Feb 06, 2019 at 08:22:27AM -0800, Matthew Barth wrote:

May I ask why inventory mgr was not enhanced to handle dict type? I'm guessing because noone needed it. Otherwise it would have been implemented, right?

I dont think looking at the Inventory.Item interface What I thought I said was xyz.openbmc_project.Inventory.Item should be a substring.

should strictly be used and instead have some sort of config for what is persisted to what is not (i.e. the white list mentioned?). One example is the CoolingType must be persisted for inventory manager to properly remove or keep the fan1 object in inventory between water and air cooled wspoons. If this is not persisted, the startup of inventory mgr will not be able to correctly keep or remove fan1 since the CoolingType would not be set in inventory until after the cooling-type app starts (which inventory has to be started already). I didn't realize Decorator was a peer to (rather than a child of) Item. In that case the substring match can just be xyz.openbmc_project.Inventory rather than xyz.openbmc_project.Inventory.Item. Does that work Matt?

[msbarth]

I'm guessing because noone needed it. Otherwise it would have been implemented, right?

Sorry, I'm referring to why is it not being implemented into inventory manager to resolve this issue of handling a dict type in inventory now instead of removing persistence of objects?

What I thought I said was xyz.openbmc_project.Inventory.Item should be a substring.

Ok

I didn't realize Decorator was a peer to (rather than a child of) Item. In that case the substring match can just be xyz.openbmc_project.Inventory rather than xyz.openbmc_project.Inventory.Item. Does that work Matt?

My mention of the Decorator.CoolingType was just an example. Another example would be the State.Decorator.OperationalStatus that is also on the fan rotors/enclosures in inventory. If that is no longer persisted, inventory wouldnt show if fans are functional or not at any point inventory mgr is restarted until the fan monitor app updates it. These would then be lost on the webpage? or any tool that queries these like XCat, right?

Maybe a collection of what interfaces are being supported by inventory mgr should be gathered and determined whether what could be stored on an object under that interface should be persisted or is ok to not be?

[bradbishop]

On Wed, Feb 06, 2019 at 12:10:16PM -0800, Matthew Barth wrote:

Sorry, I'm referring to why is it not being implemented into inventory manager to resolve this issue of handling a dict type in inventory now instead of removing persistence of objects? I get it now. That is a good question. It probably isn't easy to implement dictionary because that isn't a concrete type (dictionary of whats?) but we should at least consider how it would be done as one of the options here.

My mention of the Decorator.CoolingType was just an example. Another example would be the State.Decorator.OperationalStatus that is also on the fan rotors/enclosures in inventory. If that is no longer persisted, inventory could show that fans are nonfunctional at any point inventory mgr is restarted until the fan monitor app updates it. This would introduce error logs being created for these fans as well. Yes, good call...I overlooked this. Glad you were watching.

Maybe a collection of what interfaces are being supported by inventory mgr should be gathered and determined whether what could be stored on an object under that interface should be persisted or is ok to not be? Having trouble parsing this...

[bradbishop]

@ratagupt What interface are we adding with a dictionary property?

[msbarth]

Having trouble parsing this...

Hmm, at one point I thought there was a list of supported interfaces we allowed on inventory but it looks like it really accepts any since they are pulled in at compile time. I guess this strengthens my feeling in investigating how we could implement/add dict type support (maybe somehow related to the interface that contains the dict type?).

[msbarth]

Just thinking out loud, but is it possible to pull in the dict type from the interface object it extends?

[jinuthomas]

i have currently coded it to use the Inventory interfaces as well as the Example interface and required ones like (UUID,OperationalStatus,Version) , this is a list which can be extended as needed. Coding to support dict can be done but would be better to have it done when we need it only and not by default currently, so that testing and usage can be confirmed.

[bradbishop]

On Wed, Feb 06, 2019 at 12:51:43PM -0800, Matthew Barth wrote:

Just thinking out loud, but is it possible to pull in the dict type from the interface object it extends? On the surface that seems like a good starting point for an investigation.

[bradbishop]

On Wed, Feb 06, 2019 at 09:07:39PM -0800, Jinu Joy Thomas wrote:

i have currently coded it to use the Inventory interfaces as well as the Example interface and required ones like (UUID,OperationalStatus,Version) , this is a list which can be extended as needed. Coding to support dict can be done but would be better to have it done when we need it only and not by default currently, so that testing and usage can be confirmed. My concern with this is it might break people using PIM when they upgrade. We can't possibly know what interfaces everyone is sending to PIM ("the required ones"). This is a key point for open source development. You can't focus on your use-case only. You have to consider all possible use cases.

[bradbishop]

@ratagupt What interface are we adding with a dictionary property?

I stopped being lazy and looked this up myself. It is the subject and issuer properties of the new certificate API. Here is an example of what these dicts will store.

C=BE, O=GlobalSign nv-sa, CN=GlobalSign Organization Validation CA - SHA256 - G2

[bradbishop]

After thinking about this some more, I can probably live with a whitelist. I do agree with not adding function until it is actually needed and it does let us optimize the flash footprint. We'll have to make sure this is well documented in case people have any trouble when they upgrade.

[dkodihal]

Fixing the problem with the dict type seems like the better option, since that's the problem here. PIM otherwise allows apps to make D-Bus objects out of any D-Bus interfaces.

[ratagupt]

My vote is with whitelist approach as here we don't want to persist the certificate/network or other interfaces which are not used. Making a list should not be tough as we can dump the inventory objects and can find all the interfaces. In this case, we don't want to make the cert object via inventory manager so extending the support fo dict is not required. However, we can do it in the future if it is needed.

[msbarth]

A couple other thoughts if adding dict type support at this time isnt favorable would be:

1. Have inventory manager dynamically/actively blacklist those interfaces containing a dict type (would need to be documented and have an issue created on inventory mgr's backlog to support dict types)

2. Enhance inventory manager to be configurable by platform on the interfaces it would support (would still need an issue created on its backlog for interface with dict types, but would narrow down the interfaces it uses)

I guess I would like to see the second option implemented here which would reduce inventory mgr's overhead and sounds like it would handle correcting the issue here as well?

[jinuthomas]

@dkodihal ^^^ could you look at the above and comment please

[dkodihal]

@jinuthomas Is supporting the dict type very hard to do? I think we should stick to that, everything else seems like a work-around. This script runs at build time, so "dumping inventory objects", as suggested, is not an option.