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.

Sign up for GitHub

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

Jump to bottom

[YUNIKORN-2267] add docs of using BinaryData to configure scheduler #393

Closed
wants to merge 1 commit into from
Closed

[YUNIKORN-2267] add docs of using BinaryData to configure scheduler #393

wants to merge 1 commit into from

Conversation

FrankYang0529
Copy link
Member

What is this PR for?

Add binaryData config example.

What type of PR is it?

  • - Documentation

What is the Jira issue?

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

craigcondit
craigcondit requested changes Jan 26, 2024
View reviewed changes
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.

Minor changes, otherwise looks good.

docs/user_guide/service_config.md Outdated
@@ -977,6 +977,37 @@ Example:
admissionController.accessControl.externalGroups: "^sales$,^marketing$,^admin-"
```

### Compresion setting

The data in ConfigMap cannot exceed 1 MiB. YuniKorn supports Gzip algorithm to decompress data in the `binaryData` field.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "YuniKorn supports the gzip algorithm" instead of "YuniKorn supports Gzip algorithm" (gzip is lowercased, and should have the article "the" preceding it).

docs/user_guide/service_config.md Outdated
@@ -977,6 +977,37 @@ Example:
admissionController.accessControl.externalGroups: "^sales$,^marketing$,^admin-"
```

### Compresion setting
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Compresion setting -> Using compressed values

docs/user_guide/service_config.md Outdated
### Compresion setting

The data in ConfigMap cannot exceed 1 MiB. YuniKorn supports Gzip algorithm to decompress data in the `binaryData` field.
If a key ends with `.gz`, YuniKorn decodes the value by base64 and decompress it by Gzip algorithm.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about: "If a key ends with .gz. YuniKorn will treat the value as gzip-compressed data and decompress it automatically. The base64 encoding is automatically "

docs/user_guide/service_config.md Outdated

The data in ConfigMap cannot exceed 1 MiB. YuniKorn supports Gzip algorithm to decompress data in the `binaryData` field.
If a key ends with `.gz`, YuniKorn decodes the value by base64 and decompress it by Gzip algorithm.
If both `queues.yaml` setting is set in `data` field and `queues.yaml.gz` setting is set in `binaryData` field, the value in `binaryData` field will be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead: If a value is set in both the data and binaryData sections, the value in the binaryData section will be used.

Copy link
Member

Choose a reason for hiding this comment

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

If a value is set in both the data and binaryData sections, the value in the binaryData section will be used.

we can use "the queues.yaml will get override by binaryData" as a example.

Also, that is unrelated issue, but should we add WARN logging for it?

Copy link
Contributor

@craigcondit craigcondit Jan 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We don't want to log a warning because it's not uncommon that a default queues.yaml is provided via Helm in the yunikorn-defaults configmap that is then overridden by a binaryData version in yunikorn-configs. It's not possible to remove a config value so having both would create an "unfixable" warning.

Copy link
Member

Choose a reason for hiding this comment

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

it's not uncommon that a default queues.yaml is provided via Helm in the yunikorn-defaults configmap that is then overridden by a binaryData version in yunikorn-configs.

make sense to me. thanks

chia7712
chia7712 reviewed Jan 27, 2024
View reviewed changes
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@FrankYang0529 nice docs. some trivial comments below.

docs/user_guide/service_config.md Outdated

The data in ConfigMap cannot exceed 1 MiB. YuniKorn supports Gzip algorithm to decompress data in the `binaryData` field.
If a key ends with `.gz`, YuniKorn decodes the value by base64 and decompress it by Gzip algorithm.
If both `queues.yaml` setting is set in `data` field and `queues.yaml.gz` setting is set in `binaryData` field, the value in `binaryData` field will be used.
Copy link
Member

Choose a reason for hiding this comment

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

If a value is set in both the data and binaryData sections, the value in the binaryData section will be used.

we can use "the queues.yaml will get override by binaryData" as a example.

Also, that is unrelated issue, but should we add WARN logging for it?

docs/user_guide/service_config.md Outdated
@@ -243,7 +243,7 @@ Sets the tolerations for the YuniKorn scheduler pod.

Default: `[]`

Example:
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to fix those unrelated code in separate jira. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I disable trim trailing whitespace in my IDE, so we don't have unrelated change in this PR. Thanks!

@FrankYang0529
Copy link
Member Author

Hi @chia7712 and @craigcondit, thanks for reviewing. I resolved all comments. Could you help to take a look again? Thanks.

chia7712
chia7712 approved these changes Feb 7, 2024
View reviewed changes
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM

@chia7712 chia7712 closed this in 0f84c83 Feb 7, 2024
github-actions bot pushed a commit that referenced this pull request Feb 7, 2024
@yunikorn-bot
Auto refresh: [YUNIKORN-2267] add docs of using BinaryData to configu…
c997cb4 
…re scheduler (#393)
github-actions bot pushed a commit to chia7712/yunikorn-site that referenced this pull request Feb 9, 2024
@yunikorn-bot
Auto refresh: [YUNIKORN-2267] add docs of using BinaryData to configu…
f70d318 
…re scheduler (apache#393)
@FrankYang0529 FrankYang0529 deleted the YUNIKORN-2267 branch February 11, 2024 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@craigcondit craigcondit craigcondit requested changes

@chia7712 chia7712 chia7712 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FrankYang0529 @chia7712 @craigcondit