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
add logs when pod failed to start when using WaitUntilPodAvailableE
#1252
base: master
Are you sure you want to change the base?
Conversation
@denis256 we'd love to be able to use this change in kumahq/kuma any chance to get a review? |
modules/k8s/errors.go
Outdated
@@ -68,7 +68,7 @@ type PodNotAvailable struct { | |||
|
|||
// Error is a simple function to return a formatted error message as a string | |||
func (err PodNotAvailable) Error() string { | |||
return fmt.Sprintf("Pod %s is not available, reason: %s, message: %s", err.pod.Name, err.pod.Status.Reason, err.pod.Status.Message) | |||
return fmt.Sprintf("Pod %s is not available, reason: %s, message: %s, phase: %s, conditions: %+v", err.pod.Name, err.pod.Status.Reason, err.pod.Status.Message, err.pod.Status.Phase, err.pod.Status.Conditions) |
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.
I was wondering if can be added tests to track generated errors
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.
You mean Unit Tests or integration tests? Unit tests for sure I can add some.
For integration tests I'll need a bit of hand holding (at least pointers to similar tests) as I don't seem to get how terratest is e2e tested.
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.
Just added a bunch of tests. I've only reworked things a little to make less verbose and able to detect more complex failures.
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.
@denis256 any input here?
Fix gruntwork-io#1251 Signed-off-by: Charly Molter <charly.molter@konghq.com>
test/kubernetes_failed_pod_test.go
Outdated
return err | ||
} | ||
|
||
func TestUnknownImage(t *testing.T) { |
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.
For K8S tests in the test
directory, naming convention for functions is TestKubernetes*
, with current names - tests functions weren't executed in CICD
Examples:
https://github.com/gruntwork-io/terratest/blob/master/test/kubernetes_basic_example_test.go#L25
break | ||
} | ||
logger.Logf(t, "Wait %s before checking if pod %s is still available", sleepBetweenRetries, podName) | ||
time.Sleep(sleepBetweenRetries) |
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 logic looks to be for a quite specific case: pod becomes ready and after a while changes status to not ready - not sure if should be included in PR for adding additional logs when pod fail to start
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.
Up to you I can either do this as a separate issue or update PR title. The reason why this was added here is that in the case of a failing startup the pod may become ready and very quickly switch to not ready.
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.
I think this should go as a separate feature
Noticed that some of added Kubernetes are failing
|
Description
Fixes #1251.
Here's an example of this change in kumahq/kuma:
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added more information when pod fail to start
Migration Guide