[ https://issues.apache.org/jira/browse/YUNIKORN-1351?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17821767#comment-17821767 ]
Craig Condit edited comment on YUNIKORN-1351 at 2/28/24 4:57 PM: ----------------------------------------------------------------- My thoughts on V2: We shouldn't introduce a configuration flag for the admission controller; that's going to cause more confusion than anything else. Instead we should strive to be consistent. The admission controller currently *only* sets the Application ID, and then only if if it is auto-generated. Unfortunately, it does set the non-canonical label version. We should update this to generate both the {{yunikorn.apache.org/app-id}} *label* and {{applicationId}} *label* and in 1.7.0, we can switch to generating only the {{yunikorn.apache.org/app-id}} {*}label{*}. We can infer that if a user has not supplied an Application ID explicitly, they are not concerned with where that metadata gets stored. We need to generate both in 1.6.0 to avoid an upgrade problem where the admission controller gets started first, and starts generating labels which the 1.5.0 YuniKorn scheduler does not understand. Once 1.7.0 is released, we can assume that any running YuniKorn schedulers *do* understand the new labels. In all cases, we need to have both the scheduler and admission controller read from all possible locations. For {{{}disableStateAware{}}}, we don't need to do anything; state-aware scheduling will be gone by the 1.6.0 release. We absolutely *must not* change {{schedulingPolicyParameters}} (or other configurations) to the new format. This will just lead to mass confusion, as this is codified in external tooling and we would now have multiple legal namespaced identifiers with little clarity as to which is the correct one. Just to be sure I'm interpreting your table correctly, I'm going to restate what labels/ annotations we will support in a slightly different format. For each entry, I'll list the canonical representation first (and bolded), followed by the compatibility versions: * Application ** *{{yunikorn.apache.org/app-id}} [label]* – generated by AM in 1.6.0+ ** {{yunikorn.apache.org/app-id}} [annotation] ** {{applicationId}} [label] – generated by AM in 1.6.0, not in 1.7.0+ ** {{spark-app-selector}} [label] – required for compatibility with Spark apps * Queue ** *{{yunikorn.apache.org/queue}} [label]* ** {{yunikorn.apache.org/queue}} [annotation] ** {{queue}} [label] * DisableStateAware flag – feature to be removed in 1.6.0, *remove documentation* ** [none] * Placeholder flag – used internally by scheduler only, *remove documentation* ** *{{internal.yunikorn.apache.org/placeholder}} [annotation]* – generate in 1.6.0+ ** {{yunikorn.apache.org/placeholder}} [annotation] – parse in 1.6.0, remove in 1.7.0 ** {{placeholder}} [label] – parse in 1.6.0, remove in 1.7.0 * Task Group Name – publicly documented interface for external applications ** *{{yunikorn.apache.org/task-group-name}} [annotation]* * Task Groups – publicly documented interface for external applications ** *{{yunikorn.apache.org/task-groups}} [annotation]* * Scheduling Policy Parameters – publicly documented interface for external applications ** *{{yunikorn.apache.org/schedulingPolicyParameters}} [annotation]* * Allow-preemption flag – publicly documented interface for Pod / PriorityClass ** *{{yunikorn.apache.org/allow-preemption}} [annotation]* was (Author: ccondit): My thoughts on V2: We shouldn't introduce a configuration flag for the admission controller; that's going to cause more confusion than anything else. Instead we should strive to be consistent. The admission controller currently *only* sets the Application ID, and then only if if it is auto-generated. Unfortunately, it does set the non-canonical label version, which is unfortunate. We should update this to generate both the {{yunikorn.apache.org/app-id}} *label* and {{applicationId}} *label* and in 1.7.0, we can switch to generating only the {{yunikorn.apache.org/app-id}} {*}label{*}. We can infer that if a user has not supplied an Application ID explicitly, they are not concerned with where that metadata gets stored. We need to generate both in 1.6.0 to avoid an upgrade problem where the admission controller gets started first, and starts generating labels which the 1.5.0 YuniKorn scheduler does not understand. Once 1.7.0 is released, we can assume that any running YuniKorn schedulers *do* understand the new labels. In all cases, we need to have both the scheduler and admission controller read from all possible locations. For {{{}disableStateAware{}}}, we don't need to do anything; state-aware scheduling will be gone by the 1.6.0 release. We absolutely *must not* change {{schedulingPolicyParameters}} (or other configurations) to the new format. This will just lead to mass confusion as this is codified in external tooling and having multiple legal namespaced identifiers with little clarity as to which is the correct one. Just to be sure I'm interpreting your table correctly, I'm going to restate what labels/ annotations we will support in a slightly different format. For each entry, I'll list the canonical representation first (and bolded), followed by the compatibility versions: * Application ** *{{yunikorn.apache.org/app-id}} [label]* – generated by AM in 1.6.0+ ** {{yunikorn.apache.org/app-id}} [annotation] ** {{applicationId}} [label] – generated by AM in 1.6.0, not in 1.7.0+ ** {{spark-app-selector}} [label] – required for compatibility with Spark apps * Queue ** *{{yunikorn.apache.org/queue}} [label]* ** {{yunikorn.apache.org/queue}} [annotation] ** {{queue}} [label] * DisableStateAware flag – feature to be removed in 1.6.0, *remove documentation* ** [none] * Placeholder flag – used internally by scheduler only, *remove documentation* ** *{{internal.yunikorn.apache.org/placeholder}} [annotation]* – generate in 1.6.0+ ** {{yunikorn.apache.org/placeholder}} [annotation] – parse in 1.6.0, remove in 1.7.0 ** {{placeholder}} [label] – parse in 1.6.0, remove in 1.7.0 * Task Group Name – publicly documented interface for external applications ** *{{yunikorn.apache.org/task-group-name}} [annotation]* * Task Groups – publicly documented interface for external applications ** *{{yunikorn.apache.org/task-groups}} [annotation]* * Scheduling Policy Parameters – publicly documented interface for external applications ** *{{yunikorn.apache.org/schedulingPolicyParameters}} [annotation]* * Allow-preemption flag – publicly documented interface for Pod / PriorityClass ** *{{yunikorn.apache.org/allow-preemption}} [annotation]* > Define policy for when to use annotations vs. labels > ---------------------------------------------------- > > Key: YUNIKORN-1351 > URL: https://issues.apache.org/jira/browse/YUNIKORN-1351 > Project: Apache YuniKorn > Issue Type: Improvement > Components: shim - kubernetes > Reporter: Wilfred Spiegelenburg > Assignee: Yu-Lin Chen > Priority: Major > Labels: newbie, pull-request-available > Attachments: [Design Doc] Define Usage of Labels and Annotations in > YuniKorn.pdf, [Design Doc][v2] Define policy for when to use annotations vs > labels.pdf > > > Currently, we have very inconsistent use of labels vs. annotations in > YuniKorn. Additionally, some values are namespaced under yunikorn.apache.org > while some are not. We will likely need to keep legacy values around for > backwards compatibility but we should define a strategy for defining > canonical representations of this metadata as well as a developer-oriented > pollcy around when to use labels vs. annotations. -- This message was sent by Atlassian Jira (v8.20.10#820010) --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@yunikorn.apache.org For additional commands, e-mail: issues-h...@yunikorn.apache.org