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?