Skip to content
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

k8s 1.29 bump #1633

Merged
merged 5 commits into from Jan 31, 2024
Merged

k8s 1.29 bump #1633

merged 5 commits into from Jan 31, 2024

Conversation

kannon92
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Update kubernetes to 0.29 api changes.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

I see two groups of failures:

field.NotSupported(field.NewPath("spec", "tolerations").Index(2).Child("effect"), corev1.TaintEffect("not-valid"), nil),

Error is "cannot infer t" and these errors are in pkg/webhooks/resourceflavor_webhook_test.go and pkg/webhooks/workload_webhooks_test.go.

And the pkg/controller/job/pod has the following error while running make test.

--- FAIL: TestReconciler_ErrorFinalizingPod (0.00s)
    /home/kehannon/Work/kueue/pkg/controller/jobs/pod/reconciler.go:233: "level"=2 "msg"="Reconciling Job" "job"="ns/pod" "gvk"="/v1, Kind=Pod"
    /home/kehannon/Work/kueue/pkg/controller/jobs/pod/pod_controller_test.go:2213: Expected reconcile error (-want,+got):
          any(
        - 	e"connection refused: connection refused",
          )
    /home/kehannon/Work/kueue/pkg/controller/jobs/pod/reconciler.go:233: "level"=2 "msg"="Reconciling Job" "job"="ns/pod" "gvk"="/v1, Kind=Pod"

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 22, 2024
Copy link

netlify bot commented Jan 22, 2024

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit ea8ef47
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65b96943dd89420008276053

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 22, 2024
go.mod Outdated Show resolved Hide resolved
@tenzen-y
Copy link
Member

@kannon92 Hi! Are you still here?

I think we should include this in the next new minor version.
Since we will release the new minor version in the first week of February, could you complete this by its day?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2024
@kannon92
Copy link
Contributor Author

I updated the PR but I need someone with Kueue experience to look into these errors.

I see two groups of failures:

field.NotSupported(field.NewPath("spec", "tolerations").Index(2).Child("effect"), corev1.TaintEffect("not-valid"), nil),
Error is "cannot infer t" and these errors are in pkg/webhooks/resourceflavor_webhook_test.go and pkg/webhooks/workload_webhooks_test.go.

And the pkg/controller/job/pod has the following error while running make test.

--- FAIL: TestReconciler_ErrorFinalizingPod (0.00s)
    /home/kehannon/Work/kueue/pkg/controller/jobs/pod/reconciler.go:233: "level"=2 "msg"="Reconciling Job" "job"="ns/pod" "gvk"="/v1, Kind=Pod"
    /home/kehannon/Work/kueue/pkg/controller/jobs/pod/pod_controller_test.go:2213: Expected reconcile error (-want,+got):
          any(
        - 	e"connection refused: connection refused",
          )
    /home/kehannon/Work/kueue/pkg/controller/jobs/pod/reconciler.go:233: "level"=2 "msg"="Reconciling Job" "job"="ns/pod" "gvk"="/v1, Kind=Pod"

Happy to resolve them if someone else can suggest a fix. Otherwise, I'm not sure how much progress I can make as I haven't done a lot of Kueue development lately.

@kannon92
Copy link
Contributor Author

Running make test:

=== Failed
=== FAIL: pkg/controller/jobs/pod TestReconciler_ErrorFinalizingPod (0.02s)
    reconciler.go:263: "level"=2 "msg"="Reconciling Job" "job"="ns/pod" "gvk"="/v1, Kind=Pod"
    pod_controller_test.go:2526: Expected reconcile error (-want,+got):
          any(
        -       e"connection refused: connection refused",
          )
    reconciler.go:263: "level"=2 "msg"="Reconciling Job" "job"="ns/pod" "gvk"="/v1, Kind=Pod"

=== Errors
pkg/webhooks/resourceflavor_webhook_test.go:112:23: cannot infer T (/home/kehannon/go/pkg/mod/k8s.io/apimachinery@v0.29.1/pkg/util/validation/field/errors.go:203:19)
pkg/webhooks/workload_webhook_test.go:263:23: cannot infer T (/home/kehannon/go/pkg/mod/k8s.io/apimachinery@v0.29.1/pkg/util/validation/field/errors.go:203:19)
pkg/webhooks/workload_webhook_test.go:349:23: cannot infer T (/home/kehannon/go/pkg/mod/k8s.io/apimachinery@v0.29.1/pkg/util/validation/field/errors.go:203:19)

@trasc At a quick glance you added

				field.NotSupported(field.NewPath("spec", "tolerations").Index(2).Child("effect"), corev1.TaintEffect("not-valid"), nil),

We are getting cannot infer T from usage of this in kube 1.29.

@kannon92
Copy link
Contributor Author

/cc @trasc

@kannon92
Copy link
Contributor Author

@kannon92 Hi! Are you still here?

I think we should include this in the next new minor version. Since we will release the new minor version in the first week of February, could you complete this by its day?

I don't think I'll be able to hit that february deadline as I'll be traveling.

@mimowo
Copy link
Contributor

mimowo commented Jan 29, 2024

the first issue with cannot infer can probably be easily fixed by wrapping the nil with type []string(nil)?

I'm not sure about the missing reconcile error. Probably can take a look tomorrow.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024
@kannon92
Copy link
Contributor Author

the first issue with cannot infer can probably be easily fixed by wrapping the nil with type []string(nil)?

I'm not sure about the missing reconcile error. Probably can take a look tomorrow.

Thank you! That fix worked like a charm. Still got the reconcile error and it looks to also impact 1.29 e2e test

@@ -109,7 +109,7 @@ func TestValidateResourceFlavor(t *testing.T) {
Value: "v",
Effect: corev1.TaintEffectNoSchedule,
}, ""),
field.NotSupported(field.NewPath("spec", "tolerations").Index(2).Child("effect"), corev1.TaintEffect("not-valid"), nil),
field.NotSupported(field.NewPath("spec", "tolerations").Index(2).Child("effect"), corev1.TaintEffect("not-valid"), []string{}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
field.NotSupported(field.NewPath("spec", "tolerations").Index(2).Child("effect"), corev1.TaintEffect("not-valid"), []string{}),
field.NotSupported(field.NewPath("spec", "tolerations").Index(2).Child("effect"), corev1.TaintEffect("not-valid"), []corev1.TaintEffect{}),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

@trasc
Copy link
Contributor

trasc commented Jan 30, 2024

For the flaky e2e multikueue test we have #1659 under review.

For

    pod_controller_test.go:2526: Expected reconcile error (-want,+got):
          any(
        -       e"connection refused: connection refused",
          )
    reconciler.go:263: "level"=2 "msg"="Reconciling Job" "job"="ns/pod" "gvk"="/v1, Kind=Pod"

It looks like the TypeMeta of the object in

Update: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
gvk := obj.GetObjectKind().GroupVersionKind()
if gvk.GroupVersion().String() == "v1" && gvk.Kind == "Pod" {
defer func() { reqcount++ }()
if reqcount == 0 {
// return a connection refused error for the first update request.
return errMock
}
if reqcount == 1 {
// Exec a regular update operation for the second request
return client.Update(ctx, obj, opts...)
}
}
return client.Update(ctx, obj, opts...)
},
})

is not populated, you could :

a. get the GVK from the clients schema

                               kinds, _, err := client.Scheme().ObjectKinds(obj)
                               if err == nil {
                                       gvk = kinds[0]
                               }

b. do a type assertion instead _, isPod := obj.(*corev1.Pod)

@mimowo
Copy link
Contributor

mimowo commented Jan 30, 2024

b. do a type assertion instead _, isPod := obj.(*corev1.Pod)

If this is just about unit tests, it is probably ok to go with b as simpler.

@tenzen-y
Copy link
Member

For the flaky e2e multikueue test we have #1659 under review.

For

    pod_controller_test.go:2526: Expected reconcile error (-want,+got):
          any(
        -       e"connection refused: connection refused",
          )
    reconciler.go:263: "level"=2 "msg"="Reconciling Job" "job"="ns/pod" "gvk"="/v1, Kind=Pod"

It looks like the TypeMeta of the object in

Update: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
gvk := obj.GetObjectKind().GroupVersionKind()
if gvk.GroupVersion().String() == "v1" && gvk.Kind == "Pod" {
defer func() { reqcount++ }()
if reqcount == 0 {
// return a connection refused error for the first update request.
return errMock
}
if reqcount == 1 {
// Exec a regular update operation for the second request
return client.Update(ctx, obj, opts...)
}
}
return client.Update(ctx, obj, opts...)
},
})

is not populated, you could :

a. get the GVK from the clients schema

                               kinds, _, err := client.Scheme().ObjectKinds(obj)
                               if err == nil {
                                       gvk = kinds[0]
                               }

b. do a type assertion instead _, isPod := obj.(*corev1.Pod)

@trasc I guess that the root cause of the TypeMeta not being populated is kubernetes-sigs/controller-runtime#2633.

@kannon92
Copy link
Contributor Author

Pushed a fix for each. Thank you for looking into that.

@kannon92
Copy link
Contributor Author

/assign @trasc @tenzen-y
This is good to go now.

/retest

Copy link
Contributor

@trasc trasc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ffea7a787d1f05b97f48c6426da47254625ea516

Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kannon92, tenzen-y

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2024
@k8s-ci-robot k8s-ci-robot merged commit 0b0961d into kubernetes-sigs:main Jan 31, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.6 milestone Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants