Skip to content
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

Capture CNI logs in istioctl bug-report #45712

Merged
merged 3 commits into from
Jun 30, 2023
Merged

Conversation

syw14
Copy link
Contributor

@syw14 syw14 commented Jun 28, 2023

Issue #45466
Fetch Cni logs from kube-system namespace pods selected by k8s-app=istio-cni-node.

The logs are written into cni.log file per pod (not per container)

@syw14 syw14 requested a review from a team as a code owner June 28, 2023 17:26
@istio-policy-bot istio-policy-bot added area/user experience release-notes-none Indicates a PR that does not require release notes. labels Jun 28, 2023
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test labels Jun 28, 2023
@istio-testing
Copy link
Collaborator

Hi @syw14. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

tools/bug-report/pkg/cluster/cluster.go Outdated Show resolved Hide resolved
for _, cs := range r.Pod[PodKey(namespace, pod)].Status.ContainerStatuses {
func (r *Resources) ContainerRestarts(namespace, pod, container string, isCniPod bool) int {
var podItem *corev1.Pod
if isCniPod {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this? I thought r.Pod is a superset of r.CniPod

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added CniPod as a new field in Resource due to there's some pre defined config that will exclude all pods in namespace kube-system for r.Pod when building the "resource tree" of namespace/pod/container.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see...

It seems like a bit weird to have CNI code in ContainerRestarts, a bit fragile. Maybe it makes sense to have an AllPods one that isn't filtered...

Or we can keep this, not a big deal

@hanxiaop
Copy link
Member

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Jun 29, 2023
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Supporting other namespace would be great, can add in a followup if you want

for _, cs := range r.Pod[PodKey(namespace, pod)].Status.ContainerStatuses {
func (r *Resources) ContainerRestarts(namespace, pod, container string, isCniPod bool) int {
var podItem *corev1.Pod
if isCniPod {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see...

It seems like a bit weird to have CNI code in ContainerRestarts, a bit fragile. Maybe it makes sense to have an AllPods one that isn't filtered...

Or we can keep this, not a big deal

@istio-testing istio-testing merged commit 289bc68 into istio:master Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user experience ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. release-notes-none Indicates a PR that does not require release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants