Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix: fix an issue where the wrong action name could not be ignored #3994

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

xieyanke
Copy link
Contributor

@xieyanke xieyanke commented Feb 4, 2025

What type of PR is this?

/kind bug
/area scheduling

What this PR does / why we need it:

fix an issue where the wrong action name could not be ignored

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?


@volcano-sh-bot
Copy link
Contributor

Welcome @xieyanke!

It looks like this is your first PR to volcano-sh/volcano.

Thank you, and welcome to Volcano. 😃

@volcano-sh-bot volcano-sh-bot added kind/bug Categorizes issue or PR as related to a bug. area/scheduling labels Feb 4, 2025
@volcano-sh-bot volcano-sh-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. kind/flake Categorizes issue or PR as related to a flaky test. area/controllers area/cli area/dependency Issues or PRs related to dependency changes area/webhooks area/deploy Issues or PRs related to deploy/helm/build/scripts changes area/documentation documentation of design or user-guide area/performance Issues or PRs related to performance size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/test CI and test related Issues or PRs labels Feb 4, 2025
@xieyanke
Copy link
Contributor Author

xieyanke commented Feb 4, 2025

/assign @k82cn

@bogo-y
Copy link
Contributor

bogo-y commented Feb 6, 2025

I think that should not be ignored.
For example, when a user makes a typo mistake such as typing "allocated" instead of "allocate", it is better to detect and resolve the issue immediately.

@Monokaix Monokaix removed kind/feature Categorizes issue or PR as related to a new feature. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/controllers area/cli area/dependency Issues or PRs related to dependency changes kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. kind/flake Categorizes issue or PR as related to a flaky test. labels Feb 6, 2025
@Monokaix Monokaix removed kind/documentation Categorizes issue or PR as related to documentation. area/test CI and test related Issues or PRs area/webhooks area/deploy Issues or PRs related to deploy/helm/build/scripts changes area/documentation documentation of design or user-guide area/performance Issues or PRs related to performance labels Feb 6, 2025
@Monokaix
Copy link
Member

Monokaix commented Feb 6, 2025

I think that should not be ignored. For example, when a user makes a typo mistake such as typing "allocated" instead of "allocate", it is better to detect and resolve the issue immediately.

Currently there is a log showing the message.

@bogo-y
Copy link
Contributor

bogo-y commented Feb 6, 2025

Currently there is a log showing the message.

There is too much messages when vc-scheduler just start... Maybe we need keep use klog.Errorf to make it highly visible?

@xieyanke
Copy link
Contributor Author

xieyanke commented Feb 6, 2025

Currently there is a log showing the message.

There is too much messages when vc-scheduler just start... Maybe we need keep use klog.Errorf to make it highly visible?

Since actions are configurable, it’s reasonable to allow ignoring user misconfigurations while providing warning-level log messages.
I think your concern can be addressed through another feature "action classification". Some actions, such as "enqueue or allocate" are essential and require action validation, while others can be optionally ignored.

@bogo-y
Copy link
Contributor

bogo-y commented Feb 6, 2025

Currently there is a log showing the message.

There is too much messages when vc-scheduler just start... Maybe we need keep use klog.Errorf to make it highly visible?

Since actions are configurable, it’s reasonable to allow ignoring user misconfigurations while providing warning-level log messages. I think your concern can be addressed through another feature "action classification". Some actions, such as "enqueue or allocate" are essential and require action validation, while others can be optionally ignored.

There is no benefit to keep a wrong action name in configuration file, and if users discovered a mistake like this, they will fix it very easily.
just my personal opinion

@Monokaix
Copy link
Member

Monokaix commented Feb 7, 2025

Currently there is a log showing the message.

There is too much messages when vc-scheduler just start... Maybe we need keep use klog.Errorf to make it highly visible?

Since actions are configurable, it’s reasonable to allow ignoring user misconfigurations while providing warning-level log messages. I think your concern can be addressed through another feature "action classification". Some actions, such as "enqueue or allocate" are essential and require action validation, while others can be optionally ignored.

It's not a good way to classify different actions, all actions are important.

@Monokaix
Copy link
Member

Monokaix commented Feb 7, 2025

Currently there is a log showing the message.

There is too much messages when vc-scheduler just start... Maybe we need keep use klog.Errorf to make it highly visible?

Since actions are configurable, it’s reasonable to allow ignoring user misconfigurations while providing warning-level log messages. I think your concern can be addressed through another feature "action classification". Some actions, such as "enqueue or allocate" are essential and require action validation, while others can be optionally ignored.

There is no benefit to keep a wrong action name in configuration file, and if users discovered a mistake like this, they will fix it very easily. just my personal opinion

How about change Warning to Error?

@bogo-y
Copy link
Contributor

bogo-y commented Feb 7, 2025

How about change Warning to Error?

I think it's better than just use warning.

FYI: https://github.com/kubernetes/kubernetes/blob/release-1.32/pkg/scheduler/framework/runtime/framework.go#L658

@xieyanke
Copy link
Contributor Author

xieyanke commented Feb 7, 2025

How about change Warning to Error?

I think it's better than just use warning.

FYI: https://github.com/kubernetes/kubernetes/blob/release-1.32/pkg/scheduler/framework/runtime/framework.go#L658

Okey agreed, I'll make the changes.

@xieyanke xieyanke force-pushed the master branch 2 times, most recently from 241b791 to 9cd276a Compare February 7, 2025 04:39
Signed-off-by: xieyanke <xieyanke007@gmail.com>
@Monokaix
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2025
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

/approve
/ok-to-test

@volcano-sh-bot volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Feb 24, 2025
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hwdef

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2025
@volcano-sh-bot volcano-sh-bot merged commit c41fdb1 into volcano-sh:master Feb 24, 2025
17 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/scheduling kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants