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

[MVP] add resourcequota plugin in scheduler-estimator: add resourcequota plugin #4566

Merged
merged 1 commit into from
Jan 28, 2024
Merged

[MVP] add resourcequota plugin in scheduler-estimator: add resourcequota plugin #4566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions api/openapi-spec/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17952,10 +17952,18 @@
"description": "ReplicaRequirements represents the requirements required by each replica.",
"type": "object",
"properties": {
"namespace": {
"description": "Namespace represents the resources namespaces",
"type": "string"
},
"nodeClaim": {
"description": "NodeClaim represents the node claim HardNodeAffinity, NodeSelector and Tolerations required by each replica.",
"$ref": "#/definitions/com.github.karmada-io.karmada.pkg.apis.work.v1alpha2.NodeClaim"
},
"priorityClassName": {
"description": "PriorityClassName represents the resources priorityClassName",
"type": "string"
},
"resourceRequest": {
"description": "ResourceRequest represents the resources required by each replica.",
"type": "object",
Expand Down
6 changes: 6 additions & 0 deletions charts/karmada/_crds/bases/work/work.karmada.io_clusterresourcebindings.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,9 @@ spec:
description: ReplicaRequirements represents the requirements required
by each replica.
properties:
namespace:
description: Namespace represents the resources namespaces
type: string
nodeClaim:
description: NodeClaim represents the node claim HardNodeAffinity,
NodeSelector and Tolerations required by each replica.
Expand Down Expand Up @@ -1066,6 +1069,9 @@ spec:
type: object
type: array
type: object
priorityClassName:
description: PriorityClassName represents the resources priorityClassName
type: string
resourceRequest:
additionalProperties:
anyOf:
Expand Down
6 changes: 6 additions & 0 deletions charts/karmada/_crds/bases/work/work.karmada.io_resourcebindings.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,9 @@ spec:
description: ReplicaRequirements represents the requirements required
by each replica.
properties:
namespace:
description: Namespace represents the resources namespaces
type: string
nodeClaim:
description: NodeClaim represents the node claim HardNodeAffinity,
NodeSelector and Tolerations required by each replica.
Expand Down Expand Up @@ -1066,6 +1069,9 @@ spec:
type: object
type: array
type: object
priorityClassName:
description: PriorityClassName represents the resources priorityClassName
type: string
resourceRequest:
additionalProperties:
anyOf:
Expand Down
8 changes: 8 additions & 0 deletions pkg/apis/work/v1alpha2/binding_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,14 @@ type ReplicaRequirements struct {
// ResourceRequest represents the resources required by each replica.
// +optional
ResourceRequest corev1.ResourceList `json:"resourceRequest,omitempty"`

// Namespace represents the resources namespaces
// +optional
Namespace string `json:"namespace,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated with ObjectReference.Namespace, why not share? Did I miss something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is because the ReplicaEstimator interface shares the same ReplicaRequirements struct with ResourceBinding:

karmada/pkg/estimator/client/interface.go

Line 39 in f054313

MaxAvailableReplicas(ctx context.Context, clusters []*clusterv1alpha1.Cluster, replicaRequirements *workv1alpha2.ReplicaRequirements) ([]workv1alpha2.TargetCluster, error)

I also noticed this issue at #4534 (comment).

Another reason just as mentioned in PR description:

We discuss if we need to add duplicate namespace filed workv1alpha2.ReplicaRequirements. I add it because we also need that in the resource interpreter https://github.com/karmada-io/karmada/blob/master/pkg/resourceinterpreter/interpreter.go#L46-L47
if we don't include the namespace in workv1alpha2.ReplicaRequirements, I have to make this interface change to return namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is now tracked by #4578.


// PriorityClassName represents the resources priorityClassName
// +optional
PriorityClassName string `json:"priorityClassName,omitempty"`
}

// NodeClaim represents the node claim HardNodeAffinity, NodeSelector and Tolerations required by each replica.
Expand Down
2 changes: 2 additions & 0 deletions pkg/estimator/client/accurate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ func (se *SchedulerEstimator) maxAvailableReplicas(ctx context.Context, cluster
}
if replicaRequirements != nil {
req.ReplicaRequirements.ResourceRequest = replicaRequirements.ResourceRequest
req.ReplicaRequirements.Namespace = replicaRequirements.Namespace
req.ReplicaRequirements.PriorityClassName = replicaRequirements.PriorityClassName
if replicaRequirements.NodeClaim != nil {
req.ReplicaRequirements.NodeClaim = &pb.NodeClaim{
NodeAffinity: replicaRequirements.NodeClaim.HardNodeAffinity,
Expand Down
188 changes: 135 additions & 53 deletions pkg/estimator/pb/generated.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions pkg/estimator/pb/generated.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions pkg/estimator/pb/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,15 @@ type ReplicaRequirements struct {
// ResourceRequest represents the resources required by each replica.
// +optional
ResourceRequest corev1.ResourceList `json:"resourceRequest,omitempty" protobuf:"bytes,2,rep,name=resourceRequest,casttype=k8s.io/api/core/v1.ResourceList,castkey=k8s.io/api/core/v1.ResourceName"`
// Namespace represents the namespaces belonged to a ResourceRequest
// +optional
Namespace string `json:"namespace,omitempty" protobuf:"bytes,3,opt,name=namespace"`
// PriorityClassName represents the priority class name for a given ResourceRequest
// Resource quotas are introduced for multi tenants sharing a cluster
// Besides estimate the replica based on nodes' resources, we need to consider the resource quota of a ResourceRequest
// ResourceQuota have an associated set of scopes, one of them is priority class
// +optional
PriorityClassName string `json:"priorityClassName,omitempty" protobuf:"bytes,4,opt,name=priorityClassName"`
}

// MaxAvailableReplicasResponse represents the response that sent by gRPC server to calculate max available replicas.
Expand Down
5 changes: 4 additions & 1 deletion pkg/estimator/server/framework/plugins/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@ limitations under the License.
package plugins

import (
"github.com/karmada-io/karmada/pkg/estimator/server/framework/plugins/resourcequota"
"github.com/karmada-io/karmada/pkg/estimator/server/framework/runtime"
)

// NewInTreeRegistry builds the registry with all the in-tree plugins.
func NewInTreeRegistry() runtime.Registry {
registry := runtime.Registry{}
registry := runtime.Registry{
resourcequota.Name: resourcequota.New,
}
Comment on lines +26 to +28
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
registry := runtime.Registry{
resourcequota.Name: resourcequota.New,
}
registry := runtime.Registry{}
if features.FeatureGate.Enabled(features.ResourceQuotaEstimate) {
registry.Register(resourcequota.Name, resourcequota.New) // TODO: we might need to deal with the unhandled error
}

Is it better? So that we can skip the effort to check the feature gate in multiple places when implementing the plugin.
In addition, a feature gate will be removed eventually once it moves to a stable, we don't need to touch the plugin logic when doing so.

Copy link
Contributor Author

@wengyao04 wengyao04 Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of controlling in the registry, do you think if we can have feature-gate check in each plugin.New function like https://github.com/karmada-io/karmada/pull/4566/files#diff-e489afa02e13c3ea92d007b4a8575045b8695f34cd31539769f2e28c3c0cb322R72-R77 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm saying that just because I see there are two places we need to check the feature-gate in the plugin. :)
I'm not insisting on it, both will work. No big deal.

return registry
}
Loading
Loading