-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Improve certificate condition checking and error logging #4298
Conversation
@inteon: The label(s) In response to this:
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. |
Hi @inteon. Thanks for your PR. I'm waiting for a jetstack or cert-manager 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. |
/ok-to-test |
180815f
to
6dd0c94
Compare
Signed-off-by: Inteon <42113979+inteon@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @inteon
I'm struggling to get my head round the changes and I've asked a bunch of (potentially stupid) questions below.
Please expand the comments for the helper functions so that it's clear to users like me how they work and when they should be used.
Also please write a bit more about exactly what is wrong with the existing code that causes the tests to still be flaky.
/kind cleanup |
Signed-off-by: Inteon <42113979+inteon@users.noreply.github.com>
4d27e2c
to
e388d68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @inteon
There's one thing that looks like a mistake (waiting for Certificate with name of Secret)
But all my other comments are optional.
Please address the points or answer them and unhold this if you think it's ok to merge.
/hold
/lgtm
|
||
By("changing the name of the corresponding secret in the cert") | ||
secretName := types.NamespacedName{Name: cert.Spec.SecretName, Namespace: f.Namespace.Name} | ||
cert = cert.DeepCopy() // DeepCopy before updating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, was that the cause of the intermittent failure?
Did you consider doing the DeepCopy in the WaitForCertificate...
helper functions, to help avoid this same bug happening elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be related, but I fear that there are multiple errors causing failures.
I would like to keep the cert = cert.DeepCopy()
visible in the code, so that we can more easily see when DeepCopies are needed.
Signed-off-by: Inteon <42113979+inteon@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the latest adjustments @inteon
/lgtm
/approve
/kind cleanup
By("Waiting for Certificate to exist") | ||
err := util.WaitForCertificateToExist(certClient, certificateSecretName, foreverTestTimeout) | ||
cert, err := f.Helper().WaitForCertificateToExist(f.Namespace.Name, certificateSecretName, time.Second*60) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here again, we seem to be using the name of the Secret as the name of the Certificate.
cert, err := f.Helper().WaitForCertificateToExist(f.Namespace.Name, certificateSecretName, time.Second*60) | |
cert, err := f.Helper().WaitForCertificateToExist(f.Namespace.Name, certificateName, time.Second*60) |
I guess it works because both names are the same.
I can't face another round of code review and E2E tests. Let's just leave it.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inteon, wallrj 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 |
/cherry-pick release-1.5 |
@wallrj: new pull request created: #4322 In response to this:
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. |
What this PR does / why we need it:
In #4234 there were intermittent test failures in the cainjector E2E tests:
https://prow.build-infra.jetstack.net/view/gs/jetstack-logs/pr-logs/pull/jetstack_cert-manager/4234/pull-cert-manager-e2e-v1-21/1422524737686343681
https://prow.build-infra.jetstack.net/view/gs/jetstack-logs/pr-logs/pull/jetstack_cert-manager/4234/pull-cert-manager-e2e-v1-21/1421109702468571136
In #4239, these flakes were partially resolved, although there is still an uncovered edge case where the flakes still persist.
This PR introduces more robust checking for both conditions Ready=true and Issuing not set.
Additionally it improves the logging in case of an error.
The logic for doing this is now aggregated in
test/e2e/framework/helper/certificates.go
.The certificate functions in
test/e2e/util/util.go
are now not used anymore and deprecation comments have been added.Release note:
/kind flake