Migrated pkg/proxy/ipvs to structured logging #104932

shivanshuraj1333 · 2021-09-11T11:46:41Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrate pkg/proxy/ipvs to structured logging

Which issue(s) this PR fixes:

Fixes #
Ref #104872

Special notes for your reviewer:

Does this PR introduce a user-facing change?

migrated pkg/proxy/ipvs to structured logging

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

k8s-ci-robot · 2021-09-11T11:46:50Z

Hi @shivanshu1333. 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.

pacoxu · 2021-09-13T03:28:52Z

migrate pkg/proxy/ipvs/graceful_termination.go to structured logs #98684 was opened for pkg/proxy/ipvs/graceful_termination.go only and got lgtm
Migrate pkg/proxy/ipvs/ipset.go logs to structured logging #102031 was opened for pkg/proxy/ipvs/ipset.go only and was not reviewed.

Remember to close them if this is merged.

shivanshuraj1333 · 2021-09-13T06:13:20Z

Yes, sure

shivanshuraj1333 · 2021-09-13T06:14:46Z

@danwinship, @rramkumar1 kindly review

serathius · 2021-09-13T08:31:51Z

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

shivanshuraj1333 · 2021-09-14T05:06:43Z

/auto-cc

shivanshuraj1333 · 2021-09-21T18:55:20Z

pkg/proxy/ipvs/graceful_termination.go

 	m.rsList.add(ele)
 	return nil
 }

 func (m *GracefulTerminationManager) deleteRsFunc(rsToDelete *listItem) (bool, error) {
-	klog.V(5).Infof("Trying to delete rs: %s", rsToDelete.String())
+	klog.V(5).InfoS("Trying to delete real server", "realServer", rsToDelete)


here I've removed .String() as it is a helper function to convert real server to string.
Structured logging does this by default, and I've made every logging statement consistent with having "key" - "value" pairs.

log output using klog.V(5).InfoS("Deleting real server", "realServers", rsToDelete.String()). *
I0921 23:54:10.037778 58346 graceful_termination.go:173] "Deleting real server" realServer="1.1.1.1:80/tcp/10.0.0.1:80"

klog.V(5).InfoS("Deleting real server", "realServer", rsToDelete) *
I0921 23:56:34.439093 58827 graceful_termination.go:173] "Deleting real server" realServer="1.1.1.1:80/tcp/10.0.0.1:80"

Thanks for testing this and providing examples! Looks better!

shivanshuraj1333 · 2021-09-21T18:57:42Z

@serathius kindly re-review changes, thanks!

shivanshuraj1333 · 2021-09-21T19:05:45Z

/test pull-kubernetes-e2e-gci-gce-ipvs

serathius · 2021-09-21T20:39:26Z

pkg/proxy/ipvs/graceful_termination.go

@@ -167,10 +167,10 @@ func (m *GracefulTerminationManager) deleteRsFunc(rsToDelete *listItem) (bool, e
 			//     (existing connections will be deleted on the next packet because sysctlExpireNoDestConn=1)
 			// For other protocols, don't delete until all connections have expired)
 			if utilipvs.IsRsGracefulTerminationNeeded(rsToDelete.VirtualServer.Protocol) && rs.ActiveConn+rs.InactiveConn != 0 {
-				klog.V(5).Infof("Not deleting, RS %v: %v ActiveConn, %v InactiveConn", rsToDelete.String(), rs.ActiveConn, rs.InactiveConn)
+				klog.V(5).InfoS("Not deleting real server, active connection, and inactive connection", "realServer", rsToDelete, "realServerActiveConnection", rs.ActiveConn, "realServerInactiveConnection", rs.InactiveConn)


Suggested change

klog.V(5).InfoS("Not deleting real server, active connection, and inactive connection", "realServer", rsToDelete, "realServerActiveConnection", rs.ActiveConn, "realServerInactiveConnection", rs.InactiveConn)

klog.V(5).InfoS("Skip deleting real server till all connection have expired", "realServer", rsToDelete, "activeConnection", rs.ActiveConn, "inactiveConnection", rs.InactiveConn)

Ahh yes, this is better

serathius · 2021-09-21T20:43:14Z

pkg/proxy/ipvs/ipset.go

@@ -191,13 +191,13 @@ func ensureIPSet(set *IPSet) error {
 func checkMinVersion(vstring string) bool {
 	version, err := utilversion.ParseGeneric(vstring)
 	if err != nil {
-		klog.Errorf("vstring (%s) is not a valid version string: %v", vstring, err)
+		klog.ErrorS(err, "Vstring is not a valid version string", "versionString", vstring)


Suggested change

klog.ErrorS(err, "Vstring is not a valid version string", "versionString", vstring)

klog.ErrorS(err, "Got invalid version string", "versionString", vstring)

mentioning name of variable in logs is not useful as simple rename will result in this log line not making sense.

Yes, makes sense, thanks!

serathius · 2021-09-21T20:44:21Z

pkg/proxy/ipvs/ipset.go

 		return false
 	}

 	minVersion, err := utilversion.ParseGeneric(MinIPSetCheckVersion)
 	if err != nil {
-		klog.Errorf("MinCheckVersion (%s) is not a valid version string: %v", MinIPSetCheckVersion, err)
+		klog.ErrorS(err, "MinCheckVersion is not a valid version string", "minCheckVersion", MinIPSetCheckVersion)


Suggested change

klog.ErrorS(err, "MinCheckVersion is not a valid version string", "minCheckVersion", MinIPSetCheckVersion)

klog.ErrorS(err, "Got invalid version string", "versionString", MinIPSetCheckVersion)

Yes, it's similar to the previous suggestion, fixed it too, thank you very much for suggesting, slowly I'm getting better at this :)

shivanshuraj1333 · 2021-09-22T09:06:58Z

/retest

shivanshuraj1333 · 2021-09-23T18:12:19Z

@serathius kindly re-review, thanks!

serathius · 2021-09-29T19:37:02Z

/lgtm
/assign @lbernail

shivanshuraj1333 · 2021-10-05T09:20:00Z

pinging @thockin for suggestions/approval
Thanks!

shivanshuraj1333 · 2021-10-14T15:34:02Z

/cc @aojea

aojea · 2021-10-14T15:43:34Z

/label tide/merge-method-squash
/approve
Thanks

k8s-ci-robot · 2021-10-14T15:44:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, shivanshu1333

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:

~~pkg/proxy/ipvs/OWNERS~~ [aojea]

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

* migrated ipset.go * migrated graceful_termination.go * fixed vstring * fixed ip set entry, made it consistent * fixed rs logging * resolving review comments for key graceful_termination.go * refactoring ipset.go * included review changes

shivanshuraj1333 added 2 commits September 11, 2021 16:58

migrated ipset.go

13c14a7

migrated graceful_termination.go

d0bd4c4

k8s-ci-robot added area/ipvs 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 11, 2021

k8s-ci-robot requested review from danwinship and rramkumar1 September 11, 2021 11:47

shivanshuraj1333 changed the title ~~Migrated pkg/proxy/ipvs to structured~~ Migrated pkg/proxy/ipvs to structured logging Sep 11, 2021

shivanshuraj1333 mentioned this pull request Sep 11, 2021

Kube-proxy migration to structured logs #104872

Closed

k8s-ci-robot requested a review from andrewsykim September 14, 2021 05:07

shivanshuraj1333 added 2 commits September 21, 2021 23:59

resolving review comments for key graceful_termination.go

bbc88a2

refactoring ipset.go

7e94569

shivanshuraj1333 commented Sep 21, 2021

View reviewed changes

serathius reviewed Sep 21, 2021

View reviewed changes

included review changes

e32c714

shivanshuraj1333 mentioned this pull request Sep 22, 2021

REQUEST: New membership for shivanshu1333 kubernetes/org#2993

Closed

7 tasks

k8s-ci-robot assigned serathius Sep 29, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2021

k8s-ci-robot requested a review from aojea October 14, 2021 15:34

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 14, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2021

k8s-ci-robot merged commit 7d9a6d1 into kubernetes:master Oct 14, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 14, 2021

This was referenced Oct 14, 2021

migrate pkg/proxy/ipvs/graceful_termination.go to structured logs #98684

Closed

Migrate pkg/proxy/ipvs/ipset.go logs to structured logging #102031

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrated pkg/proxy/ipvs to structured logging #104932

Migrated pkg/proxy/ipvs to structured logging #104932

shivanshuraj1333 commented Sep 11, 2021

k8s-ci-robot commented Sep 11, 2021

pacoxu commented Sep 13, 2021 •

edited

Loading

shivanshuraj1333 commented Sep 13, 2021

shivanshuraj1333 commented Sep 13, 2021

serathius commented Sep 13, 2021

shivanshuraj1333 commented Sep 14, 2021

shivanshuraj1333 Sep 21, 2021

serathius Sep 21, 2021

shivanshuraj1333 commented Sep 21, 2021

shivanshuraj1333 commented Sep 21, 2021

serathius Sep 21, 2021

shivanshuraj1333 Sep 22, 2021

serathius Sep 21, 2021

shivanshuraj1333 Sep 22, 2021

serathius Sep 21, 2021

shivanshuraj1333 Sep 22, 2021

shivanshuraj1333 commented Sep 22, 2021

shivanshuraj1333 commented Sep 23, 2021

serathius commented Sep 29, 2021

shivanshuraj1333 commented Oct 5, 2021

shivanshuraj1333 commented Oct 14, 2021

aojea commented Oct 14, 2021

k8s-ci-robot commented Oct 14, 2021

	klog.V(5).InfoS("Not deleting real server, active connection, and inactive connection", "realServer", rsToDelete, "realServerActiveConnection", rs.ActiveConn, "realServerInactiveConnection", rs.InactiveConn)
	klog.V(5).InfoS("Skip deleting real server till all connection have expired", "realServer", rsToDelete, "activeConnection", rs.ActiveConn, "inactiveConnection", rs.InactiveConn)

	klog.ErrorS(err, "Vstring is not a valid version string", "versionString", vstring)
	klog.ErrorS(err, "Got invalid version string", "versionString", vstring)

	klog.ErrorS(err, "MinCheckVersion is not a valid version string", "minCheckVersion", MinIPSetCheckVersion)
	klog.ErrorS(err, "Got invalid version string", "versionString", MinIPSetCheckVersion)

Migrated pkg/proxy/ipvs to structured logging #104932

Migrated pkg/proxy/ipvs to structured logging #104932

Conversation

shivanshuraj1333 commented Sep 11, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Sep 11, 2021

pacoxu commented Sep 13, 2021 • edited Loading

shivanshuraj1333 commented Sep 13, 2021

shivanshuraj1333 commented Sep 13, 2021

serathius commented Sep 13, 2021

shivanshuraj1333 commented Sep 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshuraj1333 commented Sep 21, 2021

shivanshuraj1333 commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshuraj1333 commented Sep 22, 2021

shivanshuraj1333 commented Sep 23, 2021

serathius commented Sep 29, 2021

shivanshuraj1333 commented Oct 5, 2021

shivanshuraj1333 commented Oct 14, 2021

aojea commented Oct 14, 2021

k8s-ci-robot commented Oct 14, 2021

pacoxu commented Sep 13, 2021 •

edited

Loading