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

Volcano adapts to the k8s v1.29 #3295

Merged
merged 7 commits into from
Feb 1, 2024

Conversation

guoqinwill
Copy link
Contributor

@guoqinwill guoqinwill commented Jan 9, 2024

1、Volcano adapts to the k8s v1.29
2、In k8s v1.29, some apis have changed. For example, cpuset from "k8s.io/kubernetes/pkg/kubelet/cm/cpuset/cpuset.go" export to "k8s.io/utils/cpuset". Remove 'selectorspread' plugin and use 'podTopologySpread' plugin instead .
3、In k8s v1.29.Migrated the volumebinding scheduler plugins to use contextual logging.Graduated the ReadWriteOncePod and PodSecurity feature gate to GA, and PodSecurity have been removed .Added 'return Skip in PreFilter' when calculates the NodePorts and PodTopologySpread for each existing pod on each node. This update adapts to this change.

@volcano-sh-bot volcano-sh-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2024
@volcano-sh-bot volcano-sh-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 9, 2024
@guoqinwill guoqinwill force-pushed the us-adapt-1.29 branch 4 times, most recently from a410b6d to a68909d Compare January 12, 2024 09:50
@guoqinwill guoqinwill force-pushed the us-adapt-1.29 branch 3 times, most recently from a4580f9 to e50065b Compare January 16, 2024 12:16
@guoqinwill guoqinwill changed the title [WIP]Volcano adapts to the k8s v1.29 Volcano adapts to the k8s v1.29 Jan 16, 2024
@volcano-sh-bot volcano-sh-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 16, 2024
@guoqinwill guoqinwill force-pushed the us-adapt-1.29 branch 3 times, most recently from c616764 to 473a7ef Compare January 16, 2024 14:15
skipPlugins[taskKey] = plugins
}
skipPlugins[taskKey].Insert(podTopologySpreadFilter.Name())
klog.V(3).Infof("pod(%s/%s) affinity require information is nil, plugin podTopologySpreadFilter is skipped",
Copy link
Member

Choose a reason for hiding this comment

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

Change log level to 5 to avoid rredundant logs, and so does other plugin's skip check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -401,7 +409,16 @@ func (pp *predicatesPlugin) OnSessionOpen(ssn *framework.Session) {
// the processing logic needs to be added to the return value result.
if predicate.podTopologySpreadEnable {
_, status := podTopologySpreadFilter.PreFilter(context.TODO(), state, task.Pod)
if !status.IsSuccess() {
if status.IsSkip() {
Copy link
Member

Choose a reason for hiding this comment

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

Abstract this logic to a function because many plugins do the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

for _, node := range nodes {
nodeScores[node.Name] = podAffinityScores[node.Name] + nodeTolerationScores[node.Name] + podTopologySpreadScores[node.Name] + selectorSpreadScores[node.Name]
Copy link
Member

Choose a reason for hiding this comment

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

What's the compatibility policy of volcano with kubernets version support? User with lower kubernetes version than 1.29 will miss the plugin.

Copy link
Contributor Author

@guoqinwill guoqinwill Jan 25, 2024

Choose a reason for hiding this comment

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

After review, the plugin of SelectorSpread can be deleted for the following reasons: First, k8s provides the PodTopologySpread plugin to replace the SelectorSpread plugin since version 1.19; secondly, volcano introduced both plugins at the same time, and the selector plugin is not used by default, but the podtopology plugin is used,users can use PodTopologySpread instead of SelectorSpread.

@Monokaix
Copy link
Member

Please also update Kubernetes compatibility matrix in README.

@Monokaix
Copy link
Member

The commit "fix some ci errors" should only contains fossa ci fix, helm yaml fix is a seperate part.

@guoqinwill guoqinwill force-pushed the us-adapt-1.29 branch 3 times, most recently from 8fa5c88 to a3492c3 Compare January 17, 2024 11:30
@guoqinwill guoqinwill closed this Jan 17, 2024
@guoqinwill guoqinwill reopened this Jan 17, 2024
@@ -8812,7 +8812,7 @@
import (
v1 "k8s.io/api/core/v1"
"k8s.io/kubernetes/pkg/kubelet/cm/cpumanager/topology"
"k8s.io/kubernetes/pkg/kubelet/cm/cpuset"
"k8s.io/utils/cpuset"
Copy link
Member

Choose a reason for hiding this comment

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

This is the UT coverage report of the previous version.
Why do we need to change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@wangyang0616
Copy link
Member

Please make CI happy. @guoqinwill

@guoqinwill guoqinwill force-pushed the us-adapt-1.29 branch 2 times, most recently from cd532d0 to 43a9684 Compare January 25, 2024 08:47
@wangyang0616
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2024
@Monokaix
Copy link
Member

Please modify file installer/dockerfile/webhook-manager/Dockerfile, change KUBE_VERISON to 1.29

…and modify some klogs formats

Signed-off-by: guoqin <[email protected]>
Signed-off-by: wangyang <[email protected]>
@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2024
@Monokaix
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2024
Copy link
Contributor

@Thor-wl Thor-wl left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Member

@william-wang william-wang left a comment

Choose a reason for hiding this comment

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

/lgtm

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: william-wang

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 1, 2024
@volcano-sh-bot volcano-sh-bot merged commit a0742c8 into volcano-sh:master Feb 1, 2024
14 checks passed
@william-wang william-wang added this to the v1.9 milestone Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants