update controller runtime

[kannon92]

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]

@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]

[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:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[netlify]

✅ 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

[kannon92]

Opened this up to demonstrate the failures.

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

[kannon92]

@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]

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

Where? Can you expand?

[alculquicondor]

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.

[kannon92]

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.

[trasc]

@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]

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.

[kannon92]

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.

[alculquicondor]

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]

@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.

[alculquicondor]

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

[trasc]

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.

[kannon92]

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

[trasc]

That is just an usage example

[kannon92]

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

/close

[k8s-ci-robot]

@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.