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

Copy some labels from Pods/Jobs into the Workload object #1959

Merged
merged 21 commits into from
Apr 12, 2024

Conversation

pajakd
Copy link
Contributor

@pajakd pajakd commented Apr 9, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

When creating a Workload object from Pod/Job, copy labels of the Pod/Job into the Workload object. It will allow to easily identify and filter the Workload objects.

Which issue(s) this PR fixes:

Fixes #1834

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added label copying from Pod/Job into the Kueue Workload.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Apr 9, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 9, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @pajakd. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 9, 2024
Copy link

netlify bot commented Apr 9, 2024

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 4130abf
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6619304a69a5e40008d6e42d

@kannon92
Copy link
Contributor

kannon92 commented Apr 9, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 9, 2024
@@ -301,6 +301,10 @@ type Integrations struct {
Frameworks []string `json:"frameworks,omitempty"`
// PodOptions defines kueue controller behaviour for pod objects
PodOptions *PodIntegrationOptions `json:"podOptions,omitempty"`

// A list of label keys that should be copied from job
// into the workload object
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain what happens if the job doesn't have the label.

And, in the case of composable jobs, what happens if there are different values in different objects.

Copy link
Contributor Author

@pajakd pajakd Apr 10, 2024

Choose a reason for hiding this comment

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

When the label is not there it is not copied (no error, no warning). Added an explanation in the comment (also about the case of mismatched labels).

keps/1834-copy-labels-into-workload/README.md Show resolved Hide resolved
- "@alculquicondor"
- "@gabesaba"
approvers:
- TBD
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.



# The target maturity stage in the current dev cycle for this KEP.
stage: alpha
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is no feature gate, maybe we should call this as beta (to match the API version).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


# The following PRR answers are required at alpha release
# List the feature gate name and the components for which it must be enabled
disable-supported: true
Copy link
Contributor

Choose a reason for hiding this comment

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

is it?
There is no feature gate (I wouldn't add one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry. Removed the disable-supported and the comments.

Comment on lines 799 to 802
workloadLabels := make(map[string]string)
if r.labelKeysToCopy == nil {
return workloadLabels
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Reducing allocations:

Suggested change
workloadLabels := make(map[string]string)
if r.labelKeysToCopy == nil {
return workloadLabels
}
if len(r.labelKeysToCopy) == 0 {
return nil
}
workloadLabels := make(map[string]string, len(r.labelKeysToCopy))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workload must have the Labels field initialized because later we use it (assigning JobUIDLabel) for every workload (that has a valid jobuid).

Since output of this function is used directly as Labels in the workload object we (probably) should not return nil.

I did apply the suggested change (but returning empty list instead of nil).

func (p *Pod) getWorkloadLabels(labelKeysToCopy []string) (map[string]string, error) {
jobLabels := p.Object().GetLabels()
workloadLabels := make(map[string]string)
if labelKeysToCopy == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar recommendation as the job controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we can return nil, so I'm applying the suggestion entirely.

pkg/controller/jobs/pod/pod_controller.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@pajakd pajakd left a comment

Choose a reason for hiding this comment

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

Thank you for the comments. I think I addressed them. Please take a look if everything looks ok.

keps/1834-copy-labels-into-workload/README.md Show resolved Hide resolved
- "@alculquicondor"
- "@gabesaba"
approvers:
- TBD
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.



# The target maturity stage in the current dev cycle for this KEP.
stage: alpha
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


# The following PRR answers are required at alpha release
# List the feature gate name and the components for which it must be enabled
disable-supported: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry. Removed the disable-supported and the comments.

Comment on lines 799 to 802
workloadLabels := make(map[string]string)
if r.labelKeysToCopy == nil {
return workloadLabels
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workload must have the Labels field initialized because later we use it (assigning JobUIDLabel) for every workload (that has a valid jobuid).

Since output of this function is used directly as Labels in the workload object we (probably) should not return nil.

I did apply the suggested change (but returning empty list instead of nil).

pkg/controller/jobs/pod/pod_controller.go Outdated Show resolved Hide resolved
func (p *Pod) getWorkloadLabels(labelKeysToCopy []string) (map[string]string, error) {
jobLabels := p.Object().GetLabels()
workloadLabels := make(map[string]string)
if labelKeysToCopy == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we can return nil, so I'm applying the suggestion entirely.

Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

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

Some questions to the KEP for now.


A case that requires more attention is creating workloads from pod groups because in that case a single workload is based on multiple pods (each of which might have labels). We propose:
* When a label from the `labelKeysToCopy` list will be present at some of the pods from the group and the value of this label on all these pods will be identical then this label will be copied into the workload. Note that we do not require that all the pods have the label but all that do have must have the same value.
* When multiple pods from the group will have the label with different values, we will raise an exception.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the Workload object created only when we have all the pods in a group created? If it is created when the first pod is created, then we may not have a chance to raise the exception.
Also, related:

  • when is the exception raised, it is during workload creation, or later? If later, how is the error communicated to the user?
  • what happens when the workload already exists, and the user creates a new pod with a non-matching value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know the workload object is created when all the pods are created.

The exception is raised during workload creation.

The new feature is only applied during the creation of a new workload so if the user creates another pod later it will have no effect on the labels on the running workload.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know the workload object is created when all the pods are created.

This would simplify this indeed. Could you please double check on a running system?

The exception is raised during workload creation.
The new feature is only applied during the creation of a new workload so if the user creates another pod later it will have no effect on the labels on the running workload.

sgtm, please add a sentence along the lines to clarify in KEP for future readers.

Copy link
Contributor

@alculquicondor alculquicondor Apr 10, 2024

Choose a reason for hiding this comment

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

As far as I know the workload object is created when all the pods are created.

That 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.

Done. Added a sentence to KEP clarifying this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying and the update.

keps/1834-copy-labels-into-workload/README.md Show resolved Hide resolved
Copy link
Contributor Author

@pajakd pajakd left a comment

Choose a reason for hiding this comment

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

Thank you for the comments!

keps/1834-copy-labels-into-workload/README.md Show resolved Hide resolved

A case that requires more attention is creating workloads from pod groups because in that case a single workload is based on multiple pods (each of which might have labels). We propose:
* When a label from the `labelKeysToCopy` list will be present at some of the pods from the group and the value of this label on all these pods will be identical then this label will be copied into the workload. Note that we do not require that all the pods have the label but all that do have must have the same value.
* When multiple pods from the group will have the label with different values, we will raise an exception.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know the workload object is created when all the pods are created.

The exception is raised during workload creation.

The new feature is only applied during the creation of a new workload so if the user creates another pod later it will have no effect on the labels on the running workload.

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.

I reviewed only proposal.

kep-number: 1834
authors:
- "@pajakd"
status: draft
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
status: draft
status: implementable

Isn't this implementable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

keps/1834-copy-labels-into-workload/README.md Show resolved Hide resolved
keps/1834-copy-labels-into-workload/README.md Show resolved Hide resolved

## Design Details

We believe that the KEP is rather straightforward and the [Proposal](#proposal) section contains sufficient details of the implementation.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add the configuration API detail 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.

Done

// constructed workload object will be created without this label. In the case
// of creating a workload from a composable job (pod group), if multiple objects
// have labels with some key form the list, the values of these labels must
// agree or otherwise the workload creation would fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also mention that if the Job labels change after the first Workload object is created, the Workload will not be updated.

keps/1834-copy-labels-into-workload/README.md Show resolved Hide resolved
@@ -784,16 +794,29 @@ func (r *JobReconciler) finalizeJob(ctx context.Context, job GenericJob) error {
return nil
}

func (r *JobReconciler) getWorkloadLabels(job GenericJob) map[string]string {
jobLabels := job.Object().GetLabels()
Copy link
Contributor

Choose a reason for hiding this comment

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

move this line after the check for len(r.labelKeysToCopy).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -569,6 +573,9 @@ var _ = ginkgo.Describe("Pod controller", ginkgo.Ordered, ginkgo.ContinueOnFailu
},
wlConditionCmpOpts...,
))
ginkgo.By("checking the workload gets assigned the correct labels")
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
ginkgo.By("checking the workload gets assigned the correct labels")
// Checking the workload gets assigned the correct labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -898,7 +900,36 @@ func (p *Pod) ensureWorkloadOwnedByAllMembers(ctx context.Context, c client.Clie
return nil
}

func (p *Pod) ConstructComposableWorkload(ctx context.Context, c client.Client, r record.EventRecorder) (*kueue.Workload, error) {
func (p *Pod) getWorkloadLabels(labelKeysToCopy []string) (map[string]string, error) {
jobLabels := p.Object().GetLabels()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: here also move below the len check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 909 to 913
for _, labelKey := range labelKeysToCopy {
if labelValue, found := jobLabels[labelKey]; found {
workloadLabels[labelKey] = labelValue
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move this pass under the !p.isGroup check? It seems the case when this is a group is handled below.

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 is handling the case when the object is a single pod. We have to copy the labels from the single pod into the workload in this case.

Copy link
Contributor

@mimowo mimowo Apr 11, 2024

Choose a reason for hiding this comment

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

Yes, my idea was:

        workloadLabels := make(map[string]string, len(labelKeysToCopy))
	if !p.isGroup {
	        for _, labelKey := range labelKeysToCopy {
		        if labelValue, found := jobLabels[labelKey]; found {
			        workloadLabels[labelKey] = labelValue
		        }
	        }
		return workloadLabels, nil
	}
        // the code for group handling

would it work?

Copy link
Contributor Author

@pajakd pajakd Apr 11, 2024

Choose a reason for hiding this comment

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

yes, I think its correct. Applied the suggested changes

Obj()},
wantPods: nil,
reconcilerOptions: []jobframework.Option{
jobframework.WithLabelKeysToCopy([]string{"toCopyKey"}),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add also a label key name that is absent in the pod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/controller/jobs/pod/pod_controller.go Show resolved Hide resolved
Copy link
Contributor

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

design, implementation and tests LGTM.
The API comment needs a bit of work.

@tenzen-y anything to add?

// a job does not have some label with the given key from this list, the
// constructed workload object will be created without this label. In the case
// of creating a workload from a composable job (pod group), if multiple objects
// have labels with some key form the list, the values of these labels must
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
// have labels with some key form the list, the values of these labels must
// have labels with some key from the list, the values of these labels must

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// constructed workload object will be created without this label. In the case
// of creating a workload from a composable job (pod group), if multiple objects
// have labels with some key form the list, the values of these labels must
// agree or otherwise the workload creation would fail. The labels are copied only
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
// agree or otherwise the workload creation would fail. The labels are copied only
// match or otherwise the workload creation would fail. The labels are copied only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -301,6 +301,17 @@ type Integrations struct {
Frameworks []string `json:"frameworks,omitempty"`
// PodOptions defines kueue controller behaviour for pod objects
PodOptions *PodIntegrationOptions `json:"podOptions,omitempty"`

// A list of label keys that should be copied from the job into the workload
// object. We don't require the job to have all the labels from this list. If
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
// object. We don't require the job to have all the labels from this list. If
// object. It is not required for the job to have all the labels from this list. If

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

keps/1834-copy-labels-into-workload/kep.yaml Show resolved Hide resolved
@@ -301,6 +301,17 @@ type Integrations struct {
Frameworks []string `json:"frameworks,omitempty"`
// PodOptions defines kueue controller behaviour for pod objects
PodOptions *PodIntegrationOptions `json:"podOptions,omitempty"`

// A list of label keys that should be copied from the job into the workload
Copy link
Contributor

Choose a reason for hiding this comment

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

One general recommendation when writing documentation and API comments, is to use active voice https://kubernetes.io/docs/contribute/style/style-guide/#use-active-voice

Try to adhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll read and follow the style guide. But here I don't see any good way of rephrasing "should be copied" into active voice...

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, there is the exception Use passive voice if active voice leads to an awkward construction :)

Copy link
Contributor

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

/approve

Leaving LGTM to @tenzen-y

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2024
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.

Thank you for this update!
Basically, lgtm. I left a few minor comments.

@@ -301,6 +301,17 @@ type Integrations struct {
Frameworks []string `json:"frameworks,omitempty"`
// PodOptions defines kueue controller behaviour for pod objects
PodOptions *PodIntegrationOptions `json:"podOptions,omitempty"`

// A list of label keys that should be copied from the job into the workload
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
// A list of label keys that should be copied from the job into the workload
// labelKeysToCopy is a list of label keys that should be copied from the job into the workload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

keps/1834-copy-labels-into-workload/README.md Show resolved Hide resolved
``` go
type Integrations struct {
...
// A list of label keys that should be copied from the job into the workload
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
// A list of label keys that should be copied from the job into the workload
// labelKeysToCopy is a list of label keys that should be copied from the job into the workload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 24 to 25
alpha: "v0.7"
beta: "v0.8"
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
alpha: "v0.7"
beta: "v0.8"
beta: "v0.7"

I'm wondering if we should declare this feature as a beta in v0.7, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok without a feature gate since the risk is minimal, but if there is no feature gate, should we declare "beta", or "stable" already?

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 know (I'll let you decide). For the time being I applied the suggestion above.

"toCopyKey": "toCopyValue"}).
Obj(),
},

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -784,16 +794,29 @@ func (r *JobReconciler) finalizeJob(ctx context.Context, job GenericJob) error {
return nil
}

func (r *JobReconciler) getWorkloadLabels(job GenericJob) map[string]string {
if len(r.labelKeysToCopy) == 0 {
return map[string]string{}
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 curious about if we can return just nil. WDYT?

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 reworked this logic (see the comment below).

Comment on lines 910 to 914
for _, labelKey := range labelKeysToCopy {
if labelValue, found := podLabels[labelKey]; found {
workloadLabels[labelKey] = labelValue
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is completely the same logic as the generic reconciler:
So, could we commonize them?

	jobLabels := job.Object().GetLabels()
	workloadLabels := make(map[string]string, len(r.labelKeysToCopy))
	for _, labelKey := range r.labelKeysToCopy {
		if labelValue, found := jobLabels[labelKey]; found {
			workloadLabels[labelKey] = labelValue
		}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, good idea. So this is in fact filtering a map with a certain list of keys. I extracted this into maps library (added a generic maps.FilterKeys function and a bunch of unit tests). And I'm using it here and in the generic reconciler. Please take a look if it looks better.

@@ -569,6 +573,9 @@ var _ = ginkgo.Describe("Pod controller", ginkgo.Ordered, ginkgo.ContinueOnFailu
},
wlConditionCmpOpts...,
))
// Checking the workload gets assigned the correct labels.
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
// Checking the workload gets assigned the correct labels.
ginkgo.By("Checking the workload gets assigned the correct labels")

This comment is helpful for the debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Message: "Created Workload: ns/" + GetWorkloadNameForJob(baseJobWrapper.Name, types.UID("test-uid")),
},
},
},
Copy link
Member

Choose a reason for hiding this comment

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

Could you add another case to verify if the labels are not populated to Workloads after the Workload is already created?

I'd like to prove this comment: "The labels are copied only during the workload creation and are not updated even if the labels of the underlying job are changed."

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 added a check in the job_controller integration test. Please take a look.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2024
Copy link
Contributor

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: eb17443205045edb36e8a055bd337161a9d89c32

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, pajakd

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 merged commit 85206ab into kubernetes-sigs:main Apr 12, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.7 milestone Apr 12, 2024
@pajakd pajakd deleted the copy_labels branch April 15, 2024 05:04
vsoch pushed a commit to researchapps/kueue that referenced this pull request Apr 18, 2024
…sigs#1959)

* Copy labels for a single job into the workload.

* Copying of the labels from pod (and pod groups)

* KEP first draft

* Unit tests for pod reconciler

* Unit test for label copying from job to workload

* Unit test for the case of label mismatch

* Typos fix

* Fix typos

* Update the autogenerated file

* Addressing PR comments

* Addressing PR comments

* Addressing PR comments

* Addressing PR comments

* Addressing PR comments

* Addressing PR comments

* Addressing PR comments

* Addressing PR comments
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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy some labels from Pods/Jobs into the Workload object
6 participants