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

[YUNIKORN-2847] Documentation: Update new label/annotation changes before 1.6.0 release #468

Closed
wants to merge 2 commits into from

Conversation

chenyulin0719
Copy link
Contributor

@chenyulin0719 chenyulin0719 commented Sep 9, 2024

What is this PR for?

Add below descriptions:

  1. Add new canonical representation labels: yunikorn.apache.org/app-id, yunikorn.apache.org/queue, also update to the latest prioirty. Add note for inconsistent pod rejection in 1.7.0.
  2. Mark the non-canonical representation as 'Classic' since we're not going to deprecate it.
  3. Use the full annotation name in annotation tables. (ex: task-groups to yunikorn.apache.org/task-groups )
  4. Update the description of spark-app-selector. Add a link to SparkOperator.

What type of PR is it?

  • - Improvement

What is the Jira issue?

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

How should this be tested?

pnpm start

Questions:

NA

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

In general, good. Some grammar / wording suggestions for clarity.

YuniKorn utilizes several Kubernetes labels and annotations to support various features.

Since YuniKorn 1.6.0, we have provided canonical representations for defining applicationID and queue in labels under the namespace `yunikorn.apache.org`. Using these labels can benefit from the Kubernetes filtering and grouping capabilities.
The non-canonical(Classic) representation will not be deprecated and will continue to be supported in future releases.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: space before parenthesis.
Suggestion: Perhaps "legacy" is a better descriptor; it's a bit more negative. Not quite deprecated, but also not preferred. So: "The non-canonical (legacy) representations will not be deprecated..."

| `spark-app-selector` | Alternative method of specifying `applicationId` used by Spark Operator if the label `applicationId` and annotation `yunikorn.apache.org/app-id` are unset. |
| Name | Description |
|-------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `yunikorn.apache.org/app-id` | Associates this pod with an application. This is the canonical representation and it is recommended to use it.<br/><br/>The priority of applicationID is determined in the following order: <ol><li>Label `yunikorn.apache.org/app-id`</li><li>Annotation `yunikorn.apache.org/app-id`</li><li>Label `applicationId`</li><li>Label `spark-app-selector`</li></ol>**Note**: Pod has inconsistent application metadata will be rejected in 1.7.0. | |
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "The priority of applicationID" -> "The priority of application ID" (add space as this is now a generic term referring to all the various representations).

Grammar: "Pod has inconsistent application metadata" -> "Pods with inconsistent application metadata"

| Name | Description |
|-------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `yunikorn.apache.org/app-id` | Associates this pod with an application. This is the canonical representation and it is recommended to use it.<br/><br/>The priority of applicationID is determined in the following order: <ol><li>Label `yunikorn.apache.org/app-id`</li><li>Annotation `yunikorn.apache.org/app-id`</li><li>Label `applicationId`</li><li>Label `spark-app-selector`</li></ol>**Note**: Pod has inconsistent application metadata will be rejected in 1.7.0. | |
| `yunikorn.apache.org/queue` | Selects the YuniKorn queue this application should be scheduled in. This is the canonical representation and it is recommended to use it.<br/><br/>Queue name should comply with [Kubernetes Syntax and character set](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) and also with [Partition and Queue Configuration](queue_config#queues). This may be ignored if a placement policy is in effect.<br/><br/>The priority of queue name is determined in the following order: <ol><li>Label `yunikorn.apache.org/queue`</li><li>Annotation `yunikorn.apache.org/queue`</li><li>Label `queue`</li></ol>**Note**: Pod has inconsistent queue metadata will be rejected in 1.7.0. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammar: "Pod has inconsistent queue metadata" -> "Pods with inconsistent queue metadata"

|-------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `yunikorn.apache.org/app-id` | Associates this pod with an application. This is the canonical representation and it is recommended to use it.<br/><br/>The priority of applicationID is determined in the following order: <ol><li>Label `yunikorn.apache.org/app-id`</li><li>Annotation `yunikorn.apache.org/app-id`</li><li>Label `applicationId`</li><li>Label `spark-app-selector`</li></ol>**Note**: Pod has inconsistent application metadata will be rejected in 1.7.0. | |
| `yunikorn.apache.org/queue` | Selects the YuniKorn queue this application should be scheduled in. This is the canonical representation and it is recommended to use it.<br/><br/>Queue name should comply with [Kubernetes Syntax and character set](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) and also with [Partition and Queue Configuration](queue_config#queues). This may be ignored if a placement policy is in effect.<br/><br/>The priority of queue name is determined in the following order: <ol><li>Label `yunikorn.apache.org/queue`</li><li>Annotation `yunikorn.apache.org/queue`</li><li>Label `queue`</li></ol>**Note**: Pod has inconsistent queue metadata will be rejected in 1.7.0. |
| [Classic]<br/>`applicationId` | It is equivalent to the label `yunikorn.apache.org/app-id`, and we recommend using it. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's switch from "[Classic] to [LEGACY], and reword descriptions as follows:

Equivalent to the preferred label yunikorn.apache.org/app-id.

| `yunikorn.apache.org/app-id` | Associates this pod with an application. This is the canonical representation and it is recommended to use it.<br/><br/>The priority of applicationID is determined in the following order: <ol><li>Label `yunikorn.apache.org/app-id`</li><li>Annotation `yunikorn.apache.org/app-id`</li><li>Label `applicationId`</li><li>Label `spark-app-selector`</li></ol>**Note**: Pod has inconsistent application metadata will be rejected in 1.7.0. | |
| `yunikorn.apache.org/queue` | Selects the YuniKorn queue this application should be scheduled in. This is the canonical representation and it is recommended to use it.<br/><br/>Queue name should comply with [Kubernetes Syntax and character set](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) and also with [Partition and Queue Configuration](queue_config#queues). This may be ignored if a placement policy is in effect.<br/><br/>The priority of queue name is determined in the following order: <ol><li>Label `yunikorn.apache.org/queue`</li><li>Annotation `yunikorn.apache.org/queue`</li><li>Label `queue`</li></ol>**Note**: Pod has inconsistent queue metadata will be rejected in 1.7.0. |
| [Classic]<br/>`applicationId` | It is equivalent to the label `yunikorn.apache.org/app-id`, and we recommend using it. |
| [Classic]<br/>`queue` | It is equivalent to the label `yunikorn.apache.org/queue`, and we recommend using it. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's switch from "[Classic] to [LEGACY], and reword descriptions as follows:

Equivalent to the preferred label yunikorn.apache.org/queue.

| `yunikorn.apache.org/queue` | Selects the YuniKorn queue this application should be scheduled in. This is the canonical representation and it is recommended to use it.<br/><br/>Queue name should comply with [Kubernetes Syntax and character set](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) and also with [Partition and Queue Configuration](queue_config#queues). This may be ignored if a placement policy is in effect.<br/><br/>The priority of queue name is determined in the following order: <ol><li>Label `yunikorn.apache.org/queue`</li><li>Annotation `yunikorn.apache.org/queue`</li><li>Label `queue`</li></ol>**Note**: Pod has inconsistent queue metadata will be rejected in 1.7.0. |
| [Classic]<br/>`applicationId` | It is equivalent to the label `yunikorn.apache.org/app-id`, and we recommend using it. |
| [Classic]<br/>`queue` | It is equivalent to the label `yunikorn.apache.org/queue`, and we recommend using it. |
| `spark-app-selector` | It is equivalent to the label `yunikorn.apache.org/app-id`.<br/><br/>It is automatically attached by third-party Spark jobs triggered by the [Spark Operator](https://github.com/kubeflow/spark-operator). YuniKorn internally supports this label as one of the sources of applicationID. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Wording: "YuniKorn internally supports this label as one of the sources of applicationID." -> "YuniKorn uses this label to define application ID if no other metadata exists."

This more clearly conveys what we intend to do in 1.7.


| Name | Description |
|-------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| [Classic]<br/>`yunikorn.apache.org/app-id` | It is equivalent to the label `yunikorn.apache.org/app-id`, and we recommend using it. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's switch from "[Classic] to [LEGACY], and reword descriptions as follows:

Equivalent to the preferred label yunikorn.apache.org/app-id.

| Name | Description |
|-------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| [Classic]<br/>`yunikorn.apache.org/app-id` | It is equivalent to the label `yunikorn.apache.org/app-id`, and we recommend using it. |
| [Classic]<br/>`yunikorn.apache.org/queue` | It is equivalent to the label `yunikorn.apache.org/queue`, and we recommend using it. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's switch from "[Classic] to [LEGACY], and reword descriptions as follows:

Equivalent to the preferred label yunikorn.apache.org/queue.

@chenyulin0719
Copy link
Contributor Author

@craigcondit Thanks for the review. Updated this PR with the suggestions.

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

+1 LGTM.

github-actions bot pushed a commit that referenced this pull request Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants