API preferred allocation: GetPreferredAllocation #267

zshi-redhat · 2020-09-08T04:25:19Z

update kubelet dependency to 1.19.0 which supports GetPreferredAllocation API
add a cli param called --allocate-policy with two options: empty or concentrated policy.
introduce Allocator interface and include it in each resource server
add GetPreferredAllocation API function call in resource server.

Fixes #255

zshi-redhat · 2020-09-08T04:30:39Z

Concentrate used in the PR may not be a good name for the allocating policy (use one PF resource until exhausted), feel free to propose other names, I will update the patch accordingly.

zshi-redhat · 2020-09-10T01:24:07Z

/cc @ahalim-intel @martinkennelly @adrianchiris PTAL.

zshi-redhat · 2020-09-10T01:25:16Z

This RP was tested against 19.0 kubernetes deployment.

amorenoz · 2020-09-10T07:00:57Z

@zshi-redhat Does it make sense to have a different allocating policy per resource pool?

zshi-redhat · 2020-09-10T07:38:02Z

@zshi-redhat Does it make sense to have a different allocating policy per resource pool?

@amorenoz good question!
I considered to make the allocate-policy as a per-resource configuration, instead of a cli-param. I think it makes sense.
The reason I didn't implement it that way is because I'm not sure if user prefers to have all the resources be allocated with same policy or they'd like to use different policies for resources.

amorenoz · 2020-09-10T07:55:01Z

@zshi-redhat Does it make sense to have a different allocating policy per resource pool?

@amorenoz good question!
I considered to make the allocate-policy as a per-resource configuration, instead of a cli-param. I think it makes sense.
The reason I didn't implement it that way is because I'm not sure if user prefers to have all the resources be allocated with same policy or they'd like to use different policies for resources.

Maybe @sseetharaman6 can comment?

We could have both: A default policy (cli) and a per-resource one that would override the former.

ahalimx86 · 2020-09-10T16:05:10Z

Thanks for this feature. So, the current implementation of ConcentrateAllocator is based on the order of pci addresses. What other allocation policy we should implement?

zshi-redhat · 2020-09-11T01:05:45Z

Thanks for this feature. So, the current implementation of ConcentrateAllocator is based on the order of pci addresses. What other allocation policy we should implement?

There were two allocation policys I'd like to implement initially:

concentrated (allocating VF from one PF until it's exhausted if multiple PFs are available in one resource pool)
distributed (VF allocation is evenly distributed among PFs if multiple PFs are available in one resource pool)

For the first one, we have an issue associated with it.
For the second one, I'm not sure if it's really needed at the moment, I'd like to see if anyone has any requirement of using the distributed policy. Another reason I didn't implement the distributed policy is since kubelet randomly chooses which device to allocate, we probably can just rely on it (although it's not guaranteed to be evenly distributed) instead of implementing our own.

I think the main usage of this API is to recommend the device affinity to kubelet when allocating more than one device. I'd like to see if there is any use case in networking or accelerator area that requires this kind of device affinity. one possible example might be: if

an accelerator device contains both a network function and accelerator function
multiple such devices are available on one node
network functions from multiple devices are exposed as a single resource pool A
accelerator functions from multiple devices are exposed as a single resource pool B

we probably want to allocate network function(pool A) and accelerator function(pool B) from the same device for affinity purpose.

adrianchiris · 2020-09-13T10:59:39Z

Concentrate used in the PR may not be a good name for the allocating policy (use one PF resource until exhausted), feel free to propose other names, I will update the patch accordingly.

Some policy naming ideas that came to mind :)

Concentrate -> Packed : VFs are preferred to be allocated in a Packed manner under the same PF
Distributed -> Balanced : VFs are preferred to be allocated in a Balanced manner across PFs of the same resource

zshi-redhat · 2020-09-14T02:05:03Z

Concentrate used in the PR may not be a good name for the allocating policy (use one PF resource until exhausted), feel free to propose other names, I will update the patch accordingly.

Some policy naming ideas that came to mind :)

Concentrate -> Packed : VFs are preferred to be allocated in a Packed manner under the same PF
Distributed -> Balanced : VFs are preferred to be allocated in a Balanced manner across PFs of the same resource

Renamed concentrated to packed.
Distributed is not yet included in this PR, I will use the new Balanced name when implementing it.

sseetharaman6 · 2020-09-24T19:35:48Z

Thanks for this PR @zshi-redhat .
In the original RFE I filed, I only have the need for a common allocatePolicy . However, per-resource allocate-policy is definitely nice to have. I can see how it will be helpful, but maybe instead of cli-param, this can come from /etc/pcidp/config.json as well?
Also I wanted to clarify thatGetPreferredAllocate is only compatible with v1.19.0 and later kubernetes deployments .. so in a cluster running an earlier version, this would be no-op.

zshi-redhat · 2020-09-28T04:11:31Z

Thanks for this PR @zshi-redhat .
In the original RFE I filed, I only have the need for a common allocatePolicy . However, per-resource allocate-policy is definitely nice to have. I can see how it will be helpful, but maybe instead of cli-param, this can come from /etc/pcidp/config.json as well?

Changed the allocatePolicy as a per resource pool config.

Also I wanted to clarify thatGetPreferredAllocate is only compatible with v1.19.0 and later kubernetes deployments .. so in a cluster running an earlier version, this would be no-op.

Yes, it shall only works with v1.19.0 and onwards, did you see any error when running this sriovdp with old k8s version?

martinkennelly · 2020-09-14T12:39:07Z

pkg/factory/factory.go

+	case "packed":
+		return resources.NewPackedAllocator()
+	default:
+		return nil


What happens if there's a misconfig or unsupported policy. Can we raise an error here? I tested this and if we specify a policy that does not match any case, then it is silently ignored.

martinkennelly · 2020-10-19T13:37:53Z

pkg/types/types.go

@@ -77,6 +77,7 @@ type ResourceConfig struct {
 	DeviceType     DeviceType       `json:"deviceType,omitempty"`
 	Selectors      *json.RawMessage `json:"selectors,omitempty"`
 	SelectorObj    interface{}
+	AllocatePolicy string `json:"allocatePolicy,omitempty"`


nit: format of json meta data is off.

martinkennelly · 2020-10-22T15:11:42Z

I tested this patch on ver. 1.19.3 of Kubernetes. I tested it on multiple VFs with the packed option. It worked as expected enabled and disabled. Thanks again @zshi-redhat for this.

martinkennelly · 2020-11-23T12:08:55Z

@zshi-redhat Do you think adding documentation for this would be good too to advertise this feature?

zshi-redhat · 2020-11-24T10:34:32Z

@zshi-redhat Do you think adding documentation for this would be good too to advertise this feature?

Yes, I will add doc for the API change, Thanks Martin!

martinkennelly · 2020-12-15T16:36:17Z

@zshi-redhat do you envision we also add the Balanced functionality to this patch? If you need help, let me know.

zshi-redhat · 2020-12-16T05:10:17Z

resolved merge conflict and updated the Readme.doc

zshi-redhat · 2020-12-16T05:18:02Z

@zshi-redhat do you envision we also add the Balanced functionality to this patch? If you need help, let me know.

@martinkennelly I thought about adding balanced functionality, the issue I can see is there seems no reliable way to balance device allocation from device plugin perspective, unless having device plugin record all the allocated/deallocated devices from kubelet. However, I'm unsure about adding the record in device plugin because there is no deallocate call from kubelet.

martinkennelly

LGTM.

zshi-redhat · 2021-01-29T01:49:17Z

@adrianchiris as we discussed in resource mgmt meeting (considering adding balanced policy later w/ solid use case), are you good with current packed allocation policy?

adrianchiris · 2021-01-31T09:52:13Z

I wonder if we can get away with no user-facing API changes for resourceList (for now) as we support only one policy.
and just have a cli flag that turns on "get preferred allocation" support globally like a feature gate (which can later be enabled by default).

Unless i missed it, I did not find a compelling reason in the conversation to justify (IMHO) having this configurable per resource at this time.

zshi-redhat · 2021-02-01T01:53:38Z

I wonder if we can get away with no user-facing API changes for resourceList (for now) as we support only one policy.
and just have a cli flag that turns on "get preferred allocation" support globally like a feature gate (which can later be enabled by default).

There is some difference between packed policy and no policy. packed always use the resources "in order" while no policy is random. I prefer not to change the default behaviour from random to packed.

Unless i missed it, I did not find a compelling reason in the conversation to justify (IMHO) having this configurable per resource at this time.

Looking at history, this PR was initially implemented the policy as cli cmd, later changed to per-resource pool configuration per suggestion. I think the use case is the same as user may want to allocate one resource in order, but not the others, because we support resourceList to contain multiple resourcePools in one device plugin instance.

adrianchiris · 2021-03-31T11:29:04Z

@zshi-redhat could you rebase this ? I assume this PR is still desired

zshi-redhat · 2021-04-01T08:39:37Z

@zshi-redhat could you rebase this ? I assume this PR is still desired

Rebased.

Signed-off-by: Zenghui Shi <[email protected]>

lgtm-com · 2021-04-12T01:17:24Z

This pull request fixes 1 alert when merging 78530f4 into 1a8c9df - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable

martinkennelly · 2021-07-22T09:10:06Z

@adrianchiris Could you find time to review this? It's rebased.

wattmto · 2022-08-17T06:39:10Z

I’m planning to migrate our in-house CNI plugin to SR-IOV CNI plugin with this device plugin. While evaluating this device plugin, I found the plugin doesn’t support allocating VFs from different PFs (balanced policy). The balanced policy allows us to assign multi PFs to a Pod to achieve higher bandwidth if the communication stack supports multi-rail network configuration.

I have two questions,

Do you agree with adding GetPreferredAllocation API to SR-IOV device plugin? If so, I’d like to prepare a new PR based on this work by rearranging and rebasing the commits. The new PR would be much smaller and easier to review.
Do you agree with adding a Balanced Policy to the SR-IOV device plugin?

superbrothers · 2022-08-25T09:15:29Z

@adrianchiris @martinkennelly @zshi-redhat
Hi guys! I would appreciate if you could comment on #267 (comment) if possible😊

adrianchiris · 2023-11-21T15:57:24Z

replaced by #443

zshi-redhat changed the title ~~WIP: Api preferred allocation: GetPreferredAllocation~~ WIP: API preferred allocation: GetPreferredAllocation Sep 8, 2020

zshi-redhat force-pushed the api-preferred-allocation branch from 2d51409 to 05aff6e Compare September 9, 2020 03:53

zshi-redhat mentioned this pull request Sep 10, 2020

Support for allocating all VFs from a single PF (bin packing) #255

Open

zshi-redhat changed the title ~~WIP: API preferred allocation: GetPreferredAllocation~~ API preferred allocation: GetPreferredAllocation Sep 28, 2020

martinkennelly reviewed Oct 21, 2020

View reviewed changes

zshi-redhat force-pushed the api-preferred-allocation branch from 0b5851e to 2ab2401 Compare December 16, 2020 05:09

martinkennelly approved these changes Jan 15, 2021

View reviewed changes

Support GetPreferredAllocation API

6b0d8d3

zshi-redhat added 4 commits April 1, 2021 16:34

Rename concentrated allocator policy to packed

1ca00d6

change allocate policy to a per resource pool config

e087e05

update kubelet to v0.19.0 in go.mod

be4e322

Add doc for allocatePolicy resourceConfig

e6376b3

zshi-redhat force-pushed the api-preferred-allocation branch from 2ab2401 to e6376b3 Compare April 1, 2021 08:38

Fix LGTM analysis: unused variable

78530f4

Signed-off-by: Zenghui Shi <[email protected]>

wattmto mentioned this pull request Aug 30, 2022

Support GetPreferredAllocation #443

Open

adrianchiris closed this Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API preferred allocation: GetPreferredAllocation #267

API preferred allocation: GetPreferredAllocation #267

zshi-redhat commented Sep 8, 2020 •

edited

Loading

zshi-redhat commented Sep 8, 2020

zshi-redhat commented Sep 10, 2020

zshi-redhat commented Sep 10, 2020

amorenoz commented Sep 10, 2020

zshi-redhat commented Sep 10, 2020

amorenoz commented Sep 10, 2020

ahalimx86 commented Sep 10, 2020

zshi-redhat commented Sep 11, 2020 •

edited

Loading

adrianchiris commented Sep 13, 2020

zshi-redhat commented Sep 14, 2020

sseetharaman6 commented Sep 24, 2020

zshi-redhat commented Sep 28, 2020

martinkennelly Sep 14, 2020

martinkennelly Oct 19, 2020

martinkennelly commented Oct 22, 2020

martinkennelly commented Nov 23, 2020

zshi-redhat commented Nov 24, 2020

martinkennelly commented Dec 15, 2020 •

edited

Loading

zshi-redhat commented Dec 16, 2020

zshi-redhat commented Dec 16, 2020

martinkennelly left a comment

zshi-redhat commented Jan 29, 2021

adrianchiris commented Jan 31, 2021 •

edited

Loading

zshi-redhat commented Feb 1, 2021

adrianchiris commented Mar 31, 2021

zshi-redhat commented Apr 1, 2021

lgtm-com bot commented Apr 12, 2021

martinkennelly commented Jul 22, 2021

wattmto commented Aug 17, 2022

superbrothers commented Aug 25, 2022

adrianchiris commented Nov 21, 2023

API preferred allocation: GetPreferredAllocation #267

API preferred allocation: GetPreferredAllocation #267

Conversation

zshi-redhat commented Sep 8, 2020 • edited Loading

zshi-redhat commented Sep 8, 2020

zshi-redhat commented Sep 10, 2020

zshi-redhat commented Sep 10, 2020

amorenoz commented Sep 10, 2020

zshi-redhat commented Sep 10, 2020

amorenoz commented Sep 10, 2020

ahalimx86 commented Sep 10, 2020

zshi-redhat commented Sep 11, 2020 • edited Loading

adrianchiris commented Sep 13, 2020

zshi-redhat commented Sep 14, 2020

sseetharaman6 commented Sep 24, 2020

zshi-redhat commented Sep 28, 2020

martinkennelly Sep 14, 2020

Choose a reason for hiding this comment

martinkennelly Oct 19, 2020

Choose a reason for hiding this comment

martinkennelly commented Oct 22, 2020

martinkennelly commented Nov 23, 2020

zshi-redhat commented Nov 24, 2020

martinkennelly commented Dec 15, 2020 • edited Loading

zshi-redhat commented Dec 16, 2020

zshi-redhat commented Dec 16, 2020

martinkennelly left a comment

Choose a reason for hiding this comment

zshi-redhat commented Jan 29, 2021

adrianchiris commented Jan 31, 2021 • edited Loading

zshi-redhat commented Feb 1, 2021

adrianchiris commented Mar 31, 2021

zshi-redhat commented Apr 1, 2021

lgtm-com bot commented Apr 12, 2021

martinkennelly commented Jul 22, 2021

wattmto commented Aug 17, 2022

superbrothers commented Aug 25, 2022

adrianchiris commented Nov 21, 2023

zshi-redhat commented Sep 8, 2020 •

edited

Loading

zshi-redhat commented Sep 11, 2020 •

edited

Loading

martinkennelly commented Dec 15, 2020 •

edited

Loading

adrianchiris commented Jan 31, 2021 •

edited

Loading