-
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
tests/e2e.sh: Check for errors in the logs #819
Conversation
This is failing because the previous fix was not merged into master just into the release branch. Should be passing after #820 is merged. But it shows the e2e test in place works! 🎉 :) |
tests/e2e.sh
Outdated
echo "found errors in kube-state-metrics logs" | ||
echo "=======================================" | ||
echo "" | ||
echo "$output_logs" |
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.
So are we printing out the entire log here? Why not pipe the matched lines and echo that?
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.
@tariq1890 This is because before we were printing out the entire log as well, that was on an error. I think it's relevant if there are errors in the logs to print out the entire log so we can debug 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.
Makes sense. Thanks!
tests/e2e.sh
Outdated
klog_err=E$(date +%m%d) | ||
echo "check for errors in logs" | ||
output_logs=$(kubectl --namespace=kube-system logs deployment/kube-state-metrics kube-state-metrics) | ||
if echo "$output_logs" | grep "^$klog_err"; then |
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.
Nit: Let's use the brace var expansion for all the variable references you have added in this PR.
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.
PR looks good. Should be good to go after addressing the nits and rebase. Thanks @lilic !
tests/e2e.sh
Outdated
if echo "$output_logs" | grep "^$klog_err"; then | ||
echo "" | ||
echo "=======================================" | ||
echo "found errors in kube-state-metrics logs" |
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.
echo "found errors in kube-state-metrics logs" | |
echo "Found errors in the kube-state-metrics logs" |
kube-state-metrics does not terminate the pod on errors, so to actually detect the errors in CI, we are adding this check by grepping for errors in the kube-state-metrics logs. This makes the assumption that any errors that are present in logs start wtih the string 'E<MONTHDAY>' (e.g.: 'E0711') as this is how klog starts all error log lines.
@tariq1890 should be ready for a re-review, thanks! |
lgtm but giving @tariq1890 another chance to review :) |
/lgtm |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LiliC, tariq1890 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:
kube-state-metrics does not terminate the pod on errors, so to actually
detect the errors in CI, we are adding this check by grepping for errors
in the kube-state-metrics logs.
This makes the assumption that any errors that are present in logs start
wtih the string 'E' (e.g.: 'E0711') as this is how klog starts
all error log lines.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):As discussed in #815 (comment)
Tested this with errors in logs and it looks like this on FAIL: