Migrated `scheduler` files server.go, node_label.go, csi.go, non_csi.go to structured logging #105855

shiva1333 · 2021-10-23T06:39:29Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrate cmd/kube-scheduler/app/server.go, pkg/scheduler/framework/plugins/nodelabel/node_label.go, pkg/scheduler/framework/plugins/nodevolumelimits/csi.go, pkg/scheduler/framework/plugins/nodevolumelimits/non_csi.go to structured logging

Which issue(s) this PR fixes:

Ref #105841

Special notes for your reviewer:

Added missing migration points, fixed previously migrated logs

Does this PR introduce a user-facing change?

Migrated `cmd/kube-scheduler/app/server.go`, `pkg/scheduler/framework/plugins/nodelabel/node_label.go`, `pkg/scheduler/framework/plugins/nodevolumelimits/csi.go`, `pkg/scheduler/framework/plugins/nodevolumelimits/non_csi.go` to structured logging

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

k8s-ci-robot · 2021-10-23T06:39:36Z

@shivanshu1333: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

shiva1333 · 2021-10-23T06:40:13Z

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

shiva1333 · 2021-10-23T06:41:20Z

/cc @serathius

shiva1333 · 2021-10-23T07:00:05Z

using label to squash
/label tide/merge-method-squash
/sig instrumentation

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

pkg/scheduler/framework/plugins/nodevolumelimits/non_csi.go

serathius · 2021-10-26T08:38:57Z

/lgtm
/assign @alculquicondor

damemi

/approve

alculquicondor · 2021-10-26T13:57:40Z

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

@@ -96,7 +96,7 @@ func (pl *CSILimits) Filter(ctx context.Context, _ *framework.CycleState, pod *v
 	csiNode, err := pl.csiNodeLister.Get(node.Name)
 	if err != nil {
 		// TODO: return the error once CSINode is created by default (2 releases)


@cofyc @msau42 can we do this TODO now?

happy to help here :)

alculquicondor · 2021-10-26T13:58:56Z

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

@@ -188,7 +188,7 @@ func (pl *CSILimits) filterAttachableVolumes(
 			}
 			// If the PVC is invalid, we don't count the volume because
 			// there's no guarantee that it belongs to the running predicate.
-			klog.V(5).InfoS("Unable to look up PVC info", "PVC", fmt.Sprintf("%s/%s", pod.Namespace, pvcName))
+			klog.V(5).InfoS("Unable to look up PVC info", "pod", klog.KObj(pod), "PVC", klog.KRef(pod.Namespace, pvcName))


Can you check which key we are using in other parts of the source code? Is it PVC or pvc or the long name?

PVC is being used as a key in this file and other files as well, also in other migration PRs, these are the most commonly used keys (eg. PV, PVC).

alculquicondor · 2021-10-26T14:01:37Z

pkg/scheduler/framework/plugins/nodevolumelimits/csi.go

 		return "", ""
 	}

 	storageClass, err := pl.scLister.Get(scName)
 	if err != nil {
-		klog.V(5).InfoS("Could not get StorageClass for PVC", "PVC", fmt.Sprintf("%s/%s", namespace, pvcName), "err", err)
+		klog.V(5).InfoS("Could not get StorageClass for PVC", "storageClass", klog.KObj(storageClass), "PVC", klog.KObj(pvc), "err", err)


isn't storageClass nil in this case? you have to build the klog.KRef. Also check if storageClass is the keyword we are using in other places.

Yes, adding storage class here is wrong, thanks for pointing it out, fixed it.
Also, we don't need to add anything else as "PVC", fmt.Sprintf("%s/%s", namespace, pvcName) is being replaced by "PVC", klog.KObj(pvc).

alculquicondor · 2021-10-26T14:04:53Z

pkg/scheduler/framework/plugins/nodevolumelimits/non_csi.go

@@ -178,7 +178,7 @@ func newNonCSILimits(
 		filter = cinderVolumeFilter
 		volumeLimitKey = v1.ResourceName(volumeutil.CinderVolumeLimitKey)
 	default:
-		klog.ErrorS(errors.New("wrong filterName"), "Cannot create nonCSILimits plugin", "candidates", fmt.Sprintf("%v %v %v %v", ebsVolumeFilterType, gcePDVolumeFilterType, azureDiskVolumeFilterType, cinderVolumeFilterType))
+		klog.ErrorS(errors.New("wrong filterName"), "Cannot create nonCSILimits plugin", "ebsVolumeFilterType", ebsVolumeFilterType, "gcePDVolumeFilterType", gcePDVolumeFilterType, "azureDiskVolumeFilterType", azureDiskVolumeFilterType, "cinderVolumeFilterType", cinderVolumeFilterType)


Listing the types like this is useless. It should be

"candidates", []string{....}

But I'm also fine with not stating them at all. After all, this line is probably unreachable.

Okay, in that case just logging the error and messages

alculquicondor · 2021-10-26T14:11:17Z

pkg/scheduler/framework/plugins/nodevolumelimits/non_csi.go

 		} else if parsedMaxVols <= 0 {
-			klog.Error("Maximum PD volumes must be a positive value, using default")
+			klog.ErrorS(nil, "Maximum PD volumes must be a positive value, using default")


let's create an error for "maximum PD volumes is negative", and use the common message "Unable to parse maximum PD volumes value, using default"

damemi · 2021-10-26T14:17:59Z

/approve cancel
sorry @alculquicondor didn't see your comments

alculquicondor · 2021-10-26T18:23:23Z

/approve

please squash

k8s-ci-robot · 2021-10-26T18:26:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, sanposhiho, 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:

~~cmd/kube-scheduler/OWNERS~~ [alculquicondor]
~~pkg/scheduler/OWNERS~~ [alculquicondor]

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

shiva1333 · 2021-10-26T18:43:31Z

using label to squash
/label tide/merge-method-squash

shiva1333 · 2021-10-26T18:44:16Z

@alculquicondor lgtm removed due to new commits, do you mind adding it?

alculquicondor · 2021-10-26T19:13:13Z

/lgtm

shiva1333 added 2 commits October 23, 2021 11:37

migrated server.go

243c639

fixed migration

28ca643

k8s-ci-robot added 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 23, 2021

k8s-ci-robot requested review from adtac and damemi October 23, 2021 06:39

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 23, 2021

k8s-ci-robot requested a review from serathius October 23, 2021 06:41

k8s-ci-robot added sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. labels Oct 23, 2021