-
Notifications
You must be signed in to change notification settings - Fork 404
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 6435: System Test Logging #6445
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Colin Hryniowski <colin.hryniowski@dell.com>
Signed-off-by: Colin Hryniowski <colin.hryniowski@dell.com>
Signed-off-by: Colin Hryniowski <colin.hryniowski@dell.com>
Signed-off-by: Colin Hryniowski <colin.hryniowski@dell.com>
Signed-off-by: Colin Hryniowski <colin.hryniowski@dell.com>
…d up. Signed-off-by: Colin Hryniowski <colin.hryniowski@dell.com>
Signed-off-by: Colin Hryniowski <colin.hryniowski@dell.com>
Codecov Report
@@ Coverage Diff @@
## master #6445 +/- ##
============================================
- Coverage 86.26% 86.26% -0.01%
+ Complexity 15434 15431 -3
============================================
Files 1008 1008
Lines 57760 57760
Branches 5885 5885
============================================
- Hits 49828 49826 -2
Misses 4870 4870
- Partials 3062 3064 +2
Continue to review full report at Codecov.
|
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 left some comments. Please, let us know the result of multiple runs of system tests and post here the output of the new log messages to demonstrate that it meets the expected goal.
test/system/src/main/java/io/pravega/test/system/framework/kubernetes/K8sClient.java
Outdated
Show resolved
Hide resolved
long runCount = pods.getItems().stream().map(pod -> pod.getStatus()).filter(this::isPodRunning).count(); | ||
long totalCount = pods.getItems().size(); | ||
|
||
if (runCount == totalCount && runCount == expectedPodCount) { |
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.
Would it be the same as saying totalCount == expectedPodCount
?
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.
No, because the totalCount
will not always be the same value as the runCount
. If we are expecting 3 pods to be running, but only have two running pods and another pod that has been terminated (another one has yet to be spun up, and the terminated pod yet to be cleaned up) we will still have a totalCount
of 3, an expectedPodCount
of 3 and a runCount
of 2.
.stream() | ||
.map(pod -> pod.getMetadata().getName()) | ||
.collect(Collectors.toList()); | ||
log.info("Set of running pods after scale event: {}", names); |
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.
What is the output of this?
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 prints the names of the pods that belong to the current set of running pods for some resource (i.e. Pravega or BookKeeper cluster). I found this to be useful information to have readily available to make parsing through the logs (our fluent-bit ones) easier. It helps to know what the set of active pods were during some given system test.
test/system/src/main/java/io/pravega/test/system/framework/kubernetes/K8sClient.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Colin Hryniowski <colin.hryniowski@dell.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.
LGTM with one minor comment regarding the log line.
// Break out of loop. | ||
retriesRemaining.set(0); | ||
} else if (retriesRemaining.get() == 0) { | ||
log.error("Retries exhausted waiting for pod(s): <{}={}>", labelName, labelValue); |
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: Can we print the expected count and the current count here in the same log line ? This will simplify debugging.
Change log description
waitUntilPodIsRunning
wait for both the expected amount of active pods be available, as well as ensuring no pods remain in a terminated state, i.e. are properly removed.waitUntilPodIsRunning
.Purpose of the change
Fixes #6435
What the code does
See #6435.
How to verify it
Run system tests.