[YUNIKORN-1351] alway prefer annotations above labels #677

chenyulin0719 · 2023-09-15T16:43:17Z

What is this PR for?

Since we're looking for deprecated labels and move everything to annotation in the future.
This PR change newly added labels to annotations in admission controller when mutating the pods.
This PR also fix existing mismatch to align the logic of "fetching annotations from Pods takes precedence over labels."

Below are the detailed change list:

Get queue name from annotation first. (GetQueueNameFromPod)
Get isStateAwareDisabled from annotation first (isStateAwareDisabled)
Move lables applicationId/queue in newPlaceholder to annotation (newPlaceholder)
Add new annotation 'yunikorn.apache.org/disable-state-aware' to replace equivalent label.
Rename updateLabels to updateApplicationInfo in AdmissionController
Rename util.updatePodLabel to util.getAnnotationsForApplicationInfoUpdate

What type of PR is it?

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-1351

How should this be tested?

Screenshots (if appropriate)

Before vs After result. (Pod with existing applicationId, queue labels.)

Before vs After result. (Pod with no label.)

Questions:

- The original functions admission_controller.updateLabels(), util.updatePodLabel() can't reflect the behaviors anymore, so I renamed it. Please let me know if we have better name for those two functions.
- When state aware annotation set but can't be converted to boolean, I'll return false without check label's value. The reason is we're not expecting the state aware policy be set in label. (refer this test code)
- There will be another PR to update this doc

codecov · 2023-09-16T05:25:20Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (27ce47c) 71.32% compared to head (4305c07) 71.46%.

Files	Patch %	Lines
pkg/admission/util.go	95.83%	2 Missing and 1 partial ⚠️
pkg/admission/admission_controller.go	95.23%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
+ Coverage   71.32%   71.46%   +0.14%     
==========================================
  Files          43       43              
  Lines        7592     7679      +87     
==========================================
+ Hits         5415     5488      +73     
- Misses       1976     1988      +12     
- Partials      201      203       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

FrankYang0529

Need some minor changes. Thanks for the contribution.

pkg/common/utils/utils.go

pkg/cache/placeholder.go

pkg/admission/util.go

craigcondit · 2023-09-19T15:01:32Z

We need to be very, very careful about this implementation. To support backwards compatibility, we need to honor labels for the foreseeable future. In the admission controller, we should do the following:

Check for the annotation and use it if present
If the annotation is not present, check for the label and copy the value into the annotation

In the shim, we need to then check for the annotation first, and then fall back to the label (or in the case of applicationID, use Spark ApplicationID if neither is present). We can't simply look at the annotations alone, as users may not use the admission controller at all, and there may also be existing long-running pods that only contain labels.

chenyulin0719 · 2023-09-20T12:26:09Z

Thanks @FrankYang0529 , @craigcondit 's comment.

I'll update a new version which will keep the original labels for backforward, and create missing annotations from corresponding labels.

chenyulin0719 · 2023-09-22T06:52:38Z

Hi @FrankYang0529, @craigcondit, @wilfred-s

I just uploaded a newer version of commit, could I have your review? The existing labels are still kept in admission controller and will be added to annotation if the corresponding tag are not existing in pod.

Below is the newer change list of the whole PR:

Get queue name from annotation first. (GetQueueNameFromPod)
(Also add default queue in parameter)
Get isStateAwareDisabled from annotation first (isStateAwareDisabled)
Move labels applicationId/queue in newPlaceholder to annotation (newPlaceholder)
Add new annotation 'yunikorn.apache.org/disable-state-aware' to replace equivalent label.
Rename updateLabels to updateApplicationInfo in AdmissionController
(I made some changes here, updating label/annotations to patch separately so we can easily remove labels in the future.)
Rename util.updatePodLabel to util.getNewApplicationInfo
(I made some changes here, checking new label/annotation and the existence in pod, return the to-be added list for admission controller.)

Since this PR modified the core part of admission controller, I might need your help to review, expecially for # 5 and # 6.
Please kindly let me know if I have any misunderstadnings.

chenyulin0719 · 2023-09-26T03:53:50Z

Hi @FrankYang0529 , after dived into the error in CI (v1.24.15)
I found there have a racing issue in e2e test. I opened a Jira issue here:

YUNIKORN-1998 Stale AdmissionControllerConf was used in e2e test

Could you help to re-trigger CI? I don't think it will happen every time

chenyulin0719 · 2023-11-03T07:33:16Z

Add screenshot to make this PR more readable.
Before vs After result. (Pod with existing applicationId, queue labels.)

chenyulin0719 · 2023-11-03T07:34:18Z

Before vs After result. (Pod with no label.)

pbacsko

First round from me.

pkg/admission/util.go

pkg/admission/admission_controller.go

pbacsko · 2023-11-19T22:00:29Z

pkg/admission/util.go

-	for k, v := range existingLabels {
-		result[k] = v
-	}
+func getNewApplicationInfo(pod *v1.Pod, namespace string, generateUniqueAppIds bool, defaultQueueName string) (map[string]string, map[string]string) {


In this function, I think it's useful to issue a warning to the user that labels will be deprecated in a later release, so they must adapt their app submission logic to this. It's very easy to forget about this and we must reduce the surprise factor.

What I'd do: return an extra bool from getApplicationIDFromPod(), getDisableStateAwareFromPod(), utils.GetQueueNameFromPod() to indicate whether a source was a label. If there's at least one of them is true, log something useful:

if appIdLabel || stateAwareFromLabel || queueNameFromLabel { log.Log(log.Admission).Warn("Pod contains label for Yunikorn configuration. This is deprecated and will be ignored in a future release. Please migrate to annotation-based configuration.", zap.Bool("appId from label", appIdLabel), zap.Bool("stateAware from label", stateAwareFromLabel), zap.Bool("queue name from label", queueNameFromLabel)) }

Add deprecated warning if applicationInfo is from labels (cf71eef)

Return extra bool 'isFromLabel' as suggested

I don't think we can even deprecate the labels (especially applicationId) -- they are far too entrenched in the existing ecosystem (and arguably applicationId should be a label anyway to allow querying pods by applicationId). I think our best best is simply to use the annotation if it exists, and fallback to the label if it does not.

It make sense to me, let's discuss the expecting AM behavior in below discussion.

chenyulin0719 · 2023-11-20T15:33:09Z

Thanks @pbacsko's review, I've updated a newer version based on the feedbacks.
The e2e test passed in my local env. Appreciate any advice.

chenyulin0719 · 2024-01-26T13:17:14Z

Hi @pbacsko, sorry for late update. I just rebase the commits and solve some conflicts.

pbacsko · 2024-01-26T13:23:46Z

@chenyulin0719 thanks, I'll take a look shortly.

pbacsko · 2024-01-26T13:49:20Z

pkg/admission/util.go

+	queueName, isQueueNameFromLabel := utils.GetQueueNameFromPod(pod, defaultQueueName)
+
+	if isAppIdFromLabel || isDisableStateAwareFromLabel || isQueueNameFromLabel {
+		log.Log(log.Admission).Warn("Pod contains label for Yunikorn configuration. This is deprecated and will be ignored in a future release. Please migrate to annotation-based configuration.",


I've already talked to @craigcondit about this, we might not be in a position to ever remove this, it's going to be too dangerous. Also, certain settings actually should be specified as labels because that allows pods to be grouped/selected based on certain criteria. Let's think about it.

I agree with the benefits of using label in terms of grouping and selecting.
Deprecate it might result in users unable to implement alternative solution.

Regarding this PR:

always prefer annotation above label.

Move labels to annotation, but keep existing labels before deprecation.

Before making any change, I think we need to align the scope again.
Below are my questions if we are not going to deprecate the labels:

Will AM patch missing attributes to label only? Or patch to label and annotation both?
-> I think for 'applicationID' and 'queue', we should patch to both.

How about the existing label "disableStateAware"?
-> I think we can just change it to annotation because it's only be set in AM.
-> This label could be deprecated. (Or suggest not using it)

If we have a different applicationID set in label/annotation, do we still prefer annotation above label.
-> I think the answer is yes for consistency, we must guide user not to set different value if they require labels for selecting/grouping.

If only annotations were set in pod, should AM patch it(app-id, queue) to label?
-> Yes, align with my answer of question 1

If only labels were set in pod, should AM patch it(applicationId, queue) to annoataion?
-> Yes, align with my answer of question 1

My summary of new version:

Always prefer annotation above labels

AM ensure applicationID/queue existing in label and annotation.

Change label "disableStateAware" to annotation.

@pegasas, do you have any feedback?

Also needs @wilfred-s 's review too.

Sorry, I just pinged the wrong person.

@pbacsko,@craigcondit , do you have any feedback for the new version of AM behaviror?

@pbacsko, @craigcondit ,
After reconsideration, if deprecated labels are no longer needed, I can revert most of the AM changes to keep this PR simple. We can maintain the previous UpdateLabel behavior for AM. Just make sure to always get annotation before labels when accessing the value.

Will update a version with this idea, please let me know if you have any concern.

I'd like to get @wilfred-s to weigh in on this one.

chenyulin0719 · 2024-03-02T02:03:31Z

Close it as new policy for labels and annotations is defined in YUNIKORN-1351.

steinsgateted assigned chenyulin0719 Sep 16, 2023

steinsgateted requested review from craigcondit, wilfred-s and FrankYang0529 and removed request for craigcondit September 16, 2023 05:13

FrankYang0529 requested changes Sep 16, 2023

View reviewed changes

pkg/common/utils/utils.go Outdated Show resolved Hide resolved

pkg/cache/placeholder.go Show resolved Hide resolved

pkg/admission/util.go Outdated Show resolved Hide resolved

FrankYang0529 reviewed Sep 16, 2023

View reviewed changes

pkg/admission/util.go Outdated Show resolved Hide resolved

chenyulin0719 requested a review from FrankYang0529 September 18, 2023 15:13

pbacsko requested changes Nov 19, 2023

View reviewed changes

chenyulin0719 force-pushed the YUNIKORN-1351 branch from 5da2d22 to f48e089 Compare November 20, 2023 15:12

[YUNIKORN-1351] alway prefer annotations above labels

4305c07

chenyulin0719 force-pushed the YUNIKORN-1351 branch from 7beb9aa to 4305c07 Compare January 26, 2024 09:18

chenyulin0719 requested a review from pbacsko January 26, 2024 13:15

pbacsko reviewed Jan 26, 2024

View reviewed changes

chenyulin0719 closed this Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[YUNIKORN-1351] alway prefer annotations above labels #677

[YUNIKORN-1351] alway prefer annotations above labels #677

chenyulin0719 commented Sep 15, 2023 •

edited

codecov bot commented Sep 16, 2023 •

edited

FrankYang0529 left a comment

craigcondit commented Sep 19, 2023 •

edited

chenyulin0719 commented Sep 20, 2023 •

edited

chenyulin0719 commented Sep 22, 2023 •

edited

chenyulin0719 commented Sep 26, 2023

chenyulin0719 commented Nov 3, 2023

chenyulin0719 commented Nov 3, 2023

pbacsko left a comment

pbacsko Nov 19, 2023

chenyulin0719 Nov 20, 2023

craigcondit Jan 26, 2024 •

edited

chenyulin0719 Jan 27, 2024

chenyulin0719 commented Nov 20, 2023

chenyulin0719 commented Jan 26, 2024 •

edited

pbacsko commented Jan 26, 2024

pbacsko Jan 26, 2024

chenyulin0719 Jan 26, 2024

chenyulin0719 Jan 26, 2024

chenyulin0719 Jan 27, 2024 •

edited

chenyulin0719 Feb 2, 2024

craigcondit Feb 2, 2024

chenyulin0719 commented Mar 2, 2024

[YUNIKORN-1351] alway prefer annotations above labels #677

[YUNIKORN-1351] alway prefer annotations above labels #677

Conversation

chenyulin0719 commented Sep 15, 2023 • edited

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

codecov bot commented Sep 16, 2023 • edited

Codecov Report

FrankYang0529 left a comment

Choose a reason for hiding this comment

craigcondit commented Sep 19, 2023 • edited

chenyulin0719 commented Sep 20, 2023 • edited

chenyulin0719 commented Sep 22, 2023 • edited

chenyulin0719 commented Sep 26, 2023

chenyulin0719 commented Nov 3, 2023

chenyulin0719 commented Nov 3, 2023

pbacsko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigcondit Jan 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenyulin0719 commented Nov 20, 2023

chenyulin0719 commented Jan 26, 2024 • edited

pbacsko commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenyulin0719 Jan 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenyulin0719 commented Mar 2, 2024

chenyulin0719 commented Sep 15, 2023 •

edited

codecov bot commented Sep 16, 2023 •

edited

craigcondit commented Sep 19, 2023 •

edited

chenyulin0719 commented Sep 20, 2023 •

edited

chenyulin0719 commented Sep 22, 2023 •

edited

craigcondit Jan 26, 2024 •

edited

chenyulin0719 commented Jan 26, 2024 •

edited

chenyulin0719 Jan 27, 2024 •

edited