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-1948] Introduce a command to validate the content of a give… #634

Closed
wants to merge 3 commits into from

Conversation

yzhangal
Copy link

What is this PR for?

When preparing the queue configuration file, manually, or use a program, a format error can happen. It can be costly to load the file to production and revert.

This Jira introduces a dedicated command to load the input queue config file (queues.yaml etc), and run the same code that production uses to validate the file, and report errors if there are any.

What type of PR is it?

  • - Bug Fix
  • [X ] - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Task

What is the Jira issue?

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

How should this be tested?

Tested locally with queues.yaml file by modifying the content to make it invalid

$ build/queueconfigchecker queues.yaml
2023-08-31T11:25:44.115-0700 ERROR core.config configs/config.go:181 queue configuration validation failed {"error": "guaranteed resource of parent map[memory:3384828493824 vcore:4006191] is smaller than sum of guaranteed resources map[memory:33848285986816 vcore:4006191] of the children for queue xyz"}
github.com/apache/yunikorn-core/pkg/common/configs.ParseAndValidateConfig
/Users/yongjunzhang/code/yunikorn/yunikorn-core/pkg/common/configs/config.go:181
github.com/apache/yunikorn-core/pkg/common/configs.LoadSchedulerConfigFromByteArray
/Users/yongjunzhang/code/yunikorn/yunikorn-core/pkg/common/configs/config.go:154
main.main
/Users/yongjunzhang/code/yunikorn/yunikorn-core/cmd/queueconfigchecker/queueconfigchecker.go:43
runtime.main
/usr/local/go/src/runtime/proc.go:250

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #634 (09a1e2e) into master (6e1e0a3) will decrease coverage by 0.05%.
Report is 3 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #634      +/-   ##
==========================================
- Coverage   77.74%   77.70%   -0.05%     
==========================================
  Files          79       79              
  Lines       13085    13082       -3     
==========================================
- Hits        10173    10165       -8     
- Misses       2585     2589       +4     
- Partials      327      328       +1     
Files Changed Coverage Δ
pkg/common/configs/config.go 100.00% <ø> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yzhangal
Copy link
Author

yzhangal commented Sep 1, 2023

Hi @craigcondit Thanks for looking into. The lint issue is resolved now, but there is a codecov/project check failure,
https://github.com/apache/yunikorn-core/pull/634/checks?check_run_id=16406753455
wonder what need to be done for this case. Thanks.

Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

Mostly small things.

log.Println(err)
os.Exit(2)
}
_, err1 := configs.LoadSchedulerConfigFromByteArray(iv)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for a new variable, just _, err = configs.LoadSchedulerConfigFromByteArray(iv)

os.Exit(1)
}
queueFile := os.Args[1]
iv, err := os.ReadFile(queueFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why it's called iv? I'd just name if conf.

queueFile := os.Args[1]
iv, err := os.ReadFile(queueFile)
if err != nil {
log.Println(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Be a bit more verbose, like log.Printf("Could not read file: %v", err)

}
_, err1 := configs.LoadSchedulerConfigFromByteArray(iv)
if err1 != nil {
log.Println(err1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Be a bit more verbose, like log.Printf("Config validation failed: %v", err)

Copy link
Author

Choose a reason for hiding this comment

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

Many thanks @pbacsko uploaded new rev to address your comments.

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 addition to the changes indicated by @pbacsko also remove the 'Visible by tests' comment from config.go:152 on LoadSchedulerConfigFromByteArray.

@yzhangal
Copy link
Author

yzhangal commented Sep 5, 2023

In addition to the changes indicated by @pbacsko also remove the 'Visible by tests' comment from config.go:152 on LoadSchedulerConfigFromByteArray.

Many thanks @craigcondit uploaded new rev to address your comment.

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.

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.

3 participants