change azure disk host cache to ReadOnly by default

[andyzhangx]

What type of PR is this?
/kind bug

What this PR does / why we need it:
change azure disk host cache to ReadOnly by default
The default value None would lead to slow perf. And according to below doc: https://docs.microsoft.com/en-us/azure/virtual-machines/windows/premium-storage-performance#disk-caching

default host cache should be ReadOnly, the reason why we use “cachingMode: None” by default in k8s is that there is a critical disk bug due to this cachingMode setting(ReadOnly & ReadWrite) which would lead to disk i/o error, and this issue has been fixed in the middle of this year by azure disk team.

I would also like to change the default value(from ReadWrite to ReadOnly) of caching mode in direct azure disk PV creation, there is one statement in the host cache doc:

Using ReadWrite cache with an application that does not handle persisting the required data can lead to data loss, if the VM crashes.

ReadWrite cache may lead to data loss when VM crashes according to below doc:
https://docs.microsoft.com/en-us/azure/virtual-machines/windows/premium-storage-performance#disk-caching

Which issue(s) this PR fixes:

Fixes #72228

Special notes for your reviewer:

Release note:

the azure disk persistent volume provisioner has changed the default host cache to ReadOnly, if `cachingmode` is not specified in the StorageClass parameters

/sig azure
/hold

hold this RP temporalily, I will let azure disk team do final confirmation whether ReadOnly is the best default value.
From my side, I have verified that the original issue has been fixed in azure, we can move to use ReadOnly host cache now.

[andyzhangx]

cc @khenidak @brendandburns

[liggitt]

clarify the release note that the changed value is in the azure provisioner, for storage classes that do not specify a caching mode. existing PVs and direct PV creation already defaulted cachingmode to readwrite, and will continue to do so:

kubernetes/pkg/apis/core/v1/defaults.go

Lines 274 to 278 in 0c7e207

	func SetDefaults_AzureDiskVolumeSource(obj *v1.AzureDiskVolumeSource) {
	if obj.CachingMode == nil {
	obj.CachingMode = new(v1.AzureDataDiskCachingMode)
	*obj.CachingMode = v1.AzureDataDiskCachingReadWrite
	}

[andyzhangx]

Thanks for your remindar.
@liggitt I would also like to change the default value(from ReadWrite to ReadOnly) of caching mode in direct azure disk PV creation, there is one statement in the host cache doc:

Using ReadWrite cache with an application that does not handle persisting the required data can lead to data loss, if the VM crashes.

ReadWrite cache may lead to data loss when VM crashes according to below doc:
https://docs.microsoft.com/en-us/azure/virtual-machines/windows/premium-storage-performance#disk-caching

Let me submit another commit in this PR.
cc @jsafrane

[andyzhangx]

/hold cancel
/assign @liggitt

[andyzhangx]

/assign @liggitt

[liggitt]

I would also like to change the default value(from ReadWrite to ReadOnly) of caching mode in direct azure disk PV creation, there is one statement in the host cache doc:

Using ReadWrite cache with an application that does not handle persisting the required data can lead to data loss, if the VM crashes.

ReadWrite cache may lead to data loss when VM crashes according to below doc:
https://docs.microsoft.com/en-us/azure/virtual-machines/windows/premium-storage-performance#disk-caching

Reading through that, I agree ReadWrite is not a great default for PVs, but changing an API default is considered a breaking change. We'd need to understand the full impact of changing the default to ReadOnly for an application that was properly handling persisting data. Would it only result in a performance degradation, or would it change behavior in breaking ways as well?

[andyzhangx]

Reading through that, I agree ReadWrite is not a great default for PVs, but changing an API default is considered a breaking change. We'd need to understand the full impact of changing the default to ReadOnly for an application that was properly handling persisting data. Would it only result in a performance degradation, or would it change behavior in breaking ways as well?

Hi @liggitt, I don't think k8s users could properly handling persisting data when using ReadWrite cache, since the write cache is stored in memory in the host OS. ReadWrite cache setting is more suitable for OS level application, e.g. SQL Server, while k8s container only knows about storage, it is not aware of the write cache in memory in host OS. Change ReadWrite to ReadOnly is actually a bug fix for possible data loss.

Below are details about host cache setting, it also applies to Linux:
https://blogs.msdn.microsoft.com/windowsazurestorage/2012/06/27/exploring-windows-azure-drives-disks-and-images/
"
The read cache is stored both on disk and in memory in the host OS. The write cache is stored in memory in the host OS.

WARNING: If your application does not use FILE_FLAG_WRITE_THROUGH, the write cache could result in data loss because the data could be sitting in the host OS memory waiting to be written when the physical machine crashes unexpectedly.
"

[liggitt]

I don't think k8s users could properly handling persisting data when using ReadWrite cache, since the write cache is stored in memory in the host OS.

Write caches are common in many environments, both in software and in hardware. Software responsible for persisting data typically makes use of syscalls like fsync() to ensure data has actually been persisted. I'm not familiar with the FILE_FLAG_WRITE_THROUGH option documented, but it sounds similar to fsync().

A couple questions:

1. Would making use of the OS-appropriate method (e.g. fsync(), FILE_FLAG_WRITE_THROUGH, etc) ensure data was properly persisted? The case for changing the default is weaker if standard methods for ensuring data is persisted are honored by this cache.
2. Would changing the default from Read/Write to ReadOnly modify functional behavior or just performance characteristics? The case for changing the default is weaker if it could cause any functional regression in applications that currently are working properly

[liggitt]

It looks like the field was added in 1.4 with a default of None (dea4b02#diff-14b477f6f8ca5978c702f4d21641e43f), and the default was changed to ReadWrite in 1.7 (4177b28#diff-14b477f6f8ca5978c702f4d21641e43fR242)

[k8s-ci-robot]

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

• pkg/volume/azure_dd/OWNERS [andyzhangx]

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

[andyzhangx]

Hi @liggitt I have reverted the code change of the cachingMode change in azure disk default PV setting, now this PR only changes the cachingMode from None to ReadOnly in azure disk storage class which is quite safe.
Could you take a look? Thanks.

[liggitt]

updated release note. lgtm, will let @kubernetes/sig-storage-pr-reviews tag

[andyzhangx]

/assign @feiskyer
/priority important-soon

[feiskyer]

/lgtm
/retest

[andyzhangx]

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-kops-aws

[k8s-ci-robot]

@andyzhangx: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	53145d7	link	/test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.