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.

Sign up for GitHub

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

Jump to bottom

🌱 Rewrite fetch_target_logs.sh in Golang #1763

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🌱 Rewrite fetch_target_logs.sh in Golang #1763

wants to merge 1 commit into from

Conversation

peppi-lotta
Copy link
Member

@peppi-lotta peppi-lotta commented Jun 13, 2024 •
edited
Loading

What this PR does / why we need it: Running an sh file from go file will gives warning: G204: Audit use of command execution. In this PR I've rewritten the fetch_target_logs.sh in Go to avoid getting this warning.

Compared to the original: I've added kubect get pods -A
and removed the split between stdout and stderr in the logs. Some of the logs had warning

W0613 11:04:05.645729       1 options.go:215] 
==== Removed Flag Warning ======================

logtostderr is removed in the k8s upstream and has no effect any more.

and based on further investigation it seems Kubernetes only has one API endpoint for logs and everything is logged into stdout.

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mboukhalfa for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 13, 2024
@peppi-lotta peppi-lotta changed the title 🌱 Rewrite fetch_target_logs.sh in Golang 🌱 WIP: Rewrite fetch_target_logs.sh in Golang Jun 13, 2024
tuminoid
tuminoid reviewed Jun 13, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Fix the build, and some comments on top.

test/go.mod Outdated
@@ -18,6 +18,7 @@ require (
k8s.io/apimachinery v0.29.5
k8s.io/client-go v0.29.5
k8s.io/klog/v2 v2.110.1
k8s.io/kubectl v0.29.3
Copy link
Member

Choose a reason for hiding this comment

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

Match the version with other k8s.io deps, ie. v0.29.5.

test/e2e/fetch_target_logs.go Outdated
}
defer podLogs.Close()

// Read the logs into a string
Copy link
Member

Choose a reason for hiding this comment

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

This only reads stdout. Original script reads also stderr.

test/e2e/fetch_target_logs.go Outdated
// kubectl --kubeconfig="${KUBECONFIG_WORKLOAD}" describe pods -n "${NAMESPACE}" "${POD}"
describerSettings := describe.DescriberSettings{
ShowEvents: true,

Copy link
Member

Choose a reason for hiding this comment

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

empty line

test/e2e/fetch_target_logs.go Outdated
if err != nil {
log.Fatal(err)
}

Copy link
Member

Choose a reason for hiding this comment

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

Would you add here also equivalent of kubectl --kubeconfig .... get pods -A before namespace iteration, we've been missing that.

Follow-up PR is also OK, but since you want to get this merged before vacations, adding it here makes sense.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/fetch_target_logs.sh_to_golang branch 9 times, most recently from 1f60260 to bb6e1ab Compare June 14, 2024 13:44
@peppi-lotta peppi-lotta changed the title 🌱 WIP: Rewrite fetch_target_logs.sh in Golang 🌱 Rewrite fetch_target_logs.sh in Golang Jun 14, 2024
@peppi-lotta
Copy link
Member Author

/test metal3-centos-e2e-integration-test-main
/test metal3-ubuntu-e2e-integration-test-main

@peppi-lotta
Copy link
Member Author

/test ?

@metal3-io-bot
Copy link
Contributor

@peppi-lotta: The following commands are available to trigger required jobs:

  • /test build
  • /test generate
  • /test gomod
  • /test manifestlint
  • /test markdownlint
  • /test metal3-centos-e2e-integration-test-main
  • /test metal3-ubuntu-e2e-integration-test-main
  • /test shellcheck
  • /test test
  • /test unit

The following commands are available to trigger optional jobs:

  • /test metal3-centos-e2e-basic-test-main
  • /test metal3-centos-e2e-feature-test-main
  • /test metal3-e2e-1-26-1-27-upgrade-test-main
  • /test metal3-e2e-1-27-1-28-upgrade-test-main
  • /test metal3-e2e-1-28-1-29-upgrade-test-main
  • /test metal3-e2e-clusterctl-upgrade-test-main
  • /test metal3-ubuntu-e2e-basic-test-main
  • /test metal3-ubuntu-e2e-feature-test-main

Use /test all to run the following jobs that were automatically triggered:

  • build
  • generate
  • gomod
  • manifestlint
  • shellcheck
  • unit

In response to this:

/test ?

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.

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2024
adilGhaffarDev
adilGhaffarDev reviewed Jun 20, 2024
View reviewed changes
test/e2e/fetch_target_logs.go Outdated
"k8s.io/kubectl/pkg/describe"
)

func FetchTargetLogs() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func FetchTargetLogs() {
func FetchLogs(clusterProxy framework.ClusterProxy) {

We can pass clusterProxy and make this function reusable. If in the future we want to collect logs from some other cluster it will be very helpful.

@Rozzii Rozzii added this to the 1.8.0 milestone Jun 28, 2024
@adilGhaffarDev
Copy link
Member

Let's also implement CAPI CollectInfrastructureLogs on our side in this PR.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/fetch_target_logs.sh_to_golang branch from bb6e1ab to 508874b Compare August 8, 2024 10:16
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2024
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/fetch_target_logs.sh_to_golang branch 3 times, most recently from b0362fc to 17bd439 Compare August 8, 2024 11:18
@peppi-lotta
Copy link
Member Author

/test ?

@metal3-io-bot
Copy link
Contributor

@peppi-lotta: The following commands are available to trigger required jobs:

  • /test build
  • /test generate
  • /test gomod
  • /test manifestlint
  • /test markdownlint
  • /test metal3-centos-e2e-integration-test-main
  • /test metal3-ubuntu-e2e-integration-test-main
  • /test shellcheck
  • /test test
  • /test unit

The following commands are available to trigger optional jobs:

  • /test metal3-centos-e2e-basic-test-main
  • /test metal3-centos-e2e-feature-test-main-features
  • /test metal3-centos-e2e-feature-test-main-pivoting
  • /test metal3-centos-e2e-feature-test-main-remediation
  • /test metal3-e2e-1-26-1-27-upgrade-test-main
  • /test metal3-e2e-1-27-1-28-upgrade-test-main
  • /test metal3-e2e-1-28-1-29-upgrade-test-main
  • /test metal3-e2e-clusterctl-upgrade-test-main
  • /test metal3-ubuntu-e2e-basic-test-main
  • /test metal3-ubuntu-e2e-feature-test-main-features
  • /test metal3-ubuntu-e2e-feature-test-main-pivoting
  • /test metal3-ubuntu-e2e-feature-test-main-remediation

Use /test all to run the following jobs that were automatically triggered:

  • build
  • generate
  • gomod
  • manifestlint
  • shellcheck
  • unit

In response to this:

/test ?

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-sigs/prow repository.

@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-feature-test-main-pivoting

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/fetch_target_logs.sh_to_golang branch from 7979c3a to 64f1b6d Compare August 30, 2024 08:32
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-feature-test-main-pivoting

1 similar comment
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-feature-test-main-pivoting

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/fetch_target_logs.sh_to_golang branch from 64f1b6d to 5ca9936 Compare September 3, 2024 07:56
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-feature-test-main-pivoting

1 similar comment
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-feature-test-main-pivoting

@tuminoid
Copy link
Member

tuminoid commented Sep 3, 2024

/test metal3-ubuntu-e2e-feature-test-main-pivoting

Periodic pivoting test has not passed in 27 days, not sure if its worth running unless it is specifically triggering and failing to somehting you created in this PR.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/fetch_target_logs.sh_to_golang branch 2 times, most recently from bece774 to d081c3a Compare September 5, 2024 09:57
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-integration-test-main

@adilGhaffarDev
Copy link
Member

It's looking good. A few improvements that we made in the log collection:

  • Name folders according to the actual name of the cluster, preferably also the name of the test as the folder name. Like this:

    • Logs
      • k8s-target-cluster
        • cluster-name
          • machine-name
            • pod-name
              • pod-name.log
      • k8s-management-cluster
        • cluster-name
          • machine-name
            • pod-name
              • pod-name.log

    Why is this needed, we have multiple target clusters in clusterctl tests, in future we might add tests where we have multiple clusters.

  • In logs describe yamls that are missing lets bring them back. If it's difficult to put the yamls in the same folder as logs(because we use capi code to collect logs) put them in a separate folder, you can add the same code as before to collect yamls. Let's keep the same structure as above:

    • pod-yamls
      • k8s-target-cluster
        • cluster-name
          • machine-name
            • pod-name
              • pod-name.yaml
      • k8s-management-cluster
        • cluster-name
          • machine-name
            • pod-name
              • pod-name.yaml

let's skip infrastructure logs for now.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/fetch_target_logs.sh_to_golang branch 4 times, most recently from 53cab2a to df6e0bc Compare September 17, 2024 09:48
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-integration-test-main

@peppi-lotta peppi-lotta changed the title 🌱 WIP: Rewrite fetch_target_logs.sh in Golang 🌱 Rewrite fetch_target_logs.sh in Golang Sep 19, 2024
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/fetch_target_logs.sh_to_golang branch from df6e0bc to 149df7d Compare September 19, 2024 07:26
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-integration-test-main

@metal3-io-bot
Copy link
Contributor

metal3-io-bot commented Sep 19, 2024 •
edited
Loading

@peppi-lotta: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
metal3-ubuntu-e2e-feature-test-main-pivoting 5ca9936 link false /test metal3-ubuntu-e2e-feature-test-main-pivoting

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-sigs/prow repository. I understand the commands that are listed here.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/fetch_target_logs.sh_to_golang branch from 149df7d to 6d6cb21 Compare September 19, 2024 11:07
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-integration-test-main

@metal3-io-bot metal3-io-bot added needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 19, 2024
@metal3-io-bot
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

1 similar comment
@metal3-io-bot
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuminoid tuminoid tuminoid left review comments

@mquhuy mquhuy mquhuy left review comments

@adilGhaffarDev adilGhaffarDev adilGhaffarDev left review comments

@mboukhalfa mboukhalfa Awaiting requested review from mboukhalfa

@Sunnatillo Sunnatillo Awaiting requested review from Sunnatillo

Assignees
No one assigned
Labels
needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Metal3 - Roadmap
Status: CAPM3 on hold / blocked
Milestone
CAPM3 - 1.9.0
Development

Successfully merging this pull request may close these issues.
7 participants
@peppi-lotta @metal3-io-bot @adilGhaffarDev @mboukhalfa @tuminoid @mquhuy @Rozzii