chore: little refactor of perf_optimization code #870
Conversation
k8s-ci-robot
added
kind/cleanup
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx 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 |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
size/M
pkg/azuredisk/nodeserver.go
Outdated
| diskSizeGibStr, diskIopsStr, diskBwMbpsStr) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "Failed to optimize device performance for target(%s) error(%s)", source, err) | ||
| if isPerfTuningEnabled(profile) { |
There was a problem hiding this comment.
This is redundant. We call this in disksupportsperfoptimization and it doesn't need to be called on unsupported platforms.
pkg/azuredisk/nodeserver_v2.go
Outdated
| } | ||
| } else { | ||
| klog.V(2).Infof("NodeStageVolume: perf optimization is disabled for %s. perfProfile %s accountType %s", source, profile, accountType) |
There was a problem hiding this comment.
This is a single log line per attach which tells me if all the properties made it to volume attributes. Will allow us to easily confirm if the perf optimization was not done on disk. I think we should leave it.
Absence of logs are not always a good confirmation while investigating issues.
There was a problem hiding this comment.
isPerfTuningEnabled(profile) only checks whether profile != none (none is by default), and this profile setting is already logged in NodeStageVolume request body, no need to log here again.
There was a problem hiding this comment.
if one feature is not enabled in storage class by user, no need to mention again in logs
There was a problem hiding this comment.
There is a good amount of code which goes in and parses the attributes from storage class then moves I to volume and then parses it from attribute. Even if we see something in storage class and StageVolume request may never translate in to perf optimization, this single log line tells me if optimization happened for my target. For what its worth I open my log and look for this straightaway. What you're suggesting is a round about way of determining the same.
There was a problem hiding this comment.
I mean if it's "perfProfile":"None", no need to print log again since it's not enabled; while if it's "perfProfile":"Basic" and we should definitely print logs no matter if optimization happens or not.
I0528 03:29:16.345597 1 utils.go:95] GRPC call: /csi.v1.Node/NodeStageVolume
I0528 03:29:16.345617 1 utils.go:96] GRPC request: {"publish_context":{"LUN":"0"},"staging_target_path":"/var/lib/kubelet/plugins/kubernetes.io/csi/pv/azuredisk-2079-disk.csi.azure.com-preprovsioned-pv-cd8vs/globalmount","volume_capability":{"AccessType":{"Mount":{}},"access_mode":{"mode":1}},"volume_context":{"cachingMode":"None","maxShares":"5","perfProfile":"None","requestedsizegib":"1024","skuName":"Premium_LRS"},"volume_id":"/subscriptions/0e46bd28-a80f-4d3a-8200-d9eb8d80cb2e/resourceGroups/kubetest-dwgypo97/providers/Microsoft.Compute/disks/shared-disk-multiple-pods"}
I0528 03:29:18.071367 1 azure_common_linux.go:175] azureDisk - found /dev/disk/azure/scsi1/lun0 by sdc under /dev/disk/azure/scsi1/
I0528 03:29:18.071402 1 nodeserver.go:109] NodeStageVolume: perf optimization is disabled for /dev/disk/azure/scsi1/lun0. perfProfile None accountType Premium_LRS
There was a problem hiding this comment.
In windows, you're going to see even if perfProfile is basic or anything else in storage class, then also we will not optimize. Point is even if storage class carries some value we run some logic to determine if optimization is enabled, which is bound to evolve over time. This line captures that logic and tell me the answer I would be looking for in the logs, without having to look at other things in code etc.
There was a problem hiding this comment.
anyway, I reverted, let's move forward.
|
|
||
| err = nil | ||
|
|
||
| klog.V(12).Infof("Calculating perf optimizations for rofile %s accountType %s diskSize", perfProfile, accountType, diskSizeGibStr) |
There was a problem hiding this comment.
no need to print two logs since last log already has enough info:
klog.V(2).Infof("Returning perf attributes for perfProfile %s accountType %s queueDepth %s nrRequests %s scheduler %s maxSectorsKb %s readAheadKb %s"
I am trying to reduce logs.
There was a problem hiding this comment.
I was pointing out the V(12), I thought it was a typo and the highest was V(5)?
There was a problem hiding this comment.
V(12) is for debugging, no need to print every log in our driver by default.
There was a problem hiding this comment.
Understood that part. Should we set this to v(5)? or v(12) is what we use?
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md
There was a problem hiding this comment.
we still print v(5) logs by default now, set as v(6 or bigger) would not print now.
fix comments
andyzhangx
force-pushed
the
perf_optimization_refactor
branch
from
e9af0b6 to
1010ade
Compare
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
chore: little refactor of perf_optimization code
Which issue(s) this PR fixes:
Fixes #
Requirements:
Special notes for your reviewer:
Release note: