Use private key fixtures for kubeadm unit tests #98664
Conversation
k8s-ci-robot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt 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 |
k8s-ci-robot
added
approved
|
/retest |
| @@ -442,7 +443,7 @@ func TestStaticPodControlPlane(t *testing.T) { | |||
| for i := range tests { | |||
| rt := tests[i] | |||
| t.Run(rt.description, func(t *testing.T) { | |||
| t.Parallel() | |||
| pkiutiltesting.Reset() | |||
There was a problem hiding this comment.
wondering, were the tests here and TestRenewCertsByComponent racy or should not run pkiutiltesting based test in parallel (it does have a lock for its functions)?
There was a problem hiding this comment.
These tests were made parallel in order to speed them up, but the reason they were slow was private key generation, not artificial waits in the tests. Running in parallel didn't help in CPU or random-constrained environments.
When using pkiutil/testing, we want a test to run, asking for private keys, then reset for the next test. Otherwise another test's Reset() could result in already-used private keys being handed out again to a still-running parallel test
| switch keyType { | ||
| case x509.ECDSA: | ||
| ecdsa++ | ||
| keyName = fmt.Sprintf("%d.ecdsa", ecdsa) |
There was a problem hiding this comment.
should we include pre-generated *.ecdsa files under testdata too?
yet, i can only find one tests that is using ECDSA, so currently it would not impact the performance:
kubernetes/cmd/kubeadm/app/util/pkiutil/pki_helpers_test.go
Lines 130 to 138 in 88a05df
There was a problem hiding this comment.
it seemed better to leave the unit tests in the pkiutil package alone (we don't want to replace NewPrivateKey when we're trying to test NewPrivateKey, etc)
There was a problem hiding this comment.
this is set up to only pre-generate the fixtures for private keys that are actually used, so I'd leave this as is for now
| if !more { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
call stack manipulation often needs unit tests on it's own, but this looks fine to me.
There was a problem hiding this comment.
the failure mode if this can't positively find a Test function in its stack is to fall back to default non-fixture behavior, which seems ok
|
thanks for the comments! /lgtm |
k8s-ci-robot
added
kind/cleanup
k8s-ci-robot
added
the
lgtm
|
/retest |
xref #98486
alternative to #98638
Private
Does this PR introduce a user-facing change?:
results in a ~75% speedup of kubeadm unit tests on my machine:
before:
after:
/cc @neolit123