🌱 refactor SortForCreate to use sort.Slice #9251
Conversation
|
|
|
Hi @kokes. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Welcome @kokes! |
|
I redid it a bit - I decided not to deprecate We could also:
Neither is a priority tho. |
|
/ok-to-test |
There was a problem hiding this comment.
The behaviour of the SortForCreate stays the same. Interestingly, the original implementation doesn't match the comments and the tests don't cover the scenarios.
For example, the comments say that Pods go before Replicaset but if you extend the test to include pods, deployments, replicasets like this:
cm := unstructured.Unstructured{}
cm.SetKind("ConfigMap")
ns := unstructured.Unstructured{}
ns.SetKind("Namespace")
ep := unstructured.Unstructured{}
ep.SetKind("Endpoint")
dp := unstructured.Unstructured{}
dp.SetKind("Deployment")
rs := unstructured.Unstructured{}
rs.SetKind("ReplicaSet")
pd := unstructured.Unstructured{}
pd.SetKind("Pod")
resources := []unstructured.Unstructured{ep, cm, ns, dp, rs, pd}The replicaset is sorted before the pod. The issue is mainly that in the original implementation the slice should have contained Pod & Endpoint instead of Pods & Endpoints.
|
@killianmuldoon @enxebre - do we care that the original behaviour doesn't match the comments in the code? I see we use it in the CRS and clusterctl (for cert-manager) code but i don't have any context on its original inclusion. (If we do care i'd say its out of scope for this PR though) |
IMO it's clear the current behaviour is a bug which could actually cause issues in e.g. a ClusterResourceSet. Fine with properly fixing that here so long as we update the tests to cover the full behavior of the sort. |
There was a problem hiding this comment.
Looks good - just a couple of asks for the tests.
Could you update the test to add all of the types listed in resources, and a couple that aren't e.g. `Deployment'. It would also be good to define them in a map and convert that to an input sice to ensure we get multiple permutations in the test.
|
I've pushed two commits, where I:
|
|
Done |
|
LGTM label has been added. Git tree hash: ad8b2f469df1126c134f06745627bd4753e561c9
|
|
Thanks again! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: killianmuldoon 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 |
What this PR does / why we need it:
I found SortForCreate to be very inefficient, it's basically O(n^2), it also needlessly allocates. In order to keep the contract (even though this exported function is not widely used), I didn't change this function's behaviour (i.e. its purity),
but I did deprecate it. (It could've been even faster if we opted for an in-place sort, but I didn't want to change the function's behaviour.)The function doesn't appear to be used in any tight loops or with a large number of elements, so it's a low prio PR, but it's something that could bite people applying a large number of small resources (see how it falls off a cliff with 10k elements).
Benchmark comparison
GH issue
Fixes #9253
/area util