Add labels for workload parent #1032

achernevskii · 2023-08-02T13:24:33Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Added parent name and group-kind labels for workload objects.
If parent's name, or group-kind exceeds the limit of 63 characters length, respective labels won't be added to a workload object.

Which issue(s) this PR fixes:

Fixes #992

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Workload objects have the label `kueue.x-k8s.io/job-uid` where the value matches the uid of the parent job, whether that's a Job, MPIJob, RayJob, JobSet

* Added parent name and group-kind labels for workload objects. * If parent's name, or group-kind exceeds the limit of 63 characters length, respective labels won't be added to a workload object.

netlify · 2023-08-02T13:24:40Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`0350ad8`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/64cbe270f7ec60000815cac7

k8s-ci-robot · 2023-08-02T13:24:42Z

Welcome @achernevskii!

It looks like this is your first PR to kubernetes-sigs/kueue 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kueue has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-08-02T13:24:43Z

Hi @achernevskii. 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.

trasc · 2023-08-02T13:27:23Z

/ok-to-test

pkg/controller/jobframework/reconciler.go

* UID shouldn't exceed the 63 character label value length limit, and it uniquely identifies a parent.

pkg/controller/constants/constants.go

alculquicondor · 2023-08-02T20:50:44Z

pkg/controller/jobframework/reconciler.go

-
-	if utf8.RuneCountInString(jobGVK.GroupKind().String()) < 64 {
-		wl.Labels[controllerconsts.ParentGroupKindLabel] = jobGVK.GroupKind().String()
+	if uid := string(job.Object().GetUID()); utf8.RuneCountInString(uid) < 64 && uid != "" {


k8s validation just uses len
https://github.com/kubernetes/kubernetes/blob/2c6c4566eff972d6c1320b5f8ad795f88c822d09/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L169

I think using the rune len would be weaker.

I'm wondering if we should just try to put the label and let the apiserver return an error. Note that there is a regex check, so that could potentially fail as well.

I'm wondering if we should just try to put the label and let the apiserver return an error.

We can do that, but I would rather create a workload without a job-uid label, since this functionality is not essential for the controller. We could also log a warning if a label can't be added to a workload. Please let me know which of those three options is the best in your opinion.

Note that there is a regex check, so that could potentially fail as well.

According to this page, UIDs are standardized as ISO/IEC 9834-8 and as ITU-T X.667. And ISO/IEC 9834-8 says the following:

UUIDs forming a component of an OID are represented in ASN.1 value notation as the decimal representation of their integer value, but for all other display purposes it is more usual to represent them with hexadecimal digits with a hyphen separating the different fields within the 16-octet UUID.

Regex that kuberentes is using for label validation is the following (([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?, if I'm not mistaken. This regex should match the UID format, so I think we're good here.

k8s validation just uses len

Replaced RuneCountInString call with len call in 866a09a

We can do that, but I would rather create a workload without a job-uid label, since this functionality is not essential for the controller.

In that case, use IsValidLabelValue from https://github.com/kubernetes/kubernetes/blob/2c6c4566eff972d6c1320b5f8ad795f88c822d09/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L167

We could also log a warning if a label can't be added to a workload.

Logging is not visible to users. Webhooks support warnings https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#best-practices-and-warnings:~:text=Admission%20webhooks%20can%20optionally%20return%20warning%20messages%20that%20are%20returned%20to%20the%20requesting%20client%20in%20HTTP%20Warning%20headers%20with%20a%20warning%20code%20of%20299.%20Warnings%20can%20be%20sent%20with%20allowed%20or%20rejected%20admission%20responses.

But in our case, it's not end users creating Workloads, but users. Maybe a log is not so bad so administrators can take action.

UIDs are standardized as ISO/IEC 9834-8 and as ITU-T X.667.

I think there is a way where an end user can override the UID and potentially use something that wouldn't pass label validation. However this should be very uncommon.
The log should be enough :)

Replaced len label check with IsValidLabelValue, and added the log message in 0350ad8.

mimowo

lgtm, just nits

mimowo · 2023-08-03T13:01:20Z

pkg/controller/constants/constants.go

@@ -32,4 +32,8 @@ const (
 	// ignores this Job from admission, and takes control of its suspension
 	// status based on the admission status of the parent workload.
 	ParentWorkloadAnnotation = "kueue.x-k8s.io/parent-workload"
+
+	// JobUIDLabel is the label key in the workload resource, that holds the UID of
+	// a parent job.


Suggested change

// a parent job.

// the owner job.

nit: Similarly, to avoid confusion with parent which is a relation between jobs in Kueue.

Changed the comment in 0350ad8.

mimowo · 2023-08-03T13:02:07Z

pkg/util/testing/wrappers.go

@@ -165,6 +165,11 @@ func (w *WorkloadWrapper) ReclaimablePods(rps ...kueue.ReclaimablePod) *Workload
 	return w
 }

+func (w *WorkloadWrapper) SetLabels(l map[string]string) *WorkloadWrapper {


Suggested change

func (w *WorkloadWrapper) SetLabels(l map[string]string) *WorkloadWrapper {

func (w *WorkloadWrapper) Labels(l map[string]string) *WorkloadWrapper {

nit, but we generally don't use for functions like this.

Renamed this method in 0350ad8.

* Replace len label check with IsValidLabelValue from k8s apimachinery, which is used on the server side to validate the label values. * Add a log message, for the case, when UID label validation returns any errors.

alculquicondor

/approve

alculquicondor · 2023-08-03T17:26:37Z

pkg/controller/jobframework/reconciler.go

+		wl.Labels[controllerconsts.JobUIDLabel] = jobUid
+	} else {
+		log.V(2).Info(
+			"Validation of the owner job UID label has failed. Creating workload without the label.",


Suggested change

"Validation of the owner job UID label has failed. Creating workload without the label.",

"Validation of the owner job UID label has failed. Creating workload without the label",

alculquicondor · 2023-08-03T17:26:49Z

pkg/controller/jobframework/reconciler.go

+	} else {
+		log.V(2).Info(
+			"Validation of the owner job UID label has failed. Creating workload without the label.",
+			"ValidationErrors", errs,


Suggested change

"ValidationErrors", errs,

"err", errs.ToAggregate(),

It's not a ErrorListtype here, IsValidLabelValue will return a slice of strings, so I can't call this method here.

ahhhh, that's why the value is not present either

alculquicondor · 2023-08-03T17:27:47Z

pkg/controller/jobframework/reconciler.go

+		log.V(2).Info(
+			"Validation of the owner job UID label has failed. Creating workload without the label.",
+			"ValidationErrors", errs,
+			"LabelValue", jobUid,


this will probably already be part of the error, so no need to include again.

This is the log message, that I'm getting while unit testing:

"Validation of the owner job UID label has failed. Creating workload without the label" "job"="ns/job" "ValidationErrors"=["must be no more than 63 characters"] "LabelValue"="long-uidlong-uidlong-uidlong-uidlong-uidlong-uidlong-uidlong-uid"

Label is not included in the error here.

k8s-ci-robot · 2023-08-03T17:28:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achernevskii, 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:

~~pkg/controller/OWNERS~~ [alculquicondor]
~~pkg/util/OWNERS~~ [alculquicondor]

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

alculquicondor · 2023-08-03T17:28:33Z

Please add a release note.

alculquicondor

/lgtm

alculquicondor · 2023-08-03T17:44:07Z

/release-note-edit

Workload objects have the label `kueue.x-k8s.io/job-uid` where the value matches the uid of the parent job, whether that's a Job, MPIJob, RayJob, JobSet

Added labels for workload parent

fb1a28d

* Added parent name and group-kind labels for workload objects. * If parent's name, or group-kind exceeds the limit of 63 characters length, respective labels won't be added to a workload object.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 2, 2023

k8s-ci-robot requested review from denkensk and trasc August 2, 2023 13:24

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 2, 2023

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 Aug 2, 2023

alculquicondor reviewed Aug 2, 2023

View reviewed changes

pkg/controller/jobframework/reconciler.go Outdated Show resolved Hide resolved

alculquicondor reviewed Aug 2, 2023

View reviewed changes

pkg/controller/jobframework/reconciler.go Outdated Show resolved Hide resolved

Replace parent name and group-kind labels with parent uid

2478cbd

* UID shouldn't exceed the 63 character label value length limit, and it uniquely identifies a parent.

achernevskii changed the title ~~Added labels for workload parent~~ Add labels for workload parent Aug 2, 2023

alculquicondor reviewed Aug 2, 2023

View reviewed changes

Rename parent-uid label to job-uid, change label validation

866a09a

mimowo reviewed Aug 3, 2023

View reviewed changes

Update job-uid label validation

0350ad8

* Replace len label check with IsValidLabelValue from k8s apimachinery, which is used on the server side to validate the label values. * Add a log message, for the case, when UID label validation returns any errors.

alculquicondor reviewed Aug 3, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2023

alculquicondor reviewed Aug 3, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Aug 3, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2023

k8s-ci-robot merged commit b2d395b into kubernetes-sigs:main Aug 3, 2023

k8s-ci-robot added this to the v0.5 milestone Aug 3, 2023

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 3, 2023

achernevskii deleted the feature/workload_labels branch August 3, 2023 19:28

achernevskii mentioned this pull request Oct 26, 2023

REQUEST: New membership for achernevskii kubernetes/org#4547

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add labels for workload parent #1032

Add labels for workload parent #1032

achernevskii commented Aug 2, 2023 •

edited by k8s-ci-robot

Loading

netlify bot commented Aug 2, 2023 •

edited

Loading

k8s-ci-robot commented Aug 2, 2023

k8s-ci-robot commented Aug 2, 2023

trasc commented Aug 2, 2023

alculquicondor Aug 2, 2023

achernevskii Aug 3, 2023

achernevskii Aug 3, 2023 •

edited

Loading

alculquicondor Aug 3, 2023

achernevskii Aug 3, 2023

mimowo left a comment

mimowo Aug 3, 2023

achernevskii Aug 3, 2023

mimowo Aug 3, 2023

achernevskii Aug 3, 2023

alculquicondor left a comment

alculquicondor Aug 3, 2023

alculquicondor Aug 3, 2023

achernevskii Aug 3, 2023 •

edited

Loading

alculquicondor Aug 3, 2023

alculquicondor Aug 3, 2023

achernevskii Aug 3, 2023

k8s-ci-robot commented Aug 3, 2023

alculquicondor commented Aug 3, 2023

alculquicondor left a comment

alculquicondor commented Aug 3, 2023

	func (w WorkloadWrapper) SetLabels(l map[string]string) WorkloadWrapper {
	func (w WorkloadWrapper) Labels(l map[string]string) WorkloadWrapper {

	"Validation of the owner job UID label has failed. Creating workload without the label.",
	"Validation of the owner job UID label has failed. Creating workload without the label",

Add labels for workload parent #1032

Add labels for workload parent #1032

Conversation

achernevskii commented Aug 2, 2023 • edited by k8s-ci-robot Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Aug 2, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented Aug 2, 2023

k8s-ci-robot commented Aug 2, 2023

trasc commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achernevskii Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achernevskii Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 3, 2023

alculquicondor commented Aug 3, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor commented Aug 3, 2023

achernevskii commented Aug 2, 2023 •

edited by k8s-ci-robot

Loading

netlify bot commented Aug 2, 2023 •

edited

Loading

achernevskii Aug 3, 2023 •

edited

Loading

achernevskii Aug 3, 2023 •

edited

Loading