Re: [PR] [YUNIKORN-2811] Warn if a pod has inconsistent metadata in the shim [yunikorn-k8shim]
chenyulin0719 closed pull request #964: [YUNIKORN-2811] Warn if a pod has inconsistent metadata in the shim URL: https://github.com/apache/yunikorn-k8shim/pull/964 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@yunikorn.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [YUNIKORN-2811] Warn if a pod has inconsistent metadata in the shim [yunikorn-k8shim]
chenyulin0719 commented on PR #964: URL: https://github.com/apache/yunikorn-k8shim/pull/964#issuecomment-2817014248 Create YUNIKORN-3065 to improve test coverage. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@yunikorn.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [YUNIKORN-2811] Warn if a pod has inconsistent metadata in the shim [yunikorn-k8shim]
wilfred-s commented on code in PR #964:
URL: https://github.com/apache/yunikorn-k8shim/pull/964#discussion_r2045790134
##
pkg/cache/application.go:
##
@@ -392,6 +392,9 @@ func (app *Application) scheduleTasks(taskScheduleCondition
func(t *Task) bool)
if taskScheduleCondition(task) {
// for each new task, we do a sanity check before
moving the state to Pending_Schedule
if err := task.sanityCheckBeforeScheduling(); err ==
nil {
+ // check inconsistent pod metadata before
submitting the task
+ task.checkPodMetadataBeforeScheduling()
+
Review Comment:
One remark: we should have tests that call the
`checkPodMetadataBeforeScheduling()` method.
The current tests only call the utility functions and leave out the higher
level of checks. Can be added in a follow up jira
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2811] Warn if a pod has inconsistent metadata in the shim [yunikorn-k8shim]
chenyulin0719 commented on code in PR #964:
URL: https://github.com/apache/yunikorn-k8shim/pull/964#discussion_r2030221797
##
pkg/cache/application.go:
##
@@ -392,6 +392,9 @@ func (app *Application) scheduleTasks(taskScheduleCondition
func(t *Task) bool)
if taskScheduleCondition(task) {
// for each new task, we do a sanity check before
moving the state to Pending_Schedule
if err := task.sanityCheckBeforeScheduling(); err ==
nil {
+ // check inconsistent pod metadata before
submitting the task
+ task.checkPodMetadataBeforeScheduling()
+
Review Comment:
This is the key change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2811] Warn if a pod has inconsistent metadata in the shim [yunikorn-k8shim]
chenyulin0719 commented on code in PR #964:
URL: https://github.com/apache/yunikorn-k8shim/pull/964#discussion_r2030221199
##
pkg/cache/task.go:
##
@@ -533,28 +534,38 @@ func (task *Task) releaseAllocation() {
// this reduces the scheduling overhead by blocking such
// request away from the core scheduler.
func (task *Task) sanityCheckBeforeScheduling() error {
- // After version 1.7.0, we should reject the task whose pod is unbound
and has conflicting metadata.
- if !utils.PodAlreadyBound(task.pod) {
- if err := utils.CheckAppIdInPod(task.pod); err != nil {
- log.Log(log.ShimCacheTask).Warn("Pod has inconsistent
application metadata and may be rejected in a future YuniKorn release",
- zap.String("appID", task.applicationID),
- zap.String("podName", task.pod.Name),
- zap.String("error", err.Error()))
Review Comment:
Since we're not going to reject pod if pod has inconsistent metadata, I
remove the metadata check from the sanity check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2811] Warn if a pod has inconsistent metadata in the shim [yunikorn-k8shim]
chenyulin0719 commented on code in PR #964:
URL: https://github.com/apache/yunikorn-k8shim/pull/964#discussion_r2030221797
##
pkg/cache/application.go:
##
@@ -392,6 +392,9 @@ func (app *Application) scheduleTasks(taskScheduleCondition
func(t *Task) bool)
if taskScheduleCondition(task) {
// for each new task, we do a sanity check before
moving the state to Pending_Schedule
if err := task.sanityCheckBeforeScheduling(); err ==
nil {
+ // check inconsistent pod metadata before
submitting the task
+ task.checkPodMetadataBeforeScheduling()
+
Review Comment:
Note: This is the start point of reviewing.
##
pkg/cache/task.go:
##
@@ -533,28 +534,38 @@ func (task *Task) releaseAllocation() {
// this reduces the scheduling overhead by blocking such
// request away from the core scheduler.
func (task *Task) sanityCheckBeforeScheduling() error {
- // After version 1.7.0, we should reject the task whose pod is unbound
and has conflicting metadata.
- if !utils.PodAlreadyBound(task.pod) {
- if err := utils.CheckAppIdInPod(task.pod); err != nil {
- log.Log(log.ShimCacheTask).Warn("Pod has inconsistent
application metadata and may be rejected in a future YuniKorn release",
- zap.String("appID", task.applicationID),
- zap.String("podName", task.pod.Name),
- zap.String("error", err.Error()))
Review Comment:
Note: Since we're not going to reject pod if pod has inconsistent metadata,
I remove the metadata check from the sanity check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2811] Warn if a pod has inconsistent metadata in the shim [yunikorn-k8shim]
chenyulin0719 commented on code in PR #964:
URL: https://github.com/apache/yunikorn-k8shim/pull/964#discussion_r2030221797
##
pkg/cache/application.go:
##
@@ -392,6 +392,9 @@ func (app *Application) scheduleTasks(taskScheduleCondition
func(t *Task) bool)
if taskScheduleCondition(task) {
// for each new task, we do a sanity check before
moving the state to Pending_Schedule
if err := task.sanityCheckBeforeScheduling(); err ==
nil {
+ // check inconsistent pod metadata before
submitting the task
+ task.checkPodMetadataBeforeScheduling()
+
Review Comment:
This is the start point of reviewing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2811] Warn if a pod has inconsistent metadata in the shim [yunikorn-k8shim]
chenyulin0719 commented on code in PR #964:
URL: https://github.com/apache/yunikorn-k8shim/pull/964#discussion_r2030221797
##
pkg/cache/application.go:
##
@@ -392,6 +392,9 @@ func (app *Application) scheduleTasks(taskScheduleCondition
func(t *Task) bool)
if taskScheduleCondition(task) {
// for each new task, we do a sanity check before
moving the state to Pending_Schedule
if err := task.sanityCheckBeforeScheduling(); err ==
nil {
+ // check inconsistent pod metadata before
submitting the task
+ task.checkPodMetadataBeforeScheduling()
+
Review Comment:
This is the major change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [YUNIKORN-2811] Warn if a pod has inconsistent metadata in the shim [yunikorn-k8shim]
codecov[bot] commented on PR #964: URL: https://github.com/apache/yunikorn-k8shim/pull/964#issuecomment-2781536350 ## [Codecov](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/964?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report Attention: Patch coverage is `47.2%` with `19 lines` in your changes missing coverage. Please review. > Project coverage is 67.91%. Comparing base [(`02df203`)](https://app.codecov.io/gh/apache/yunikorn-k8shim/commit/02df203223c7c6a464cfd0296309d4285843727b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`60c545f`)](https://app.codecov.io/gh/apache/yunikorn-k8shim/commit/60c545f78d1644ad70de2832793d867f713313ad?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). | [Files with missing lines](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/964?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [pkg/cache/task.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/964?src=pr&el=tree&filepath=pkg%2Fcache%2Ftask.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | 24.00% | [17 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/964?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Additional details and impacted files ```diff @@Coverage Diff @@ ## master #964 +/- ## == - Coverage 67.96% 67.91% -0.05% == Files 72 72 Lines9305 9294 -11 == - Hits 6324 6312 -12 Misses 2766 2766 - Partials 215 216 +1 ``` [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/964?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@yunikorn.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
