Aks-engine: After update to aks-engine 0.29.1, PVCs are created as managed disks, even if the cluster is using StorageAccount

[DonMartin76]

Is this a request for help?: Yup.

---

Is this an ISSUE or FEATURE REQUEST? (choose one): Issue.

---

What version of aks-engine?: 0.29.1

---

Kubernetes version: 1.13.2

What happened: After the update from acs-engine (0.22.1 or something) to aks-engine 0.29.1, the default storage class for our cluster, which is StorageAccount based, seems to be managed disks. I.e., creating a pvc with the storageClassName set to default creates a managed disk. As the VMs are storage account based, these managed disks cannot be mounted to the agents.

What's worse though is that setting the storageClassName to unmanaged-standard also creates a managed disk.

What you expected to happen: (A) For a cluster which uses StorageAccount as storage model, the default storage class for dynamically provisioned disks should be unmanaged disks, and (B) if I specify that I need an unmanaged disk via the storageClassName: unmanaged-standard property, it really should create an unmanaged disk.

How to reproduce it (as minimally and precisely as possible): (A) Create a 1.13.2 cluster with storage type set to StorageAccount. (B) Create a PVC with either the default or unmanaged-standard storage class name. (C) Watch the cluster create a managed disk instead.

Anything else we need to know: I bet I missed this somewhere in the docs... The reason why we still have stuck to the storage account mode is that those disks are, at least by the specs, a lot faster, plus they are a lot cheaper.

[welcome[bot]]

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it.

[DonMartin76]

This is the aks-engine.json we're using (redacted to take out secrets):

{
  "apiVersion": "vlabs",
  "properties": {
    "orchestratorProfile": {
      "orchestratorType": "Kubernetes",
      "orchestratorRelease": "1.13",
      "kubernetesConfig": {
        "addons": [
          {
            "name": "tiller",
            "containers": [
              {
                "name": "tiller",
                "image": "gcrio.azureedge.net/kubernetes-helm/tiller:v2.9.1",
                "cpuRequests": "1",
                "memoryRequests": "1024Mi",
                "cpuLimits": "1",
                "memoryLimits": "1024Mi"
              }
            ]
          }
        ]
      }
    },
    "masterProfile": {
      "count": 1,
      "dnsPrefix": "dev9-whatever",
      "vmSize": "Standard_A2"
    },
    "agentPoolProfiles": [
      {
        "name": "workerdev9",
        "count": 2,
        "vmSize": "Standard_A2",
        "storageProfile" : "StorageAccount",
        "availabilityProfile": "AvailabilitySet",
        "customNodeLabels": {
          "stack": "dev9",
          "instancetype": "worker"
        }
      },
      {
        "name": "helperdev9",
        "count": 1,
        "vmSize": "Standard_A2",
        "storageProfile" : "StorageAccount",
        "availabilityProfile": "AvailabilitySet",
        "customNodeLabels": {
          "stack": "dev9",
          "instancetype": "helper"
        }
      }
    ],
    "linuxProfile": {
      "adminUsername": "azureuser",
      "ssh": {
        "publicKeys": [
          {
            "keyData": "ssh-rsa <...>"
          }
        ]
      }
    },
    "servicePrincipalProfile": {
      "clientId": "<...>",
      "secret": "<..>"
    }
  }
}

[andyzhangx]

@jackfrancis we are still going to support unmanaged disk?

[DonMartin76]

I think it would be fine by now if you said you didn't, AKS doesn't support unmanaged disks either, but you don't get any errors at all, everything works up until the point where you try to create unmanaged persistent volumes.

[jackfrancis]

@DonMartin76 makes a good point

@andyzhangx is there something that changed upstream which broke PVCs for standard storage?

[andyzhangx]

Thanks, I have identified this is a config issue due to default azure disk storage class has been changed from unmanaged to managed in k8s upstream by https://github.com/kubernetes/kubernetes/pull/67483, already sent a PR to fix this config issue.

[andyzhangx]

is there something that changed upstream which broke PVCs for standard storage?

@jackfrancis no, AFAIK, usually I would make the upstream change compatible with old version and this issue is config issue.

[jackfrancis]

@andy can you comment on which k8s versions will get this fix? Thanks for fixing!

[andyzhangx]

@jackfrancis who do you mean by something that changed upstream which broke PVCs for standard storage ?
This issue is caused by kubernetes/kubernetes#67483 which changed default azure disk storage class from unmanaged to managed in k8s upstream, and has been fixed in aks-engine now.
There is nothing to do on k8s upstream.

[jackfrancis]

@andyzhangx I see, I think you're saying that the original statement in this issue "PVCs are created as managed disks, even if the cluster is using StorageAccount" is by design.

In other words, it is intentional that there is no way to create an unmanaged-backed PVC?

If so, we should add that to our documentation and close this issue. How does that sound @DonMartin76 ?

[andyzhangx]

I see, I think you're saying that the original statement in this issue "PVCs are created as managed disks, even if the cluster is using StorageAccount" is by design.
In other words, it is intentional that there is no way to create an unmanaged-backed PVC?

It's not correct, this issue is already fixed by https://github.com/Azure/aks-engine/pull/473, which specify kind as shared value, so user could still create an unmanaged-backed PVC.

I think we could close this issue now since it's already fixed in aks-engine by https://github.com/Azure/aks-engine/pull/473

[DonMartin76]

Thanks, @andyzhangx, for fixing. I wasn't actually expecting a fix of this (but rather an error message stating that storage account clusters aren't supported anymore), as the official(?) way to go nowadays is to use managed disks. Nevertheless, I appreciate it! Closing this, as it's going to be fixed in the next version (I see it has already been merged to master).

[jackfrancis]

Sorry for the confusion @andyzhangx, thanks for clarifying!