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

update controller runtime #1800

Closed
wants to merge 3 commits into from

Conversation

kannon92
Copy link
Contributor

@kannon92 kannon92 commented Mar 4, 2024

What type of PR is this?

/kind dependency

What this PR does / why we need it:

Update controller runtime.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This is failing due to

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot
Copy link
Contributor

@kannon92: The label(s) kind/dependency cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this?

/kind dependency

What this PR does / why we need it:

Update controller runtime.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This is failing due to

Does this PR introduce a user-facing change?


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Mar 4, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kannon92
Once this PR has been reviewed and has the lgtm label, please assign alculquicondor for approval. For more information see the Kubernetes Code Review Process.

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 4, 2024
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 4, 2024
Copy link

netlify bot commented Mar 4, 2024

Deploy Preview for kubernetes-sigs-kueue canceled.

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 4, 2024
@kannon92
Copy link
Contributor Author

kannon92 commented Mar 4, 2024

Opened this up to demonstrate the failures.

Discussion here: kubernetes-sigs/controller-runtime#2681

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 6, 2024
@kannon92
Copy link
Contributor Author

kannon92 commented Mar 6, 2024

@alculquicondor @tenzen-y any ideas on this one?

Trying to add the patch interceptor but it is still saying patch is not allowed.

Do I need to use a different patch option?

@alculquicondor
Copy link
Contributor

Trying to add the patch interceptor but it is still saying patch is not allowed.

Where? Can you expand?

if patch.Type() != types.ApplyPatchType {
return clnt.Patch(ctx, obj, patch, opts...)
}
check, ok := obj.DeepCopyObject().(client.Object)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect you to override the type to types.StrategicMergePatchType.

Also, maybe we should have NewFakeClientWithWeakSSA, that we explicitly use when we are sure it's safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only issue is that we hit loads of these failures throughout Kueue.

Its whatever UpdateStatus is called so there are quite a few tests that fail with this.


kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1"
"sigs.k8s.io/kueue/pkg/controller/core/indexer"
)

func NewFakeClient(objs ...client.Object) client.Client {
return NewClientBuilder().WithObjects(objs...).WithStatusSubresource(objs...).Build()
return NewClientBuilder().WithObjects(objs...).WithStatusSubresource(objs...).
WithInterceptorFuncs(interceptor.Funcs{Patch: func(ctx context.Context, clnt client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kannon92 we are mostly using SSA to update the status try to do the same

	SubResourcePatch  func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error

also I don't think we can hit the object not found, so maybe try without that part

@kannon92
Copy link
Contributor Author

kannon92 commented Mar 6, 2024

Trying to add the patch interceptor but it is still saying patch is not allowed.

Where? Can you expand?

I just mean that I am still hitting the same failures that the unit tests are getting.

I can reproduce the failures with just the workload_test.go so I use that to do a quick test.

WithInterceptorFuncs(interceptor.Funcs{SubResourcePatch: func(ctx context.Context, clnt client.Client, SubResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
// Apply patches are supposed to upsert, but fake client fails if the object doesn't exist,
// if an apply patch occurs for an object that doesn't yet exist, create it.
if patch.Type() != types.ApplyPatchType {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a debugger, I can confirm that we are hitting this function now.

I don't really know what behavior we actually want in this function though. I took this code from an example about how to work around SSA patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

The existing logic is just trying to use a Create instead of a Patch when the object doesn't exist. That's one part of the problem, although we never use SSA for objects that don't exist in Kueue. The next one is the patch type. We need to make it a StrategicMergePatch.

@k8s-ci-robot
Copy link
Contributor

@kannon92: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kueue-test-unit-main c48c47d link true /test pull-kueue-test-unit-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.


kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1"
"sigs.k8s.io/kueue/pkg/controller/core/indexer"
)

func NewFakeClient(objs ...client.Object) client.Client {
return NewClientBuilder().WithObjects(objs...).WithStatusSubresource(objs...).Build()
return NewClientBuilder().WithObjects(objs...).WithStatusSubresource(objs...).
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, this function already calls NewclientBuilder, so no need to add the interceptor again.

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/client/interceptor"

kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1"
"sigs.k8s.io/kueue/pkg/controller/core/indexer"
)

Copy link
Contributor

@trasc trasc Mar 7, 2024

Choose a reason for hiding this comment

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

I wold just go with something like:

type smPatchWrapper struct {
	client.Patch
}

func (*smPatchWrapper) Type() types.PatchType {
	return types.StrategicMergePatchType
}

func wrapSSA(patch client.Patch) client.Patch {
	if patch.Type() == types.ApplyPatchType {
		return &smPatchWrapper{Patch: patch}
	}
	return patch
}

func SSAToSM(ctx context.Context, clnt client.Client, SubResourceName string, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
	return clnt.SubResource(SubResourceName).Patch(ctx, obj, wrapSSA(patch), opts...)
}

Then in the tests that require this , use this function as

manageBuilder = manageBuilder.WithInterceptorFuncs(interceptor.Funcs{SubResourcePatch: utiltesting.SSAToSM})

Note: It cannot be just added in 'NewClientBuilder' since:

  • We don't need it in all the tests.
  • WithInterceptorFuncs that might be needed after NewClientBuilder return will replace it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't declare manageBuilder anywhere other than in multikueue.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is just an usage example

@kannon92
Copy link
Contributor Author

@trasc @alculquicondor I'm not too familar on SSA so @alculquicondor said that they will take this over.

/close

@k8s-ci-robot
Copy link
Contributor

@kannon92: Closed this PR.

In response to this:

@trasc @alculquicondor I'm not too familar on SSA so @alculquicondor said that they will take this over.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants