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
[release 4.7] Bug 1913356: Implemented gathering specific logs from openshift apiserver operator #273
Conversation
Skipping CI for Draft Pull Request. |
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
/test all |
var records []record.Record | ||
|
||
for _, pod := range pods.Items { | ||
request := coreClient.Pods(namespace).GetLogs(pod.Name, &corev1.PodLogOptions{ |
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 relies on the fact that there's only one pod in the openshift-apiserver-operator
namespace right? It's probably minor but I would suggest to narrow down the filtering to one given pod, because there can be more pods in the future.
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.
it actually collects logs from all the pods from the given namespace
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 but I think we are interested only in "openshift-apiserver-operator" pod log right?
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.
Actually the pod's name is something like "openshift-apiserver-operator-6ddb679b87-4kn55" so we can only find all or some pods from namespace "openshift-apiserver-operator", I don't think there will be other unrelatable pods in this namespace
/hold |
6d2d825
to
c80f47e
Compare
@Sergey1011010: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
@Sergey1011010 Please rebase |
f4500a9
to
1186414
Compare
/retest |
}}, | ||
metav1.CreateOptions{}, | ||
) | ||
helpers.FailOnError(t, 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.
I am asking myself. Is it worth of adding this new dependency because of using one function in the tests?
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.
and I tend to say: No it's not :)
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.
Well, there are some other functions https://github.com/RedHatInsights/insights-operator-utils/tree/master/tests/helpers which we may need in the future, but I can remove it, no problems
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.
When looking at the change in go.sum
(toooo many new stuff) then I would remove it. I would keep the unit tests as simple as possible (we can improve the integration tests once we have the independent CI image). Thanks and sorry.
48bc50c
to
1186414
Compare
@Sergey1011010: This pull request references Bugzilla bug 1913356, which is invalid:
Comment In response to this:
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. |
/bugzilla refresh |
@Sergey1011010: This pull request references Bugzilla bug 1913356, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
/retest |
1 similar comment
/retest |
/unhold |
/lgtm tested it on clusterbot cluster, the logs weren't collected when the specified error wasn't present, and they were collected when the error was present |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0sewa0, Sergey1011010, tisnik 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
@Sergey1011010: All pull requests linked via external trackers have merged: Bugzilla bug 1913356 has been moved to the MODIFIED state. In response to this:
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. |
Categories
Sample archive
config/pod/{namespace-name}/logs/{pod-name}/errors.log
Documentation
docs/gathered-data.md
Unit Tests
pkg/gather/clusterconfig/openshift_apiserver_operator_logs_test.go
Privacy
Yes. There are no sensitive data in the newly collected information.
References
https://issues.redhat.com/browse/CCXDEV-3519
https://bugzilla.redhat.com/show_bug.cgi?id=1913356
https://access.redhat.com/solutions/5448851