-
Notifications
You must be signed in to change notification settings - Fork 479
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
Changes required make a build after update of component-base #3004
base: master
Are you sure you want to change the base?
Changes required make a build after update of component-base #3004
Conversation
dbff897
to
a43e436
Compare
a10c58c
to
70b35d6
Compare
LGTM @andrewsykim @kevin85421 PTAL, this is currently blocking upgrade of KubeRay to 1.3 in Kueue, which is required for the MultiKueue integration (requiring the managedBy field). I know this is a massive amount of changes, but all of them are due to the new code generation script, which actually simplifies the code generation: https://github.com/ray-project/kuberay/pull/3004/files#diff-791462c57818fbde5da46c5925c2b709d459724d4824513356aea9816167e893R26-R41. |
@mszadkow is there a way to build Kueue on CI using this commit to KubeRay to double check there are no other blockers to bump KubeRay in Kueue? |
In terms of compatibility now, I have been able to build Kueue with this feature branch version of Kuberay. |
This LGTM, but given it bumps Go version and other deps, it would be great to get another set of eyes on this. @MortalHappiness @kevin85421 can you take a look as well pleas? |
Just realized that KubeRay v1.3 is not compatible with Kueue until this is merged. We're just about to cut v1.3 and it seems risky to include all the changes here, specifically the controller-gen bump. @mszadkow are all the changes in this PR required or is there a more minimal version of this that can work? Specifically worried about the bump to controller-gen that is changing the generated CRD yaml |
func SetFeatureGateDuringTest(tb testing.TB, f featuregate.Feature, value bool) func() { | ||
return featuregatetesting.SetFeatureGateDuringTest(tb, utilfeature.DefaultFeatureGate, f, value) | ||
func SetFeatureGateDuringTest(tb testing.TB, f featuregate.Feature, value bool) { | ||
featuregatetesting.SetFeatureGateDuringTest(tb, utilfeature.DefaultFeatureGate, f, value) |
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 think I'm missing something here. I was expecting the function signature of featuregatetesting.SetFeatureGateDuringTest(tb, utilfeature.DefaultFeatureGate, f, value)
to change here. Why do we need to bump dependencies if this doesn't change?
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.
It has changed so we also adapt this wrapped to it.
In 0.29 this give you a cleanup function that you should call with defer, but since 0.31 it sets the cleanup function for you in ginkgo.
The underlying change is also that it detects parallel set to feature flags so this required change in tests
@mszadkow may know more details but IIRC controller-gen is a transitive dependency of the core k8s, and it was refactored in 1.31 k8s. OTOH we need to align the core k8s version with Kueue to be able to compile. |
I think this would be too risky to add into v1.3 at this point. To resolve kueue compilation issues, we can consider backport to v1.3.1 or wait til v1.4 as a last resort |
8ed48e1
to
71d36bf
Compare
71d36bf
to
3211bb8
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.
Hi @mszadkow, @owenowenisme noticed that it seems this PR didn't run make generate
.
I cloned your fork and ran make generate with a small fix in update-codegen.sh
, and the changed files are as follows:

@@ -557,14 +557,11 @@ var _ = Context("Inside the default namespace", func() { | |||
headFilters := common.RayClusterHeadPodsAssociationOptions(rayCluster).ToListOptions() | |||
allFilters := common.RayClusterAllPodsAssociationOptions(rayCluster).ToListOptions() | |||
|
|||
BeforeAll(func() { |
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.
Are the changes in this file manual or automatic?
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.
Those are manual.
Some tests required a refactoring, e.g. this change was required because it's no longer run in Describe
but inside subject node It()
3211bb8
to
994555a
Compare
/test buildkite/ray-ecosystem-ci-kuberay-ci/test-autoscaler-e2e-nightly-operator |
I will attempt to reproduce the issue of autoscaler e2e tests locally |
994555a
to
398d467
Compare
#3100 has already been merged. |
398d467
to
b9c7073
Compare
@kevin85421 sure, rebased |
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 after the recent changes and rebase.
What we could consider to make the diff smaller is:
- make a small preapratory PR for ray-operator/controllers/ray/rayjob_controller_test.go
- make a small preparatory PR for bumping golang to 1.23
However, this would mean more PRs to cherry-pick for 1.3.1, and it would not reduce the volume of the PR but much %, but raising this for consideration.
@andrewsykim @kevin85421 PTAL.
![]() @mszadkow would you mind rebasing with the master branch? |
b9c7073
to
b07fb54
Compare
thanks for approval @kevin85421, it's rebased now |
Migrate to kube_codegen.sh
A in SetFeatureGateDuringTest, that stopped returning a cleanup func - kubernetes/kubernetes#123677 Required tests update to not use `It()` while it could be `By()` as using `It()` resulted in parallel access to the feature gates.
b07fb54
to
dbb42bf
Compare
Why are these changes needed?
Kuberay in v1.3.0 is incompatible with Kueue v.0.10.1 and above, due to:
The change comes from the component-base version difference:
Kuberay has k8s.io/component-base v0.29.6
Kueue has k8s.io/component-base v0.32.1
a small but significant change in https://github.com/kubernetes/component-base/blob/264c1fd30132a3b36b7588e50ac54eb0ff75f26a/featuregate/testing/feature_gate.go#L47
Because of this we had to update component-base on Kuberay.
In result:
Related issue number
Relates to: kubernetes-sigs/kueue#3822
Checks