-
Notifications
You must be signed in to change notification settings - Fork 962
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
Add featuregates for volcano capabilities #2989
Conversation
289c921
to
eb2c40f
Compare
@Monokaix Thanks for your contribution. I think I've been aware of your requirements and ideas in some degree. Can you tell more about the meaning of this fearure? In another word, what will happen without the PR? Unacceptable memory usage or poor performance due to unusable cache? |
/hold |
We hope that Volcano provides a basic framework, which only includes some core resources of k8s. Without this pr, Volcano will not start successfully if apiserver has no advanced capabilities API registered, which are showed in this pr, because it has to wait for the cache synchronization of informer resources. |
Some user builds serverless container platform based on Volcano. The infra platform is a simplied kubernetes that have no object like CSI, priority class and so on. The current Volcano does not play with it. From my side, Volcano can be enhanced to @Monokaix suggest to file a enhancement issue and clarify the scenario and associate it with this pr. |
1695d41
to
a36dc1b
Compare
+1 |
05ee33a
to
9d4bdd3
Compare
Thanks, enhancement is added. |
400a886
to
6150894
Compare
@Thor-wl Enhancement has been posted, Could you take a look once more? |
OK. Since this PR is a little big large, it may cost some time to reivew. |
cmd/scheduler/app/options/options.go
Outdated
@@ -60,8 +60,6 @@ type ServerOption struct { | |||
PrintVersion bool | |||
EnableMetrics bool | |||
ListenAddress string | |||
EnablePriorityClass bool | |||
EnableCSIStorage bool |
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.
Let's take code compatibility into consideration.
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.
Thanks, the params are kept.
cmd/scheduler/app/options/options.go
Outdated
@@ -107,8 +105,6 @@ func (s *ServerOption) AddFlags(fs *pflag.FlagSet) { | |||
fs.StringVar(&s.LockObjectNamespace, "lock-object-namespace", defaultLockObjectNamespace, "Define the namespace of the lock object that is used for leader election; it is volcano-system by default") | |||
fs.StringVar(&s.ListenAddress, "listen-address", defaultListenAddress, "The address to listen on for HTTP requests.") | |||
fs.StringVar(&s.HealthzBindAddress, "healthz-address", defaultHealthzAddress, "The address to listen on for the health check server.") | |||
fs.BoolVar(&s.EnablePriorityClass, "priority-class", true, |
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.
Same here
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.
Thanks, the params are kept.
Please provide some important test result snapshot about this feature. Thanks. |
/hold cancel |
FilterFunc: func(obj interface{}) bool { | ||
switch v := obj.(type) { | ||
case *busv1alpha1.Command: | ||
if v.TargetObject != nil && |
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 be simpled as return v.TargetObject != nil && v.TargetObject.APIVersion == batchv1alpha1.SchemeGroupVersion.String() && v.TargetObject.Kind == "Job"
pkg/features/volcano_features.go
Outdated
) | ||
|
||
const ( | ||
// WorkLoadSupport can cache and operate workload resource, Deployment/Replicas/ReplicationController/StatefulSet resources currently. |
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 update the comment to be WorkLoadSupport can cache and operate **K8s native resource**, Deployment/Replicas/ReplicationController/StatefulSet resources currently.
. That may be easier to understand.
Signed-off-by: Xuzheng Chang <changxuzheng@huawei.com>
/lgtm |
@@ -33,6 +33,7 @@ import ( | |||
utilruntime "k8s.io/apimachinery/pkg/util/runtime" | |||
"k8s.io/apimachinery/pkg/util/sets" | |||
"k8s.io/apimachinery/pkg/util/wait" | |||
utilfeature "k8s.io/apiserver/pkg/util/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.
please change utilfeature
to be a more reasonable name
runtime.Must(utilfeature.DefaultMutableFeatureGate.Add(defaultVolcanoFeatureGates)) | ||
} | ||
|
||
var defaultVolcanoFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ |
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.
change defaultVolcanoFeatureGates
to defaultFeatureGates
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.
/approve
[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 |
Releated issue: #3012
Volcano supports many kubernetes native resources and uses these resources to serve the scheduling algorithm, it also provides high-level capabilities such as queues and queue commands. In serverless scenarios, we only focus on core resources such as pods/podgroup, we should disable some resources and capabilitis by feature gate.
So this pr: