-
Notifications
You must be signed in to change notification settings - Fork 959
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
Upgrade controller runtime to v0.17.5 and k8s api to v0.29.5 #4160
Conversation
@xliuqq Please fix the code conflict. Thanks. |
/test fluid-e2e |
|
/test fluid-e2e |
@cheyang the e2e-test was passed. |
dac7f8a
to
4935c99
Compare
/test fluid-e2e |
/test fluid-e2e |
c34002f
to
b99d1ef
Compare
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
/test fluid-e2e |
/retest |
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
cmd/juicefs/app/juicefs.go
Outdated
LeaderElection: enableLeaderElection, | ||
LeaderElectionNamespace: leaderElectionNamespace, | ||
LeaderElectionID: "juicefs.data.fluid.io", | ||
Port: 9443, | ||
NewCache: juicefsctl.NewCache(scheme), | ||
Cache: juicefsctl.NewCache(), |
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.
How about using juicefsctl.NewCacheOption()
?
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.
fixed
SelectorsByObject: selectors, | ||
}) | ||
return cache.Options{ | ||
ByObject: map[client.Object]cache.ByObject{ |
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 we need to add scheme?
return cache.Options{
Scheme: scheme,
ByObject: map[client.Object]cache.ByObject{
cronJobKey: {
Label: labels.SelectorFromSet(labels.Set{
common.JobPolicy: common.CronPolicy,
}),
},
},
}
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.
no need to, as the Scheme
field in ctrl.Options
will used to create the default Client and Cache.
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.
How about setting scheme in cache option to keep consistency with previous version? My thought is to make it easy for maintenance. WDYT?
cmd/juicefs/app/juicefs.go
Outdated
@@ -18,6 +18,7 @@ package app | |||
|
|||
import ( | |||
"os" | |||
metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" |
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.
Could you please move 'metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"' near 'sigs.k8s.io/controller-runtime/pkg/client'?
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.
fixed
@@ -36,7 +36,7 @@ func (b *BitMapAllocator) needResetReservedPorts() bool { | |||
} | |||
|
|||
func newBitMapAllocator(pr *net.PortRange, log logr.Logger) (BatchAllocatorInterface, error) { | |||
alloc, err := portallocator.New(*pr, func(max int, rangeSpec string) (allocator.Interface, error) { | |||
alloc, err := portallocator.New(*pr, func(max int, rangeSpec string, offset int) (allocator.Interface, error) { |
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 add TODO for passing offset
value. Thanks.
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.
fixed
Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
/lgtm |
TypeMeta: metav1.TypeMeta{ | ||
Kind: "StatefulSet", | ||
APIVersion: "apps/v1", | ||
}, |
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 is this necessary in new version of controller-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.
It seems related to kubernetes-sigs/controller-runtime#2633. If removed, test cases will fail.
/lgtm |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang 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 |
@xliuqq Thanks for your great contribution! |
Ⅰ. Describe what this PR does
Upgrade controller runtime to v0.17.5 and k8s api to v0.29.5.
Ⅱ. Does this pull request fix one issue?
fixes #XXXX
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews