-
Couldn't load subscription status.
- Fork 1.2k
Update volcano k8s dependencies to v1.34 #4687
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
base: master
Are you sure you want to change the base?
Update volcano k8s dependencies to v1.34 #4687
Conversation
|
Welcome @tunedev! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
|
@tunedev Also please check all of the failed CIs |
|
/gemini review |
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.
Code Review
This pull request upgrades Kubernetes dependencies to v1.34, which involves significant changes to adapt to the new scheduler framework interfaces. The changes are mostly mechanical and look correct. However, I've identified a few critical issues that would prevent the code from compiling or cause runtime errors. One is a recursive function call that would lead to a stack overflow, and the others are attempts to call methods on an interface type that doesn't have them. Please see the detailed comments for suggestions on how to fix these issues.
| // ignore this err since apiserver doesn't properly validate affinity terms | ||
| // and we can't fix the validation for backwards compatibility. | ||
| podInfo, _ := k8sframework.NewPodInfo(pod) | ||
| node.AddPodInfo(podInfo) |
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 node variable is of type fwk.NodeInfo, which is an interface. This interface does not have an AddPodInfo method. You need to perform a type assertion to the concrete type *k8sframework.NodeInfo before calling this method. This will cause a compilation error.
| node.AddPodInfo(podInfo) | |
| if n, ok := node.(*k8sframework.NodeInfo); ok { | |
| n.AddPodInfo(podInfo) | |
| } else { | |
| klog.Errorf("Failed to cast node to *k8sframework.NodeInfo for pod %s/%s", pod.Namespace, pod.Name) | |
| } |
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.
Looks like gemini have some hallucinations? Node has AddPodInfo method
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.
yeah it is
Signed-off-by: Babatunde Sanusi <swisskid95@gmail.com>
Signed-off-by: Babatunde Sanusi <swisskid95@gmail.com>
55065b4 to
4bd920b
Compare
Signed-off-by: Babatunde Sanusi <swisskid95@gmail.com>
Signed-off-by: Babatunde Sanusi <swisskid95@gmail.com>
|
|
||
| // Align default feature-gates with the connected cluster's version. | ||
| if err := setupComponentGlobals(config); err != nil { | ||
| klog.Errorf("failed to set component globals: %v", err) |
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.
Should return an error here when met it
cmd/scheduler/app/server.go
Outdated
| "k8s.io/klog/v2" | ||
|
|
||
| // Register gcp auth | ||
| utilruntime "k8s.io/apimachinery/pkg/util/runtime" |
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.
Should put before the comment Register gcp auth
cmd/scheduler/app/server.go
Outdated
|
|
||
| componentGlobalsRegistry := basecompatibility.NewComponentGlobalsRegistry() | ||
| if componentGlobalsRegistry.EffectiveVersionFor(basecompatibility.DefaultKubeComponent) == nil { | ||
| utilruntime.Must(componentGlobalsRegistry.Register( |
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.
We can refactor to
err = componentGlobalsRegistry.Register(
basecompatibility.DefaultKubeComponent,
kubeEffectiveVersion,
utilfeature.DefaultMutableFeatureGate,
)
if err != nil {
return fmt.Errorf("failed to register component globals: %w", err)
}We don't need to panic here
|
@tunedev
Therefore will cause an fatal error: So, we need to init a DynamicResourcesArgs, just like VolumeBindingArgs in this file: We can first set the default DynamicResourcesArgs in predicate plugin, and user also can override it, to set the FilterTimeout through |
|
@tunedev BTW, once you fix all the CIs and fix the review comments, please squash your commits into one |
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.
DRA v1.34 has lots of new feature gate, therefore we need to also add them here:
volcano/pkg/scheduler/plugins/predicates/predicates.go
Lines 281 to 290 in a116699
| features := feature.Features{ | |
| EnableStorageCapacityScoring: utilFeature.DefaultFeatureGate.Enabled(features.StorageCapacityScoring), | |
| EnableNodeInclusionPolicyInPodTopologySpread: utilFeature.DefaultFeatureGate.Enabled(features.NodeInclusionPolicyInPodTopologySpread), | |
| EnableMatchLabelKeysInPodTopologySpread: utilFeature.DefaultFeatureGate.Enabled(features.MatchLabelKeysInPodTopologySpread), | |
| EnableSidecarContainers: utilFeature.DefaultFeatureGate.Enabled(features.SidecarContainers), | |
| EnableDRAAdminAccess: utilFeature.DefaultFeatureGate.Enabled(features.DRAAdminAccess), | |
| EnableDynamicResourceAllocation: utilFeature.DefaultFeatureGate.Enabled(features.DynamicResourceAllocation), | |
| EnableVolumeAttributesClass: utilFeature.DefaultFeatureGate.Enabled(features.VolumeAttributesClass), | |
| EnableCSIMigrationPortworx: utilFeature.DefaultFeatureGate.Enabled(features.CSIMigrationPortworx), | |
| } |
For the new DRA feature gates needed to add: https://github.com/kubernetes/kubernetes/blob/03a5f06c2695805059278c9d6b47edc3bdcf51b1/pkg/scheduler/framework/plugins/feature/feature.go#L53-L60
Signed-off-by: Babatunde Sanusi <swisskid95@gmail.com>
402953c to
963c6b0
Compare
What type of PR is this?
feature
What this PR does / why we need it:
This PR upgrades Kubernetes to v1.34, replacing the reference to Struct with interfaces in the new version.
It includes necessary code, YAML, and Docker changes to support this transition.
Which issue(s) this PR fixes:
fixes #4671
Special notes for your reviewer:
Does this PR introduce a user-facing change?