-
Notifications
You must be signed in to change notification settings - Fork 55
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 OpenshiftStateRecorder #359
Conversation
*/ | ||
@Slf4j | ||
public class OpenshiftStateRecorder implements TestExecutionListener, LifecycleMethodExecutionExceptionHandler { | ||
private static final Path statusDir = Paths.get("log","status"); |
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.
Why two tabs for space? All the project uses one tab everywhere...
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.
@Crumby Fixed.
public class OpenshiftStateRecorder implements TestExecutionListener, LifecycleMethodExecutionExceptionHandler { | ||
private static final Path statusDir = Paths.get("log","status"); | ||
|
||
private final OpenShift openShift = OpenShifts.master(); |
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.
could you move initialisation into constructor to be consistent with rest?
|
||
writer.write("Events:"); | ||
writer.newLine(); | ||
for (Event event : openShift.getEvents()){ |
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 will get all events in past N hours. N is cluster wide configurable, typically 2 hours.
Is it possible to filter just events which are connected to current test? E.g. by timestamp filtering?
If events will be not filtered just to current test, then it does not add much. There are already events saved once testsuite finished - by EventsRecorder
.
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.
Nice catch I will look into this. Original purpose for this was to easily find out, when tests fails because of some image pull failure etc.
Aren't events are also limited by number? I think I never got more that about 100, but I'm not sure about this.
private void storePods(Path dir, OpenShift openshift){ | ||
for (Pod pod : openshift.getPods()) { | ||
try { | ||
openshift.storePodLog(pod, dir, pod.getMetadata().getName() + ".log"); |
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.
In xtf there is already PodLogRecorder
to store logs once test failed.
What does it adds on top of 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.
Nothing from pod logs themselfs. Why I added it to this is:
- It also records from build namespace.
- It is also recorded when Before* method fails. So it's possible to get logs from builds, non-tested services and other thing that can fail in setup.
writer.write("Namespace " + namespace + " state - " + LocalDateTime.now()); | ||
writer.newLine(); | ||
|
||
writer.write("Pods: (name ; status ; readyContainers/totalContainers ; containerRestart)"); |
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 is useful information. We need probably more stuff like 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.
I think overal this is good idea. We need to identify useful information to store. Could you also attach example of information stored?
@Override | ||
public void executionFinished(TestIdentifier testIdentifier, TestExecutionResult testExecutionResult) { | ||
try { | ||
if (FAILED.equals(testExecutionResult.getStatus()) && testIdentifier.isTest()) { |
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.
Could that be configurable, so I am able to store the state also for PASSED test, e.g. with something like xtf.config.store_state_always
?
By default it will store infor just for failed tests.
} | ||
} | ||
|
||
private void storeOpenshiftState(String testIdentifier) throws IOException { |
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 can imagine it could be useful to have method like this available in XTF public API. So when troubelshooting some test I can let store the state in some important points. E.g. before test and after test or some important place in the middle.
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.
Yes it could be worth it.
Any idea what is the right place to put it in? I think that Openshift class might be good, but not sure if it fits here.
Yes example log attached. |
Downgrade Fabric8 to 4.9.2
@mchoma yes, you are correct. We need to record isolated logs for test. And we need to consider parallelism hence we can not record everything in a namespace. I went through this PR and toke it as inspiration for some features: record always, record services, secrets,...However there will be some other differences. |
@mnovak1 yes for me. |
@mnovak1 Yes it can be closed. |
Add OpenshiftStateRecorder class. Serves to store info about openshift state, in case of test or build/deployment failure.
Also updates junit-jupiter to version 5.6.2, because LifecycleMethodExecutionExceptionHandler requires at least 5.5.
example-log.zip