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

Migrate pkg/proxy/util to structured logging #104908

Merged
merged 11 commits into from
Sep 20, 2021
Merged

Migrate pkg/proxy/util to structured logging #104908

merged 11 commits into from
Sep 20, 2021

Conversation

CIPHERTron
Copy link
Member

@CIPHERTron CIPHERTron commented Sep 10, 2021 •
edited
Loading

What type of PR is this?

/wg structured-logging
/area logging
/priority important-longterm
/kind cleanup
/cc @kubernetes/wg-structured-logging-reviews

What this PR does / why we need it:

Migrate kube-proxy to structured logs

Which issue(s) this PR fixes:

Reference #104872

Special notes for your reviewer:

Does this PR introduce a user-facing change?

migrate pkg/proxy to structured logs

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 10, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @CIPHERTron!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @CIPHERTron. Thanks for your PR.

I'm waiting for a kubernetes 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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 10, 2021
@serathius
Copy link
Contributor

This PR is not correctly tagged for migration, please read instructions in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#what-to-include-in-the-pull-request

@CIPHERTron CIPHERTron changed the title Migrate to Structured Logs in pkg/proxy/util Migrate pkg/proxy/util to structured logging Sep 10, 2021
@CIPHERTron
Copy link
Member Author

/wg structured-logging
/area logging
/priority important-longterm
/cc @kubernetes/wg-structured-logging-reviews

@k8s-ci-robot k8s-ci-robot added wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging. area/logging priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 10, 2021
@CIPHERTron
Copy link
Member Author

This PR is not correctly tagged for migration, please read instructions in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#what-to-include-in-the-pull-request

Sorry for the inconvenience @serathius. Is it okay now?

@bowei
Copy link
Member

bowei commented Sep 10, 2021

/ok-to-test

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 10, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, CIPHERTron

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2021
@CIPHERTron
Copy link
Member Author

In my latest commit, I've changed the key names in endpoints.go as requested by @shivanshu1333 👍🏻

@shivanshuraj1333
Copy link
Contributor

@CIPHERTron can you please fix the issue in your latest commit? Thanks!

@CIPHERTron
Copy link
Member Author

@CIPHERTron can you please fix the issue in your latest commit? Thanks!

I'm not quite sure what's causing the tests to fail. Could you please help me out?

@shivanshuraj1333
Copy link
Contributor

shivanshuraj1333 commented Sep 16, 2021 •
edited
Loading

This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

see 39a6952

@CIPHERTron
Copy link
Member Author

This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

see 39a6952

Isn't it supposed to be a commit from a branch on my fork?
Like, for this PR, every commit belongs to CIPHERTron/kubernetes fork's structured-logs branch
Or maybe I'm not getting what the error means. Pretty new to it actually 😅

@karuppiah7890
Copy link

karuppiah7890 commented Sep 16, 2021 •
edited
Loading

@CIPHERTron Nothing is wrong with your commits. Your commits do belong to the branch of your fork. Turns out Kubernetes bot is not brainy enough to use the right link for the commits in this PR in it's comment OR there is some issue with GitHub linking issues, which seems to be the case with some experimentation. The right link for the commit is - this or this. If you simply try to paste the kubernetes/kubernetes repo based commit link as is, that is this link https://github.com/kubernetes/kubernetes/pull/104908/commits/39a69521438526b37d57f4e5dad53ea14b662298 in a GitHub comment, it gets rendered like this - 39a6952 and when you hover over it or click it, it leads to the wrong link because GitHub seems to have some issue with rendering the links properly with proper anchor tag. The wrong link shows code though, but says it does not belong to any branch of the kubernetes/kubernetes repo which is true, as it's still a PR and it's part of your fork and when it gets merged, it will get a different commit SHA too however. Kubernetes bot can maybe just use different way to link to the commits I guess. Or we gotta report to GitHub

So, that's the whole confusion I think. It's no way related to the test failure. The test failure is here

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/104908/pull-kubernetes-unit/1438452621122736128

k8s.io/kubernetes/vendor/k8s.io/legacy-cloud-providers/azure: TestCreateBlobDisk expand_less | 0s
-- | --
=== RUN   TestCreateBlobDisk panic: test timed out after 3m0s  goroutine 232 [running]: testing.(*M).startAlarm.func1() 	/usr/local/go/src/testing/testing.go:1788 +0xbb created by time.goFunc 	/usr/local/go/src/time/sleep.go:180 +0x4a  goroutine 1 [chan receive]: testing.(*T).Run(0xc0002921a0, {0x251cb43, 0x12}, 0x25e6c40)

as mentioned in bot's comment with the link test as link

thockin
thockin reviewed Sep 16, 2021
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

sorry to nit-pick. (not really sorry).

In general I think we want key names to be "lowerBumpyCaps" - "cidr" not "CIDR", etc

pkg/proxy/util/endpoints.go Outdated Show resolved Hide resolved
pkg/proxy/util/endpoints.go Outdated Show resolved Hide resolved
pkg/proxy/util/endpoints.go Outdated Show resolved Hide resolved
pkg/proxy/util/endpoints.go Outdated Show resolved Hide resolved
pkg/proxy/util/iptables/traffic.go Outdated Show resolved Hide resolved
pkg/proxy/util/iptables/traffic.go Outdated Show resolved Hide resolved
pkg/proxy/util/utils.go Outdated Show resolved Hide resolved
pkg/proxy/util/utils.go Outdated Show resolved Hide resolved
pkg/proxy/util/utils.go Outdated Show resolved Hide resolved
@shivanshuraj1333
Copy link
Contributor

@CIPHERTron can you please take a look at the review comments,
Thanks!

@CIPHERTron
Copy link
Member Author

@CIPHERTron can you please take a look at the review comments,
Thanks!

Yeah, sorry about that. Was caught up with something else today!

@CIPHERTron
Copy link
Member Author

Heyy @thockin, I've addressed all your suggestions. Could you please do a final check and say if everything's good to go or it needs some more changes? Thanks a lot!
/cc @serathius

@shivanshuraj1333
Copy link
Contributor

/test pull-kubernetes-integration

1 similar comment
@shivanshuraj1333
Copy link
Contributor

/test pull-kubernetes-integration

@CIPHERTron
Copy link
Member Author

Heyy folks, do I have to squash merge it from my side? Thanks!
/cc @serathius @thockin @shivanshu1333

@k8s-ci-robot
Copy link
Contributor

@CIPHERTron: GitHub didn't allow me to request PR reviews from the following users: shivanshu1333.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Heyy folks, do I have to squash merge it from my side? Thanks!
/cc @serathius @thockin @shivanshu1333

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.

@serathius
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 20, 2021
@serathius
Copy link
Contributor

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2021
@k8s-ci-robot k8s-ci-robot merged commit 060f5b8 into kubernetes:master Sep 20, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 20, 2021
@shivanshuraj1333 shivanshuraj1333 mentioned this pull request Sep 22, 2021
Closed
7 tasks
@CIPHERTron CIPHERTron mentioned this pull request Sep 23, 2021
Closed
7 tasks
ibabou pushed a commit to ibabou/kubernetes that referenced this pull request Sep 28, 2021
@CIPHERTron @yangjunmyfm192085
Migrate pkg/proxy/util to structured logging (kubernetes#104908)
c0f02a6 
* Migrate to Structured Logs in `pkg/proxy/util`

* Minor fixes

* change key to cidr and remove namespace arg

* Update key from cidr to CIDR

Co-authored-by: JUN YANG <[email protected]>

* Update key cidr to CIDR

Co-authored-by: JUN YANG <[email protected]>

* Update key ip to IP

Co-authored-by: JUN YANG <[email protected]>

* Update key ip to IP

Co-authored-by: JUN YANG <[email protected]>

* Interchange svcNamespace and svcName

* Change first letter of all messages to capital

* Change key names in endpoints.go

* Change all keynames to lower bumby caps convention

Co-authored-by: JUN YANG <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yangjunmyfm192085 yangjunmyfm192085 yangjunmyfm192085 left review comments

@mengjiao-liu mengjiao-liu mengjiao-liu left review comments

@shivanshuraj1333 shivanshuraj1333 shivanshuraj1333 requested changes

@bowei bowei Awaiting requested review from bowei

@serathius serathius Awaiting requested review from serathius

@thockin thockin Awaiting requested review from thockin

Assignees

@bowei bowei

@serathius serathius

@mengjiao-liu mengjiao-liu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/logging cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. triage/accepted Indicates an issue or PR is ready to be actively worked on. wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging.
Projects
None yet
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.
9 participants
@CIPHERTron @k8s-ci-robot @serathius @bowei @shivanshuraj1333 @yangjunmyfm192085 @mengjiao-liu @karuppiah7890 @thockin