-
Notifications
You must be signed in to change notification settings - Fork 1.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
Issue #6006 TestCustomResources with better logging and more precision #6068
Conversation
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.
@coryrc: 0 warnings.
In response to this:
Fixes #6006
Given the reasoning in issue #6006, (that K8s doesn't actually HAVE to kill anything that exceeds memory limits), the v1 api version is being moved to an e2e test. Logging has been added for better debugability.
/assign @dgerd
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.
(The first commit is actually #5984 and I will rebase this once that is approved) |
This should remove the v1alpha1 and v1beta1 tests. They have the same reasons to not be in conformance. |
It does delete v1alpha1 & v1beta1/resources_test.go ... |
Before errors between the test image and the test could cause false positives, but now that is much less likely. Also prints more logs at higher debugging levels and fewer logs at lower levels than prior.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: coryrc 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 |
} | ||
if response.StatusCode != http.StatusOK { | ||
return fmt.Errorf("StatusCode = %d, want %d", response.StatusCode, http.StatusOK) | ||
klog.V(5).Infof("Received error '%+v' from sendPostRequest (may be expected)\n", err) |
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.
Not sure I see the value in logging the error if the error is expected. Why not put this down in the "wantSuccess" statement?
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.
That's why this is at trace level, V(5). I found it valuable when it was a conformance test.
t.Fatalf("We shouldn't have got a response from bloating RAM with %d MBs", mb) | ||
} | ||
} else if response.StatusCode == http.StatusBadRequest { | ||
t.Error("Test Issue: Received BadRequest from test app, which probably means the test & test image are not cooperating with each other.") |
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 error seems unnecessarily long and should include the request that was sent to make debugging easier.
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.
Do you think this information should not be provided in the log -- like move the second half to a comment?
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.
As for including request, if people run at verbosity 6 through 9 they can see the request, so is that not sufficient?
The diff seemed off before your force push. Looks better now. |
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:
|
/retest |
/hold |
Superseded by #6978 |
@coryrc: PR needs rebase. 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. |
Fixes #6006
Given the reasoning in issue #6006, (that K8s doesn't actually HAVE to kill anything that exceeds memory limits), the v1 api version is being moved to an e2e test. Logging has been added for better debugability.
/assign @dgerd