Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add PoolInfo to implement discovery-time pool-specific logic #213

Closed
wants to merge 1 commit into from
Closed

Add PoolInfo to implement discovery-time pool-specific logic #213

wants to merge 1 commit into from

Conversation

amorenoz
Copy link
Contributor

This PR is intended to demonstrate #212 and propose a possible implementation (still work in progress).

Firstly, it splits the current PciNetDevice interface into two separate interfaces:

  • PciNetDevice for low level device information (used for filtering)
  • PoolDevice for providing the information that the ResourcePool needs (i.e: DeviceSpecs, Mounts, Envs, etc)

That way, InfoProviders could be selected based on ResourcePool information.

This is part of my efforts to cleanly implement vdpa support for the Device Plugin.

It can be observed how, with this preliminary rework, an experimental vdpa support can be implemented with little change in existing DP code:
https://github.com/amorenoz/sriov-network-device-plugin/tree/vdpa_pooldev

@amorenoz amorenoz mentioned this pull request Mar 11, 2020
Closed
@lennybe
Copy link

lennybe commented Mar 12, 2020

/retest

@amorenoz amorenoz mentioned this pull request Mar 16, 2020
Merged
@amorenoz
Create new PoolInfo interface
fd4a9be 
Currently PciDevice and its deviceType-specific derived versions
contain two types of information
- Low level device-specific information (such VF and PF names, PCI
  vendor, etc) used by the DeviceSelector implementations to filter
  devices
- High level API-centric information (mounts, env, deviceSpecs) used by
  the ResourcePool to respond to the API calls

This is fine as long as the Pool-specific information can derived solely
from the PCI information.

However, if a certain PCI device can be consumed in different ways
(like, for instance, a device compatible with DPDK and with DPDK-vDPA)
some configuration flag must be used to select which way the device
should be exposed. Unfortunately, the ResourceConfig is only available
when the ResorcePools are created (after the devices are).

In order to make that possible, split the two types of imformation into
two different interfaces (introducing PoolInfo). That way, the
ResourcePool-specific data can be generated taking into consideration
it's configuration.

The first benefit of this cleanup is that now the Rdma deviceSpec can be
appended to the rest of the device's deviceSpecs at pool-creation time
and only if the pool is properly configured, thus cleaning up the
resourcePool runtime code.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
@amorenoz amorenoz changed the title Add PoolDevice to implement discovery-time pool-specific logic Add PoolInfo to implement discovery-time pool-specific logic May 7, 2020
@amorenoz
Copy link
Contributor Author

amorenoz commented May 7, 2020

I'm waiting for some feedback regarding the high level design before starting to work on the tests. Could you ptal @ahalim-intel @zshi-redhat ?

@zshi-redhat zshi-redhat mentioned this pull request May 11, 2020
Merged
@zshi-redhat
Copy link
Collaborator

After adding poolInfo, we will have two places to get device related info, one is in discover phase where ResourceConfig is not passed in so we only do device filter based on device class, the other is when generating ResourcePool where ResourceConfig is passed in so advanced filtering is possible, for example, getting rdma spec only if isRdma is configured (see #226).

Do we have a clear definition in mind on when will each path be used? or shall we consider combining them to one, for example, use poolInfo for device discover?

@amorenoz
Copy link
Contributor Author

@zshi-redhat You're right, with this PR isRdma can be reinterpreted as a "Pool Flag" instead of a "pciNetDevice Filter", their correspondent meanings being:

  • Pool Flag: If the flag is set, this pool will provide Rdma deviceSpecs. If any of the filtered devices does not have valid Rdma device specifications, the pool creation will fail.
  • NetDevice Filter: RdmaSpecs will be retrieved from all NetDevices (it's a DeviceType property). If the flag is set, only devices with valid RdmaSpecs will be added to the pool.

In general, I think that all the properties that are not valid across all devices within a DeviceType could be interpreted as "Pool Flags" and only be retrieved if the pool flag is specified. With the future introduction of vDPA, both "isVdpa" and "isRdma" would fit in this definition since they are mutually exclusive. (Note: I don't mean to say isRdma devices cannot be exposed via vDPA, which they surely can. But the rdma devices will be handled by the vDPA Framework and only a vhost socket will be exposed via DevicePlugin API).

Regarding the other alternative you propose, IIUC it would entail:

  • Having a DeviceProvider per ResourceConfig (instead of per DeviceType)
  • Postpone the call to DeviceProvider.AddTargetDevices() to ResourceManager.initServers() so that the ResourceConfig is available.
  • Perform device discovery + filtering + pool information gathering with the ResourceConfig handy

I think your proposal would enable the usecase being tackled here (Same device info, different pool info). We would basically implement all the logic inside NewPciNetDevice(), that would now accept a ResourceConfig as parameter. We would still be combining both device-specific and pool-specific information in the same structure (hence I still think the API split proposed is technically valid) but it would make a smaller PR to enable this usecase.

@amorenoz amorenoz mentioned this pull request May 13, 2020
Merged
@zshi-redhat
Copy link
Collaborator

I think your proposal would enable the usecase being tackled here (Same device info, different pool info). We would basically implement all the logic inside NewPciNetDevice(), that would now accept a ResourceConfig as parameter. We would still be combining both device-specific and pool-specific information in the same structure (hence I still think the API split proposed is technically valid) but it would make a smaller PR to enable this usecase.

makes sense to me, let's make the small change.

@amorenoz amorenoz mentioned this pull request May 20, 2020
Merged
@ahalimx86
Copy link
Collaborator

@amorenoz please close this PR if #231 satisfies this usecase

@amorenoz
Copy link
Contributor Author

amorenoz commented Jul 2, 2020

@amorenoz please close this PR if #231 satisfies this usecase

Right. It does. Thanks

@amorenoz amorenoz closed this Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amorenoz @lennybe @zshi-redhat @ahalimx86