use contextual logging（nodeipam and nodelifecycle part） #112670

yangjunmyfm192085 · 2022-09-22T06:48:28Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

same as #111708
use klog.InfoS instead of klog.V(0).InfoS
As mentioned in the issue kubernetes-sigs/logtools#2, Calling V with zero as parameter just causes additional overhead, so we need to avoid this scenario
And the corresponding files are also log structured

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Migrate `pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go, pkg/controller/nodeipam/ipam/multi_cidr_range_allocator.go pkg/controller/nodeipam/ipam/range_allocator.go pkg/controller/nodelifecycle/node_lifecycle_controller.go` to structured logging

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

yangjunmyfm192085

/cc @bowei
/sig instrumentation
/wg structured-logging

yangjunmyfm192085

/retest

dashpole · 2022-09-22T16:29:32Z

/assign @pohly

dashpole · 2022-09-22T16:29:36Z

/triage accepted

pohly · 2022-09-22T18:19:24Z

pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go

-		klog.Fatalf("kubeClient is nil when starting NodeController")
+		klog.ErrorS(nil, "kubeClient is nil when starting NodeController")
+		// ACTION REQUIRED: Exit code changed from 255 to 1
+		os.Exit(1)


Instead of os.Exit, please use klog.FlushAndExit. os.Exit itself does not flush buffered log messages.

ok, I will update

pohly · 2022-09-23T05:56:36Z

pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go

-		klog.Fatalf("kubeClient is nil when starting NodeController")
+		klog.ErrorS(nil, "kubeClient is nil when starting NodeController")
+		// ACTION REQUIRED: Exit code changed from 255 to 1
+		os.Exit(1)


Please use klog.FlushAndExit(1).

I'm also not sure about the // ACTION REQUIRED comment. I think mentioning that in the changenote for the PR is enough. Applies also below.

let me update

pohly · 2022-09-23T05:58:21Z

pkg/controller/nodeipam/ipam/multi_cidr_range_allocator.go

-		klog.Fatalf("client is nil")
+		klog.ErrorS(nil, "client is nil")
+		// ACTION REQUIRED: Exit code changed from 255 to 1
+		os.Exit(1)


klog.FlushAndExit(1)

pohly · 2022-09-23T06:01:51Z

pkg/controller/nodeipam/ipam/range_allocator.go

-		klog.Fatalf("kubeClient is nil when starting NodeController")
+		klog.ErrorS(nil, "kubeClient is nil when starting NodeController")
+		// ACTION REQUIRED: Exit code changed from 255 to 1
+		os.Exit(1)


klog.FlushAndExit(1)

pohly · 2022-09-23T06:02:59Z

pkg/controller/nodelifecycle/node_lifecycle_controller.go

-		klog.Fatalf("kubeClient is nil when starting Controller")
+		klog.ErrorS(nil, "kubeClient is nil when starting Controller")
+		// ACTION REQUIRED: Exit code changed from 255 to 1
+		os.Exit(1)


klog.FlushAndExit(1)

Or return an error? This function can do that. I haven't checked the others.

yeah, maybe return a error is better here.

yangjunmyfm192085

/cc @pohly I have updated all suggestions, Could you please take a look again?

pohly · 2022-09-29T13:30:07Z

pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go

@@ -86,13 +86,13 @@ var _ CIDRAllocator = (*cloudCIDRAllocator)(nil)
 // NewCloudCIDRAllocator creates a new cloud CIDR allocator.
 func NewCloudCIDRAllocator(client clientset.Interface, cloud cloudprovider.Interface, nodeInformer informers.NodeInformer) (CIDRAllocator, error) {
 	if client == nil {
-		klog.Fatalf("kubeClient is nil when starting NodeController")
+		return nil, fmt.Errorf("kubeClient is nil when starting cloud CIDR allocator")


nit: When there's no need for formatting, use errors.New. Also applies below.

Looks like errors now gets imported too often:

/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go:34:2: errors redeclared in this block

Yeah, I have updated it.

pohly

/lgtm

pohly · 2022-09-29T15:57:30Z

/lgtm
/assign @bowei

For approval.

pohly · 2023-02-15T11:03:31Z

/lgtm
/assign @soltysh

Thanks for keeping the PR up-to-date. We should be able to get it merged soon.

k8s-ci-robot · 2023-02-15T11:03:37Z

LGTM label has been added.

Git tree hash: c12415be1c8fb0a6bd85649d3850b04972f7a808

soltysh

/approve

soltysh · 2023-03-07T07:35:10Z

pkg/controller/nodeipam/ipam/multi_cidr_range_allocator.go

@@ -181,28 +184,34 @@ func NewMultiCIDRRangeAllocator(
 				ra.cidrQueue.Add(key)
 			}
 		},
+		//AddFunc: createClusterCIDRHandler(func(ccc *networkingv1alpha1.ClusterCIDR) error {


Nit: this should be removed

soltysh · 2023-03-07T07:47:48Z

/priority important-longterm

soltysh · 2023-03-07T08:04:45Z

/lgtm

k8s-ci-robot · 2023-03-07T08:04:52Z

LGTM label has been added.

Git tree hash: 6f75bb255f7e541f8c0542b3ef391f3891e8267d

pohly · 2023-03-07T11:41:06Z

/assign @dims

For approval in cmd/cloud-controller-manager.

dims · 2023-03-07T12:59:04Z

changes under cmd/cloud-controller-manager/ LGTM

/approve
/lgtm

k8s-ci-robot · 2023-03-07T12:59:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, dims, pohly, soltysh, yangjunmyfm192085

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:

~~cmd/cloud-controller-manager/OWNERS~~ [dims]
~~cmd/kube-controller-manager/OWNERS~~ [dims,soltysh]
~~pkg/controller/nodeipam/OWNERS~~ [aojea,dims,soltysh]
~~pkg/controller/nodelifecycle/OWNERS~~ [dims,soltysh]
~~test/OWNERS~~ [aojea,dims,pohly,soltysh]

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

k8s-triage-robot · 2023-03-07T16:37:00Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot requested review from bowei and ingvagabund September 22, 2022 06:48

yangjunmyfm192085 commented Sep 22, 2022

View reviewed changes

k8s-ci-robot added sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging. labels Sep 22, 2022

yangjunmyfm192085 commented Sep 22, 2022

View reviewed changes

k8s-ci-robot assigned pohly Sep 22, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 22, 2022

pohly suggested changes Sep 23, 2022

View reviewed changes

yangjunmyfm192085 force-pushed the delklogV0 branch 2 times, most recently from 6008ac1 to 27eec16 Compare September 23, 2022 14:02

yangjunmyfm192085 commented Sep 23, 2022

View reviewed changes

pohly reviewed Sep 29, 2022

View reviewed changes

yangjunmyfm192085 force-pushed the delklogV0 branch from 27eec16 to 5ad03a8 Compare September 29, 2022 14:36

pohly approved these changes Sep 29, 2022

View reviewed changes

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

yangjunmyfm192085 force-pushed the delklogV0 branch from 5ad03a8 to 1d1a239 Compare September 29, 2022 15:21

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

yangjunmyfm192085 force-pushed the delklogV0 branch 2 times, most recently from bbd1b87 to e4663b6 Compare February 6, 2023 05:35

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Feb 6, 2023

yangjunmyfm192085 force-pushed the delklogV0 branch from e4663b6 to 0883222 Compare February 6, 2023 05:38

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2023

yangjunmyfm192085 force-pushed the delklogV0 branch from 0883222 to c322758 Compare February 15, 2023 01:52

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2023

k8s-ci-robot assigned soltysh Feb 15, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2023

soltysh approved these changes Mar 7, 2023

View reviewed changes

k8s-ci-robot added 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 Mar 7, 2023

use klog.InfoS instead of klog.V(0),Info

780ef3a

yangjunmyfm192085 force-pushed the delklogV0 branch from c322758 to 780ef3a Compare March 7, 2023 07:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2023

k8s-ci-robot requested a review from soltysh March 7, 2023 07:51

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2023

k8s-ci-robot assigned dims Mar 7, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2023

k8s-ci-robot merged commit 37326f7 into kubernetes:master Mar 7, 2023

k8s-ci-robot added this to the v1.27 milestone Mar 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use contextual logging（nodeipam and nodelifecycle part） #112670

use contextual logging（nodeipam and nodelifecycle part） #112670

yangjunmyfm192085 commented Sep 22, 2022 •

edited

yangjunmyfm192085 left a comment

yangjunmyfm192085 left a comment

dashpole commented Sep 22, 2022

dashpole commented Sep 22, 2022

pohly Sep 22, 2022

yangjunmyfm192085 Sep 23, 2022

pohly Sep 23, 2022

pohly Sep 23, 2022

yangjunmyfm192085 Sep 23, 2022

pohly Sep 23, 2022

pohly Sep 23, 2022

pohly Sep 23, 2022

pohly Sep 23, 2022

yangjunmyfm192085 Sep 23, 2022

yangjunmyfm192085 left a comment

pohly Sep 29, 2022

pohly Sep 29, 2022

yangjunmyfm192085 Sep 29, 2022

pohly left a comment

pohly commented Sep 29, 2022

pohly commented Feb 15, 2023

k8s-ci-robot commented Feb 15, 2023

soltysh left a comment

soltysh Mar 7, 2023

yangjunmyfm192085 Mar 7, 2023

soltysh commented Mar 7, 2023

soltysh commented Mar 7, 2023

k8s-ci-robot commented Mar 7, 2023

pohly commented Mar 7, 2023

dims commented Mar 7, 2023

k8s-ci-robot commented Mar 7, 2023

k8s-triage-robot commented Mar 7, 2023

use contextual logging（nodeipam and nodelifecycle part） #112670

use contextual logging（nodeipam and nodelifecycle part） #112670

Conversation

yangjunmyfm192085 commented Sep 22, 2022 • edited

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.:

yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

dashpole commented Sep 22, 2022

dashpole commented Sep 22, 2022

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

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

yangjunmyfm192085 left a 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

pohly left a comment

Choose a reason for hiding this comment

pohly commented Sep 29, 2022

pohly commented Feb 15, 2023

k8s-ci-robot commented Feb 15, 2023

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Mar 7, 2023

soltysh commented Mar 7, 2023

k8s-ci-robot commented Mar 7, 2023

pohly commented Mar 7, 2023

dims commented Mar 7, 2023

k8s-ci-robot commented Mar 7, 2023

k8s-triage-robot commented Mar 7, 2023

yangjunmyfm192085 commented Sep 22, 2022 •

edited