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

Set cluster_name var in logs vars separately and not in init #2777

Merged
merged 2 commits into from
Mar 26, 2024

Conversation

viveksinghggits
Copy link
Contributor

@viveksinghggits viveksinghggits commented Mar 21, 2024

Change Overview

If we set the cluster_name field in log variables (extran fields that are printed in logs), inside the init function of log package, we would un necessarily be calling k8s api even from the places where we don't have to do anything with the k8s api.
To fix this, this commit introduces a new function that can be used to setup that field in the logs vars and that function will only be called from the places where we need the logs to have to the cluster_name field, for example kanctl, controller and repository-controller.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • fixes #issue-number

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

Detailed steps are part of a comment below.

If we set the `cluster_name` field in log variables (extran fields that are
printed in logs), inside the `init` function of `log` package, we would
un necessarily be calling k8s api even from the places where we don't have to
do anything with the k8s api.
To fix this, this commit introduces a new function that can be used to setup
that field in the logs vars and that function will only be called from the
places where we need the logs to have to the `cluster_name` field, for example
`kanctl`, `controller` and `repository-controller`.
@viveksinghggits
Copy link
Contributor Author

viveksinghggits commented Mar 21, 2024

In current version kando just run kando or kando output a b command and we would
see in the output of tcpdump that a connection is being made.

11:38:35.721885 IP ubukando-755d98cc67-wxgwp.33736 > kubernetes.default.svc.cluster.local.443: Flags [S], seq 626173369, win 64240, options [mss 1460,sackOK,TS val 17765482 ecr 0,nop,wscale 7], length 0

Build new binary and test these kando commands

kando

no comms being made to apiserver, verified by output of tcpdump.

kando output ab cd

no comms being made to apiserver, verified by looking at output of tcpdump.

kando location push

kando location push --profile 'profilejson' hello.txt --path test-path

we see a lot of messages in the output of tcpdump but none of those
are to kubernetes.default but they are to s3 and that is obvious.

tcpdump logs are here.

kando location pull

kando location pull --profile 'profilejson' --path test-path hello

and tcpdump logs can be verified to make sure no connections are made to apiserver.

tcpdump logs are here.

Build new controller binary and use it in kanister installation and make sure that the logs have cluster_name properly set in the output.

Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit 510494c into master Mar 26, 2024
14 checks passed
@mergify mergify bot deleted the initclustername-diff branch March 26, 2024 17:11
mabhi pushed a commit that referenced this pull request Mar 28, 2024
If we set the `cluster_name` field in log variables (extran fields that are
printed in logs), inside the `init` function of `log` package, we would
un necessarily be calling k8s api even from the places where we don't have to
do anything with the k8s api.
To fix this, this commit introduces a new function that can be used to setup
that field in the logs vars and that function will only be called from the
places where we need the logs to have to the `cluster_name` field, for example
`kanctl`, `controller` and `repository-controller`.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants