feat: setup property checker for capacity #1210
Conversation
britaniar
force-pushed
the
setupCapacity
branch
3 times, most recently
from
68b3cfd to
a364391
Compare
pkg/scheduler/framework/plugins/clusteraffinity/azure/capcacity.go
Outdated
Show resolved
Hide resolved
britaniar
changed the title
pkg/scheduler/framework/plugins/clusteraffinity/azure/capacity.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/clusteraffinity/azure/capcacity.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/clusteraffinity/azure/capcacity.go
Outdated
Show resolved
Hide resolved
britaniar
force-pushed
the
setupCapacity
branch
2 times, most recently
from
c427486 to
32caad2
Compare
britaniar
force-pushed
the
setupCapacity
branch
2 times, most recently
from
0b6f14a to
5b54e69
Compare
pkg/scheduler/framework/plugins/clusteraffinity/azure/capacity.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/clusteraffinity/azure/capacity.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/clusteraffinity/azure/compute.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/clusteraffinity/azure/compute.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/clusteraffinity/azure/compute.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/clusteraffinity/azure/compute.go
Outdated
Show resolved
Hide resolved
britaniar
force-pushed
the
setupCapacity
branch
from
7e7c636 to
642479e
Compare
britaniar
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
britaniar
force-pushed
the
setupCapacity
branch
from
7b330eb to
419f4ac
Compare
ryanzhang-oss
left a comment
ryanzhang-oss
left a comment
There was a problem hiding this comment.
I am a bit confused, what does this PR do? I think it will be good to add an overall description.
britaniar
force-pushed
the
setupCapacity
branch
2 times, most recently
from
213cc94 to
52ad7d7
Compare
zhiying-lin
left a comment
zhiying-lin
left a comment
There was a problem hiding this comment.
I have not reviewed the code in the Scheduler package as the current PR is big enough. Let's merge the scheduler related changes in a separate PR.
| // a Kubernetes cluster. | ||
| PerGBMemoryCostProperty = "kubernetes.azure.com/per-gb-memory-cost" | ||
|
|
||
| NodeCountPerSKUPropertyTmpl = "kubernetes.azure.com/vm-size/%s/count" |
There was a problem hiding this comment.
I want to be more careful to update existing files to avoid potential merge conflicts. And the capacity API is not used in propertyprovider at all. @zhiying-lin
There was a problem hiding this comment.
Basically I think the capacity property should be put in propertychecker not propertyprovider
There was a problem hiding this comment.
Zhiying asked me to move here. I think we wanted to group the azure properties together.
#1210 (comment)
There was a problem hiding this comment.
it won't conflict with the backport, as it's azure folder.
Yeah, would like to group them together and they're sharing the same prefix though the property is not provided by property provider controller.
There was a problem hiding this comment.
https://github.com/kubefleet-dev/kubefleet/tree/main/pkg/propertyprovider/azure kubefleet has azure propertyprovider too.
There was a problem hiding this comment.
please check with Ryan to see what's the diff for azure property provider in kubefleet & fleet.
If they're going to diverge, let's keep the prefix in this file and move the CapacityPerSKUPropertyTmpl to propertychecker.
| // a Kubernetes cluster. | ||
| PerGBMemoryCostProperty = "kubernetes.azure.com/per-gb-memory-cost" | ||
|
|
||
| NodeCountPerSKUPropertyTmpl = "kubernetes.azure.com/vm-size/%s/count" |
There was a problem hiding this comment.
please check with Ryan to see what's the diff for azure property provider in kubefleet & fleet.
If they're going to diverge, let's keep the prefix in this file and move the CapacityPerSKUPropertyTmpl to propertychecker.
|
|
||
| // Package azure provides property checkers for Azure-specific cluster requirements. | ||
| // It checks whether the cluster can meet the requirement defined by the property selector. | ||
| package azure |
There was a problem hiding this comment.
just realized that this is not part of the Azure provider? I wonder why? This is only going to be used by the azure provider, right?
There was a problem hiding this comment.
property checker is a interface to check if the cluster meets the property requirement. The current azure one is not generic enough and it can be used by other properties if we have, which we can refactor later. It won't be used by the provider and will be used by the clusteraffinity plugin instead.
While the property provider is to collect and store the property as part of the member cluster CR.
| func (s *PropertyChecker) CheckIfMeetSKUCapacityRequirement( | ||
| cluster *clusterv1beta1.MemberCluster, | ||
| req placementv1beta1.PropertySelectorRequirement, | ||
| sku string, |
There was a problem hiding this comment.
where is this sku coming from? IIRC. req includes both the sku and the capacity (sku in the name while capacity in the value). I wonder why we process the sku info outside of this function while parse the capacity inside this function?
There was a problem hiding this comment.
52ad7d7#diff-909e9d3ff38befb3e868379f89427c9f606366ca40d4f997f4bf0f0668a68b8a
we need to first figure out whether it's azure SKUproperty first before calling this.
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
britaniar
force-pushed
the
setupCapacity
branch
from
684d833 to
0363ca3
Compare
4 participants
Description of your changes
Create a new struct called property checker with capacity related client and functions to verify cluster is valid based on property requirement.
Run
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
Another PR will be opened containing the deployment changes.
Another PR will be opened containing scheduler changes.