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.

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

Add v1beta1 API #532

Closed
wants to merge 2 commits into from
Closed

Conversation

alculquicondor
Copy link
Contributor

@alculquicondor alculquicondor commented Jan 26, 2023

What type of PR is this?

/kind feature

/kind api-change

What this PR does / why we need it:

Adds a v1beta1 API with changes summarized in this doc https://docs.google.com/document/d/1Uu4hfGxux4Wh_laqZMLxXdEVdty06Sb2DwB035hj700/edit?usp=sharing&resourcekey=0-b7mU7mGPCkEfhjyYDsXOBg (join https://groups.google.com/a/kubernetes.io/g/wg-batch to access)

Which issue(s) this PR fixes:

Ref #23

Special notes for your reviewer:

The first commit contains a pure copy of the v1alpha2 API objects, to facilitate reviewing the diff.

This PR keeps the storage version as v1alpha2 so that tests continue passing. I plan to remove in a follow up PR, along with updating the implementation of the controllers.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jan 26, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 26, 2023
@tenzen-y
Copy link
Member

/cc


type ResourceGroup struct {
// resources is the list of resources covered by the flavors in this group.
Resources []corev1.ResourceName `json:"resources"`
Copy link
Contributor

Choose a reason for hiding this comment

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

The field has the same name as the one in FlavorQuota but a different type. Maybe one of them should have a different name? It looks a bit weird in yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's ok. They are in different contexts.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 with @mwielgus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Copy link
Contributor

@ahg-g ahg-g left a comment

Choose a reason for hiding this comment

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

any changes compared to v1alpha1 other than clusterqueue_types.go and the queue-name label?

// If the ClusterQueue belongs to a cohort, the sum of the quotas for each
// (flavor, resource) combination defines the maximum quantity that can be
// allocated by a ClusterQueue in the cohort.
Quota resource.Quantity `json:"quota"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Following up on @liggitt suggestion, we can use AssuredLimit for absolute value, and in the future we can add AssuredLimitShares or AssuredLimitPercentage (depending on the semantics we want to enable) when we build hierarchal quota.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The source for this suggestion was API priority and fairness.

In alpha they used AssuredConcurrencyShares https://github.com/kubernetes/kubernetes/blob/f97d14c6c88e92cb505f8a9147294705a93247e3/staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go#L437

But in beta they are using NominalConcurrencyShares https://github.com/kubernetes/kubernetes/blob/f97d14c6c88e92cb505f8a9147294705a93247e3/staging/src/k8s.io/api/flowcontrol/v1beta3/types.go#L471

Definition of nominal:

stated or expressed but not necessarily corresponding exactly to the real value.

It matches our intent, don't you think?

apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Show resolved Hide resolved

// Total is the total quantity of the resource used, including resources
// borrowed from the cohort.
Total resource.Quantity `json:"total,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Here seems to be a bit of inconsistency. In spec we have base + borrowed. Here is total + borrowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point, but I think it's rather useful:
If an administrator considers increasing the quota, it's useful for them to directly see the total usage.
And observing the borrowed quota is useful for adjusting borrowingLimit.

// name of the resource
Name corev1.ResourceName `json:"name"`

// Total is the total quantity of the resource used, including resources
Copy link
Contributor

Choose a reason for hiding this comment

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

What about resources borrowed BY the cohort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot quantify those. A ClusterQueue doesn't directly borrow from another ClusterQueue. They borrow from the pool of unused resources in the Cohort.

This is so that a single Workload can use borrowed quota from multiple CQs at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we kind-of do. Imagine the situation when all capacity in the cohort is taken and the queue itself has no admitted workloads. Then borrowedByCohort = base queue quota. If All queues have low usage, within their quota then borrowedByCohort = 0. Right :)? So we can generalize it to:

max(0, SumOfUsageInOtherQueuesInCohort - SumOfQuotaInOtherQuesesInCohort)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, but it can be added later. Not sure how much worth it is. We don't maintain those numbers in the cache. We just calculate them when we snapshot the cache to run the admission cycle.

type ResourceFlavor struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`

Copy link
Contributor

Choose a reason for hiding this comment

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

Why no Spec and Status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far we haven't found a case for Status, so we skipped it for simplicity. Can you think of something?

This is similar to how PriorityClasses are defined https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#priorityclass

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we will have a problem IF we find a use case for status (like resource depleted notification or so). Without Spec and Status we will have to do another api version, deprecation, migration etc just to have that bit of info exposed. Empty Status doesn't cost us much at this moment, but may be very handy in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we don't need to add a status field, but we can add the spec. See update.

apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
Comment on lines 27 to 33
// resourceGroups describes groups of resources.
// Each resource group defines the list of resources and a list of flavors
// that provide quotas for these resources.
// resourceGroups can be up to 16.
// +listType=atomic
// +kubebuilder:validation:MaxItems=16
Copy link
Member

Choose a reason for hiding this comment

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

Can we mention that it does not guarantee the actual availability of resources?
Moreover, can we add examples, the same as v1alpha2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rephrased a little the definition of nominalQuota below

Copy link
Member

Choose a reason for hiding this comment

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

The word, nominal sounds good.
Thanks for the updates!

apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/constants.go Outdated Show resolved Hide resolved
//+kubebuilder:printcolumn:name="Strategy",JSONPath=".spec.queueingStrategy",type=string,description="The queueing strategy used to prioritize workloads",priority=1
//+kubebuilder:printcolumn:name="Pending Workloads",JSONPath=".status.pendingWorkloads",type=integer,description="Number of pending workloads"
//+kubebuilder:printcolumn:name="Admitted Workloads",JSONPath=".status.admittedWorkloads",type=integer,description="Number of admitted workloads that haven't finished yet",priority=1
//+kubebuilder:storageversion
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to add the conversion webhooks?

Copy link
Contributor Author

@alculquicondor alculquicondor Jan 31, 2023

Choose a reason for hiding this comment

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

Most likely not. But that's part of the justification of going to beta: that from now on we would have a conversion webhook if there are breaking changes from v1beta1 to v1beta2 or v1.
But if you know of users running in prod, it would be good to have their feedback.

Copy link
Member

Choose a reason for hiding this comment

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

Most likely not. But that's part of the justification of going to beta: that from now on we would have a conversion webhook if there are breaking changes from v1beta1 to v1beta2 or v1.

It sounds good to me.

But if you know of users running in prod, it would be good to have their feedback.

I haven't known of users running in production, ever.

@alculquicondor
Copy link
Contributor Author

/retest

@alculquicondor alculquicondor mentioned this pull request Feb 2, 2023
4 tasks
@alculquicondor alculquicondor changed the title WIP Add v1beta1 API Add v1beta1 API Feb 2, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2023
package v1beta1

const (
ResourceInUseFinalizerName = "kueue.k8s.io/resource-in-use"
Copy link
Member

Choose a reason for hiding this comment

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

Is this domain correct?
Probably, kueue.x-k8s.io/resource-in-use?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is my misunderstanding.
Since the API will be graduated to beta, kueue.k8s.io/resource-in-use is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll stay in x-k8s. We have consulted with @liggitt in any case, to make sure we are following best practices in our first beta release.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Thanks for letting me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

"kueue.k8s.io/resource-in-use" or"kueue.x-k8s.io/resource-in-use"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I swear I had changed it before :|

It should be kueue.x-k8s.io. Fixed.


// ResourceFlavor is the Schema for the resourceflavors API.
type ResourceFlavor struct {
metav1.TypeMeta `json:",inline"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a switch to turn off a resource flavor in all of the queues (possibly as a follow up PR)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's beyond the scope of this PR and it deserves it's own kep with stories.

Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Also, how do reviewers feel about merging this as is and have a separate PR for swapping the controllers into the new APIs?

I'm ok.

// If the ClusterQueue belongs to a cohort, the sum of the quotas for each
// (flavor, resource) combination defines the maximum quantity that can be
// allocated by a ClusterQueue in the cohort.
NominalQuota resource.Quantity `json:"quota"`
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
NominalQuota resource.Quantity `json:"quota"`
NominalQuota resource.Quantity `json:"nominalQuota"`

Comment on lines 41 to 42
// cohort. Only quota from [resource, flavor] pairs listed in the CQ can be
// borrowed.
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
// cohort. Only quota from [resource, flavor] pairs listed in the CQ can be
// borrowed.
// cohort. Only borrowingLimit from [resources, flavors] pairs listed in the CQ can be
// borrowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The word quota was intended. The documentation for how much quota is allowed to be borrowed is left for the description of the borrowingLimit field itself.

Copy link
Member

Choose a reason for hiding this comment

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

It sounds good to me.

@netlify
Copy link

netlify bot commented Feb 7, 2023

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 6136eed
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/64022527101eb900082c18e7

Comment on lines -15 to -16
shortNames:
- cq
Copy link
Member

Choose a reason for hiding this comment

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

Is the removal of shortName intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah.... too cryptic

Copy link
Member

Choose a reason for hiding this comment

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

It sounds good to me.
In this case, we may need to remove kubebuiler markers from v1alpha1 API.

@@ -13,7 +13,8 @@ spec:
listKind: ResourceFlavorList
plural: resourceflavors
shortNames:
- rf
Copy link
Member

Choose a reason for hiding this comment

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

Is the removal of shortName intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same, too cryptic. flavor is better

Copy link
Member

Choose a reason for hiding this comment

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

Good.

@tenzen-y
Copy link
Member

tenzen-y commented Feb 7, 2023

Generally, LGTM.

apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved

type ResourceGroup struct {
// resources is the list of resources covered by the flavors in this group.
Resources []corev1.ResourceName `json:"resources"`
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 with @mwielgus

// There could be up to 16 resources.
// +listType=map
// +listMapKey=name
// +kubebuilder:validation:MaxItems=16
Copy link
Contributor

Choose a reason for hiding this comment

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

What about MinItems here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch, added.

apis/kueue/v1beta1/clusterqueue_types.go Outdated Show resolved Hide resolved
Comment on lines +162 to +165
// combination that this ClusterQueue is allowed to borrow from the unused
// quota of other ClusterQueues in the same cohort.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// combination that this ClusterQueue is allowed to borrow from the unused
// quota of other ClusterQueues in the same cohort.
// combination that this ClusterQueue is allowed to borrow from the unused
// quota of other ClusterQueues in the same cohort, but not guaranteed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure... I already mentioned this is unused quota. Or are you referring to something else? But please check the update for your previous comment.

ClusterQueueActive string = "Active"
)

type Usage struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where we used this? Didn't find them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to delete it :)

package v1beta1

const (
ResourceInUseFinalizerName = "kueue.k8s.io/resource-in-use"
Copy link
Contributor

Choose a reason for hiding this comment

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

"kueue.k8s.io/resource-in-use" or"kueue.x-k8s.io/resource-in-use"?


// template is the Pod template.
//
// The only allowed fields in template.metadata are labels and annotations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean we'll validate this in Kueue? Or we'll ignore other fields directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we'll validate in Kueue.

// allocated by a ClusterQueue in the cohort.
NominalQuota resource.Quantity `json:"nominalQuota"`

// borrowingLimit is the maximum amount of quota for the [flavor, resource]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe describe that MaximumQuota = NominalQuota + BorrowingLimit is more intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See update.

@@ -19,10 +19,13 @@ package constants
import "time"

const (
// QueueAnnotation is the annotation in the workload that holds the queue name.
// QueueLabel is the label key in the workload that holds the queue name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// QueueLabel is the label key in the workload that holds the queue name.
// QueueLabel is the label key in the job that holds the queue name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhm... not sure. Others Job APIs should use the same label.

@alculquicondor
Copy link
Contributor Author

/retest

@alculquicondor
Copy link
Contributor Author

It looks like the CI was not liking having different shortnames across versions.

//
// +kubebuilder:default=Never
// +kubebuilder:validation:Enum=Never;LowerPriority;Any
ReclaimWithinCohort PreemptionPolicy `json:"withinCohort,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ReclaimWithinCohort PreemptionPolicy `json:"withinCohort,omitempty"`
WithinCohort PreemptionPolicy `json:"withinCohort,omitempty"`

In order to align the struct name and the json field name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, this was a mistake in #514. It should be reclaimWithinCohort

//
// The type of the condition could be:
//
// - Admitted: the Workload was admitted through a ClusterQueue.
Copy link
Contributor

Choose a reason for hiding this comment

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

Extend the list with PodsReady for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

apis/kueue/v1alpha2/resourceflavor_types.go Show resolved Hide resolved
// Gatekeeper should be used to enforce more advanced policies.
// Defaults to null which is a nothing selector (no namespaces eligible).
// If set to an empty selector `{}`, then all namespaces are eligible.
NamespaceSelector *metav1.LabelSelector `json:"namespaceSelector,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can disambiguate this by naming it allowedNamespcesSelector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't anticipate other kind of namespaceSelector, so I don't see a strong benefit. There is also precedence in the NetworkPolicy https://kubernetes.io/docs/concepts/services-networking/network-policies/#networkpolicy-resource

Copy link
Contributor

Choose a reason for hiding this comment

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

The confusion I heard once was that this field selects the Jobs submitted to the queue. It would be better if the name coveys that this is about policy.

@liggitt any thoughts on naming here?

Copy link

Choose a reason for hiding this comment

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

how does a namespace submit workloads to the cluster queue? what happens if a workload in a namespace not matched by this selector tries to use this cluster queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A namespace submits workloads to a LocalQueue. A LocalQueue references a ClusterQueue.

If the selector doesn't match, the Workload gets a Condition indicating that the Workload is not admitted because it doesn't match the namespaceSelector.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a basic form of policy enforcement, ideally the cluster admin sets up something like gatekeeper, but in most cases gatekeeper is an overkill.

// The list cannot be empty and it can contain up to 16 resources.
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=16
CoveredResources []corev1.ResourceName `json:"coveredResources"`
Copy link
Contributor

@kerthcet kerthcet Feb 10, 2023

Choose a reason for hiding this comment

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

I read the design docs https://docs.google.com/document/d/1Uu4hfGxux4Wh_laqZMLxXdEVdty06Sb2DwB035hj700/edit?resourcekey=0-b7mU7mGPCkEfhjyYDsXOBg#heading=h.dkobprho4lxe and I got the intension about adding a slice of resource names as a visual aid, just want to make sure we all agree on this. cc @ahg-g

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to being a visual aid, it makes the API more explicit, which is a good principle.

But I agree that it might add burden, specially for resource groups with a single resource. But, on the other hand, ClusterQueues should be APIs that don't change often.

Resources []ResourceUsage `json:"resources"`
}

type ResourceUsage struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that means borrowed, not borrowing. my brain....

@kerthcet
Copy link
Contributor

Thanks @alculquicondor for the great work, LGTMed.

@alculquicondor
Copy link
Contributor Author

Thank you all
I think we are pretty much settled now.
I have been working in transitioning the controllers to the new APIs. I'll keep this PR open (on hold) just in case I find some limitations in the implementation.

@tenzen-y
Copy link
Member

Thank you all I think we are pretty much settled now. I have been working in transitioning the controllers to the new APIs. I'll keep this PR open (on hold) just in case I find some limitations in the implementation.

Sounds good. I'm looking forward to kueue v1beta1 :)
If you would help from others, feel free to send a ping to me.

@@ -19,10 +19,13 @@ package constants
import "time"

const (
// QueueAnnotation is the annotation in the workload that holds the queue name.
// QueueLabel is the label key in the workload that holds the queue name.
QueueLabel = "kueue.x-k8s.io/queue-name"
Copy link
Contributor

@kerthcet kerthcet Feb 23, 2023

Choose a reason for hiding this comment

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

Revisit this label, I'm reading the api designs about MatchFields https://github.com/kubernetes/kubernetes/blob/fc8b5668f506b6d71ddc5243a21f5d0c5f8271d3/pkg/apis/core/types.go#L2558-L2563

This field was designed specially for daemonSet pod scheduling, one reason why we avoid to reuse MatchExpressions is because we can't model label as non-label usages(take daemonset for example, we're finding the exactly node not a set of nodes), in this case, it should be an unique localQueue name, not a set of queue names. Annotation is something we think not bad as an experimental feature, and considering we haven't populate queueName to job yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying that we should stay with an annotation instead of a label?

I'm not understanding what you mean by a unique localQueue name. Multiple workloads can point to the same queue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean a workload can only belong to one localQueue, we're taking the name field into labels,, which seems like we're abusing the label.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This label is for the (custom) Job objects, not the Workloads. We call them workloads in general. Maybe we should call them jobs? not sure.

Change-Id: Ic4cc56f07e8ef5caa7b55ac02901a54708c35f20
Change-Id: I9a12e50eaa66c1897406b44e59f9e0813382645d
@alculquicondor
Copy link
Contributor Author

/close
in favor of #604

@k8s-ci-robot
Copy link
Contributor

@alculquicondor: Closed this PR.

In response to this:

/close
in favor of #604

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants