-
Notifications
You must be signed in to change notification settings - Fork 953
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.25 #2533
Volcano adapts to the k8s v1.25 #2533
Conversation
@wangyang0616: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
047a109
to
ed07e12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it nessary to update the volcano.sh/apis with k8s v1.25 first?
42d931b
to
cf1dd30
Compare
It is true that API changes should be incorporated first. volcano.sh/apis adapts to K8S V1.25 volcano-sh/apis#93 |
54e727b
to
d390372
Compare
@wangyang0616: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
2ed5181
to
e00f74a
Compare
e130f7a
to
981c423
Compare
@@ -22,6 +22,7 @@ import ( | |||
"fmt" | |||
"sync" | |||
"time" | |||
"volcano.sh/volcano/cmd/scheduler/app/options" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest to move package volcano.sh/volcano/cmd/scheduler/app/options
behind k8s.io
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In K8S V1.25, the EnableCSIStorageCapacity
feature in the k8s.io/kubernetes/pkg/scheduler/framework/plugins/feature
package is removed and cannot be used.
After the Volcano adapts to K8S V1.25, to ensure compatibility of earlier versions (K8S V1.21 and V1.23), the Volcano internal feature switch options.ServerOpts.EnableCSIStorage
is still used, that is, volcano.sh/volcano/cmd/scheduler/app/options
is introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess its meaning go fmt
is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess its meaning
go fmt
is needed.
Thank you. I misunderstood.
It's been revised.
@@ -25,6 +25,7 @@ import ( | |||
|
|||
v1 "k8s.io/api/core/v1" | |||
storagev1 "k8s.io/api/storage/v1" | |||
storagev1beta1 "k8s.io/api/storage/v1beta1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both v1
and v1beta1
is reserved in references, would they disturbe each other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage of k8s v1.23 is still used, and no conflict occurs. CSIStorageCapacity uses v1beta1 and other storage interfaces use v1. Ensure the forward compatibility after the Volcano is upgraded to K8S V1.25 (for example, K8S V1.23 and V1.21).
@@ -227,7 +231,7 @@ func NewVolumeBinder( | |||
pvcInformer coreinformers.PersistentVolumeClaimInformer, | |||
pvInformer coreinformers.PersistentVolumeInformer, | |||
storageClassInformer storageinformers.StorageClassInformer, | |||
capacityCheck CapacityCheck, | |||
capacityCheck *CapacityCheck, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does capacityCheck
is modified to pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does
capacityCheck
is modified to pointer?
The Volcano uses the feature switch (options.ServerOpts.EnableCSIStorage) to control the use of the CSIStorageCapacity function. For details, see the implementation of K8S V1.23. The value of CapacityCheck is changed back to the pointer type.
If the CSIStorageCapacity function is not enabled, the pointer is null. The informer is not initialized when the CSIStorageCapacity function is enabled. After the CSIStorageCapacity function is enabled, the external invoker can assign a value to the CapacityCheck pointer. The informer is initialized when the CSIStorageCapacity function is enabled.
@@ -87,7 +87,7 @@ func Run(opt *options.ServerOption) error { | |||
// add a uniquifier so that two processes on the same host don't accidentally both become active | |||
id := hostname + "_" + string(uuid.NewUUID()) | |||
|
|||
rl, err := resourcelock.New(resourcelock.ConfigMapsResourceLock, | |||
rl, err := resourcelock.New(resourcelock.ConfigMapsLeasesResourceLock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why locktype is changed from configmaps
to configmapsleases
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In K8S V1.25, configMapsResourceLock is removed and replaced by ConfigMapsLeasesResourceLock.
It is true that API changes should be incorporated first. |
/lgtm |
@jiangkaihua: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: wangyang <wangyang289@huawei.com>
…rsions. Signed-off-by: wangyang <wangyang289@huawei.com>
cae2f30
to
c414ca5
Compare
podAffinityFilter := plugin.(*interpodaffinity.InterPodAffinity) | ||
// 6. NodeVolumeLimits | ||
features := feature.Features{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does feature struct contain a lot of sub-feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy that.
Feature.Features{} needs to be initialized and assigned with a value.
Modified.
@@ -398,7 +398,8 @@ func (pp *predicatesPlugin) OnSessionOpen(ssn *framework.Session) { | |||
} | |||
|
|||
// InterPodAffinity Predicate | |||
status = podAffinityFilter.PreFilter(context.TODO(), state, task.Pod) | |||
// TODO use framework.PreFilterResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to add some thing more here? if not, please delete useless codes or comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy that. Modified.
@@ -415,7 +416,8 @@ func (pp *predicatesPlugin) OnSessionOpen(ssn *framework.Session) { | |||
} | |||
|
|||
// Check VolumeBinding: handle immediate claims unbounded case | |||
status = volumebindingFilter.PreFilter(context.TODO(), state, task.Pod) | |||
// TODO use framework.PreFilterResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same with above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy that. Modified.
@@ -433,7 +435,8 @@ func (pp *predicatesPlugin) OnSessionOpen(ssn *framework.Session) { | |||
} | |||
|
|||
// Check PodTopologySpread | |||
status = podTopologySpreadFilter.PreFilter(context.TODO(), state, task.Pod) | |||
// TODO use framework.PreFilterResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The save as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy that. Modified.
pkg/scheduler/cache/cache.go
Outdated
@@ -50,7 +50,7 @@ import ( | |||
"k8s.io/client-go/util/workqueue" | |||
"k8s.io/klog" | |||
podutil "k8s.io/kubernetes/pkg/api/v1/pod" | |||
volumescheduling "k8s.io/kubernetes/pkg/scheduler/framework/plugins/volumebinding" | |||
volumescheduling "volcano.sh/volcano/pkg/scheduler/plugins/volumebinding" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cache should not depend plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest add capabilities directory like pkg/scheduler/capabilities/volumebinding and put our volumebinding there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy that. Modified.
EnableReadWriteOncePod: utilFeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod), | ||
EnableVolumeCapacityPriority: utilFeature.DefaultFeatureGate.Enabled(features.VolumeCapacityPriority), | ||
EnableCSIStorageCapacity: utilFeature.DefaultFeatureGate.Enabled(features.CSIStorageCapacity), | ||
EnableReadWriteOncePod: utilFeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnablePodDisruptionBudget is removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PodDisruptionBudget
feature has passed GA. The EnablePodDisruptionBudget
feature is removed in 1.25.
@@ -362,7 +361,7 @@ func (pp *nodeOrderPlugin) OnSessionOpen(ssn *framework.Session) { | |||
ssn.AddNodeOrderFn(pp.Name(), nodeOrderFn) | |||
|
|||
plArgs := &config.InterPodAffinityArgs{} | |||
p, _ = interpodaffinity.New(plArgs, handle, fts) | |||
p, _ = interpodaffinity.New(plArgs, handle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason of deleting fts for interpodaffinity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PodAffinityNamespaceSelector is GA in V1.24. The switch for this feature is deleted.
In this case, the fst parameter is not transferred when the PodAffinityNamespaceSelector is new.
@@ -371,7 +370,7 @@ func (pp *nodeOrderPlugin) OnSessionOpen(ssn *framework.Session) { | |||
ptsArgs := &config.PodTopologySpreadArgs{ | |||
DefaultingType: config.SystemDefaulting, | |||
} | |||
p, _ = podtopologyspread.New(ptsArgs, handle) | |||
p, _ = podtopologyspread.New(ptsArgs, handle, fts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason of adding fts to podtopologyspread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In K8S V1.25, the alpha and beta feature switches related to TopologySpread are added, for example, EnableMinDomainsInPodTopologySpread
, EnableNodeInclusionPolicyInPodTopologySpread
, EnableMatchLabelKeysInPodTopologySpread
.
4ef6e8d
to
b8dbeb1
Compare
…che on the plugin. Signed-off-by: wangyang <wangyang289@huawei.com>
b8dbeb1
to
cd7a58f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[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 |
Signed-off-by: wangyang wangyang289@huawei.com
/kind feature #2510