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

chore: little refactor of perf_optimization code #870

Merged
merged 2 commits into from
May 29, 2021
Merged

chore: little refactor of perf_optimization code #870

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f4d61c
chore: little refactor of perf_optimization code
andyzhangx May 28, 2021
1010ade
test: increase shared disk size
andyzhangx May 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ all: azuredisk
verify: unit-test
hack/verify-all.sh
go vet ./pkg/...
go build ./pkg/tool/

.PHONY: unit-test
unit-test: unit-test-v1 unit-test-v2
Expand Down
12 changes: 6 additions & 6 deletions pkg/azuredisk/device_perf_optimization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ package azuredisk

// This is the interface for DeviceHelper
type Interface interface {
DiskSupportsPerfOptimization(diskPerfProfile string, diskAccountType string) bool
OptimizeDiskPerformance(nodeInfo *NodeInfo, diskSkus map[string]map[string]DiskSkuInfo, devicePath string,
perfProfile string, accountType string, diskSizeGibStr string, diskIopsStr string, diskBwMbpsStr string) error
DiskSupportsPerfOptimization(diskPerfProfile, diskAccountType string) bool
OptimizeDiskPerformance(nodeInfo *NodeInfo, diskSkus map[string]map[string]DiskSkuInfo, devicePath,
perfProfile, accountType, diskSizeGibStr, diskIopsStr, diskBwMbpsStr string) error
}

// Compile-time check to ensure all Mounter DeviceHelper satisfy
Expand All @@ -38,11 +38,11 @@ type SafeDeviceHelper struct {
Interface
}

func (dh *SafeDeviceHelper) DeviceSupportsPerfOptimization(diskPerfProfile string, diskAccountType string) bool {
func (dh *SafeDeviceHelper) DeviceSupportsPerfOptimization(diskPerfProfile, diskAccountType string) bool {
return dh.Interface.DiskSupportsPerfOptimization(diskPerfProfile, diskAccountType)
}

func (dh *SafeDeviceHelper) OptimizeDiskPerformance(nodeInfo *NodeInfo, diskSkus map[string]map[string]DiskSkuInfo, devicePath string,
perfProfile string, accountType string, diskSizeGibStr string, diskIopsStr string, diskBwMbpsStr string) error {
func (dh *SafeDeviceHelper) OptimizeDiskPerformance(nodeInfo *NodeInfo, diskSkus map[string]map[string]DiskSkuInfo, devicePath,
perfProfile, accountType, diskSizeGibStr, diskIopsStr, diskBwMbpsStr string) error {
return dh.Interface.OptimizeDiskPerformance(nodeInfo, diskSkus, devicePath, perfProfile, accountType, diskSizeGibStr, diskIopsStr, diskBwMbpsStr)
}
1 change: 0 additions & 1 deletion pkg/azuredisk/device_perf_optimization_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,5 @@ func accountSupportsPerfOptimization(accountType string) bool {
if strings.HasPrefix(accountTypeLower, "premium") || strings.HasPrefix(accountTypeLower, "standardssd") {
return true
}

return false
}
7 changes: 1 addition & 6 deletions pkg/azuredisk/device_perf_optimization_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,7 @@ func echoToFile(content string, filePath string) (err error) {
}

func getOptimalDeviceSettings(nodeInfo *NodeInfo, diskSkus map[string]map[string]DiskSkuInfo, perfProfile string, accountType, diskSizeGibStr, diskIopsStr, diskBwMbpsStr string) (queueDepth, nrRequests, scheduler, maxSectorsKb, readAheadKb string, err error) {
klog.V(2).Infof("Calculating perf optimizations for rofile %s accountType %s diskSize", perfProfile, accountType, diskSizeGibStr)

err = nil

klog.V(12).Infof("Calculating perf optimizations for rofile %s accountType %s diskSize", perfProfile, accountType, diskSizeGibStr)
Copy link
Contributor

Choose a reason for hiding this comment

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

V(2)?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was pointing out the V(12), I thought it was a typo and the highest was V(5)?

Copy link
Member Author

Choose a reason for hiding this comment

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

V(12) is for debugging, no need to print every log in our driver by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

we still print v(5) logs by default now, set as v(6 or bigger) would not print now.

iopsHeadRoom := .25
maxHwSectorsKb := 512.0

Expand Down Expand Up @@ -222,8 +219,6 @@ func getOptimalDeviceSettings(nodeInfo *NodeInfo, diskSkus map[string]map[string
// getMatchingDiskSku gets the smallest SKU which matches the size, io and bw requirement
// TODO: Query the disk size (e.g. P10, P30 etc) and use that to find the sku
func getMatchingDiskSku(diskSkus map[string]map[string]DiskSkuInfo, accountType, diskSizeGibStr, diskIopsStr, diskBwMbpsStr string) (matchingSku *DiskSkuInfo, err error) {
err = nil
matchingSku = nil
accountTypeLower := strings.ToLower(accountType)
skus, ok := diskSkus[accountTypeLower]

Expand Down
9 changes: 5 additions & 4 deletions pkg/azuredisk/device_perf_optimization_unsupported.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,16 @@ limitations under the License.

package azuredisk

import "fmt"

type DeviceHelper struct{}

func (deviceHelper *DeviceHelper) DiskSupportsPerfOptimization(diskPerfProfile string, diskAccountType string) bool {
func (deviceHelper *DeviceHelper) DiskSupportsPerfOptimization(diskPerfProfile, diskAccountType string) bool {
// return false on unsupported platforms
return false
}

func (deviceHelper *DeviceHelper) OptimizeDiskPerformance(nodeInfo *NodeInfo, diskSkus map[string]map[string]DiskSkuInfo, devicePath string,
perfProfile string, accountType string, diskSizeGibStr string, diskIopsStr string, diskBwMbpsStr string) (err error) {
// Don't fail if we are in an unsupported platform
return nil
perfProfile, accountType, diskSizeGibStr, diskIopsStr, diskBwMbpsStr string) (err error) {
return fmt.Errorf("OptimizeDiskPerformance not implemented")
}
16 changes: 9 additions & 7 deletions pkg/azuredisk/nodeserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,16 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe
return nil, status.Errorf(codes.Internal, "Failed to get perf attributes for %s. Error: %v", source, err)
}

if d.getDeviceHelper().DiskSupportsPerfOptimization(profile, accountType) {
err = d.getDeviceHelper().OptimizeDiskPerformance(d.getNodeInfo(), d.getDiskSkuInfoMap(), source, profile, accountType,
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) {
Copy link
Contributor

@abhisheksinghbaghel abhisheksinghbaghel May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is redundant. We call this in disksupportsperfoptimization and it doesn't need to be called on unsupported platforms.

// save one logging since perfTuning is disabled by default
if d.getDeviceHelper().DiskSupportsPerfOptimization(profile, accountType) {
if err := d.getDeviceHelper().OptimizeDiskPerformance(d.getNodeInfo(), d.getDiskSkuInfoMap(), source, profile, accountType,
diskSizeGibStr, diskIopsStr, diskBwMbpsStr); err != nil {
return nil, status.Errorf(codes.Internal, "Failed to optimize device performance for target(%s) error(%s)", source, err)
}
} else {
klog.V(2).Infof("NodeStageVolume: perf optimization is disabled for %s. perfProfile %s accountType %s", source, profile, accountType)
}
} else {
klog.V(2).Infof("NodeStageVolume: perf optimization is disabled for %s. perfProfile %s accountType %s", source, profile, accountType)
}
}

Expand Down
16 changes: 9 additions & 7 deletions pkg/azuredisk/nodeserver_v2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,16 @@ func (d *DriverV2) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolume
return nil, status.Errorf(codes.Internal, "Failed to get perf attributes for %s. Error: %v", source, err)
}

if d.getDeviceHelper().DiskSupportsPerfOptimization(profile, accountType) {
err = d.getDeviceHelper().OptimizeDiskPerformance(d.getNodeInfo(), d.getDiskSkuInfoMap(), source, profile, accountType,
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) {
// save one logging since perfTuning is disabled by default
if d.getDeviceHelper().DiskSupportsPerfOptimization(profile, accountType) {
if err := d.getDeviceHelper().OptimizeDiskPerformance(d.getNodeInfo(), d.getDiskSkuInfoMap(), source, profile, accountType,
diskSizeGibStr, diskIopsStr, diskBwMbpsStr); err != nil {
return nil, status.Errorf(codes.Internal, "Failed to optimize device performance for target(%s) error(%s)", source, err)
}
} else {
klog.V(2).Infof("NodeStageVolume: perf optimization is disabled for %s. perfProfile %s accountType %s", source, profile, accountType)
}
} else {
klog.V(2).Infof("NodeStageVolume: perf optimization is disabled for %s. perfProfile %s accountType %s", source, profile, accountType)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

if one feature is not enabled in storage class by user, no need to mention again in logs

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

anyway, I reverted, let's move forward.

}
}

Expand Down
Binary file removed pkg/tool/tool
View file
Open in desktop
Binary file not shown.