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 all commits
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
}
13 changes: 4 additions & 9 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 @@ -36,7 +36,7 @@ const (
blockDeviceRootPath = "/sys/block"
)

func (deviceHelper *DeviceHelper) DiskSupportsPerfOptimization(diskPerfProfile string, diskAccountType string) bool {
func (deviceHelper *DeviceHelper) DiskSupportsPerfOptimization(diskPerfProfile, diskAccountType string) bool {
return isPerfTuningEnabled(diskPerfProfile) && accountSupportsPerfOptimization(diskAccountType)
}

Expand Down Expand Up @@ -133,7 +133,7 @@ func getDeviceName(lunPath string) (deviceName string, err error) {
}

// echoToFile echos setting value to the file
func echoToFile(content string, filePath string) (err error) {
func echoToFile(content, filePath string) (err error) {
outfile, err := os.Create(filePath)
if err != nil {
return err
Expand All @@ -148,11 +148,8 @@ func echoToFile(content string, filePath string) (err error) {
return cmd.Wait()
}

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

func getOptimalDeviceSettings(nodeInfo *NodeInfo, diskSkus map[string]map[string]DiskSkuInfo, perfProfile, accountType, diskSizeGibStr, diskIopsStr, diskBwMbpsStr string) (queueDepth, nrRequests, scheduler, maxSectorsKb, readAheadKb string, err error) {
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")
}
5 changes: 2 additions & 3 deletions pkg/azuredisk/nodeserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,8 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe
}

if d.getDeviceHelper().DiskSupportsPerfOptimization(profile, accountType) {
err = d.getDeviceHelper().OptimizeDiskPerformance(d.getNodeInfo(), d.getDiskSkuInfoMap(), source, profile, accountType,
diskSizeGibStr, diskIopsStr, diskBwMbpsStr)
if err != nil {
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 {
Expand Down
5 changes: 2 additions & 3 deletions pkg/azuredisk/nodeserver_v2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,8 @@ func (d *DriverV2) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolume
}

if d.getDeviceHelper().DiskSupportsPerfOptimization(profile, accountType) {
err = d.getDeviceHelper().OptimizeDiskPerformance(d.getNodeInfo(), d.getDiskSkuInfoMap(), source, profile, accountType,
diskSizeGibStr, diskIopsStr, diskBwMbpsStr)
if err != nil {
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 {
Expand Down
Binary file removed pkg/tool/tool
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion test/e2e/pre_provisioning_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ var _ = ginkgo.Describe("Pre-Provisioned", func() {
ginkgo.It("should succeed when creating a shared disk [disk.csi.azure.com][windows]", func() {
skipIfUsingInTreeVolumePlugin()
skipIfOnAzureStackCloud()
req := makeCreateVolumeReq("single-shared-disk", 256)
req := makeCreateVolumeReq("single-shared-disk", 512)
req.Parameters = map[string]string{
"skuName": "Premium_LRS",
"maxShares": "2",
Expand Down