Statefulsets for cinder: allow multi-AZ deployments, spread pods across zones

[zetaab]

What this PR does / why we need it: Currently if we do not specify availability zone in cinder storageclass, the cinder is provisioned to zone called nova. However, like mentioned in issue, we have situation that we want spread statefulset across 3 different zones. Currently this is not possible with statefulsets and cinder storageclass. In this new solution, if we leave it empty the algorithm will choose the zone for the cinder drive similar style like in aws and gce storageclass solutions.

Which issue this PR fixes fixes #44735

Special notes for your reviewer:

example:

kind: StorageClass
apiVersion: storage.k8s.io/v1beta1
metadata:
  name: all
provisioner: kubernetes.io/cinder
---
apiVersion: v1
kind: Service
metadata:
  annotations:
    service.alpha.kubernetes.io/tolerate-unready-endpoints: "true"
  name: galera
  labels:
    app: mysql
spec:
  ports:
  - port: 3306
    name: mysql
  clusterIP: None
  selector:
    app: mysql
---
apiVersion: apps/v1beta1
kind: StatefulSet
metadata:
  name: mysql
spec:
  serviceName: "galera"
  replicas: 3
  template:
    metadata:
      labels:
        app: mysql
      annotations:
        pod.alpha.kubernetes.io/initialized: "true"
    spec:
      containers:
      - name: mysql
        image: adfinissygroup/k8s-mariadb-galera-centos:v002
        imagePullPolicy: Always
        ports:
        - containerPort: 3306
          name: mysql
        - containerPort: 4444
          name: sst
        - containerPort: 4567
          name: replication
        - containerPort: 4568
          name: ist
        volumeMounts:
        - name: storage
          mountPath: /data
        readinessProbe:
          exec:
            command:
            - /usr/share/container-scripts/mysql/readiness-probe.sh
          initialDelaySeconds: 15
          timeoutSeconds: 5
        env:
          - name: POD_NAMESPACE
            valueFrom:
              fieldRef:
                apiVersion: v1
                fieldPath: metadata.namespace
  volumeClaimTemplates:
  - metadata:
      name: storage
      annotations:
        volume.beta.kubernetes.io/storage-class: all
    spec:
      accessModes: [ "ReadWriteOnce" ]
      resources:
        requests:
          storage: 12Gi

If this example is deployed it will automatically create one replica per AZ. This helps us a lot making HA databases.

Current storageclass for cinder is not perfect in case of statefulsets. Lets assume that cinder storageclass is defined to be in zone called nova, but because labels are not added to pv - pods can be started in any zone. The problem is that at least in our openstack it is not possible to use cinder drive located in zone x from zone y. However, should we have possibility to choose between cross-zone cinder mounts or not? Imo it is not good way of doing things that they mount volume from another zone where the pod is located(means more network traffic between zones)? What you think? Current new solution does not allow that anymore (should we have possibility to allow it? it means removing the labels from pv).

There might be some things that needs to be fixed still in this release and I need help for that. Some parts of the code is not perfect.

Issues what i am thinking about (I need some help for these):

1. Can everybody see in openstack what AZ their servers are? Can there be like access policy that do not show that? If AZ is not found from server specs, I have no idea how the code behaves.
2. In GetAllZones() function, is it really needed to make new serviceclient using openstack.NewComputeV2 or could I somehow use existing one
3. This fetches all servers from some openstack tenant(project). However, in some cases kubernetes is maybe deployed only to specific zone. If kube servers are located for instance in zone 1, and then there are another servers in same tenant in zone 2. There might be usecase that cinder drive is provisioned to zone-2 but it cannot start pod, because kubernetes does not have any nodes in zone-2. Could we have better way to fetch kubernetes nodes zones? Currently that information is not added to kubernetes node labels automatically in openstack (which should I think). I have added those labels manually to nodes. If that zone information is not added to nodes, the new solution does not start stateful pods at all, because it cannot target pods.

cc @rootfs @anguslees @jsafrane

Default behaviour in cinder storageclass is changed. If availability is not specified, the zone is chosen by algorithm. It makes possible to spread stateful pods across many zones.

[k8s-reviewable]

This change is 

[k8s-ci-robot]

Hi @zetaab. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

[zetaab]

actually I was wrong, the cloudprovider is adding the labels nowadays for zones:

https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_node_status.go#L284
https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/openstack/openstack.go#L488

so in GetAllZones() function, I should maybe filter through nodes and check those labels, instead of calling openstack API? If so, which library i should use and how?

[zetaab]

@liggitt could you take a look, it looks like rootfs has been away for a while.

[liggitt]

@liggitt could you take a look, it looks like rootfs has been away for a while.

I'm not the right person to review this... I'd suggest some intersection of @kubernetes/sig-storage-pr-reviews and @kubernetes/sig-openstack-pr-reviews

[0xmichalis]

I'm not the right person to review this... I'd suggest some intersection of @kubernetes/sig-storage-pr-reviews and @kubernetes/sig-openstack-pr-reviews

And @kubernetes/sig-apps-api-reviews since this affects statefulsets

[rootfs]

Leaving default zone to nova is a problem. I find AWS/GCE uses a heuristics that hashes PVC name to a zone. Cinder could use the same idea for this issue.

[rootfs]

also see #41598

[zetaab]

Hi @rootfs this PR actually uses that same heuristics thing what is used in AWS/GCE. So if you do not specify AZ for cinder storageclass it uses that same algorithm to choose correct zone. By using that algorithm and labeling PVs we can achieve situation to spread pods to multiple zones

[rootfs]

@zetaab I see it now, cinder_util.go is modified twice in 1st and 3rd commits. Can you merge these two commits?

Please also use camel case for consistency.

@kubernetes/sig-openstack-pr-reviews please review gophercloud update.

[smarterclayton]

@justinsb for drive by on az spreading of volumes

[kow3ns]

@rootfs @justinsb It uses the same function, ChooseZoneForVolume, used by the GCE and AWS CloudProviders, to implement the round robin balancing. It will have, for better or worse, the same benefits and limitations.
@zetaab If your Volume are zonal resources be aware of the scheduling implications this has wrt spreading, Affinity, and AntiAffinity.

[zetaab]

@rootfs 1st and 3rd commit now merged. I did not fully get where I am missing camelcase?

[zetaab]

@kow3ns yes, the volumes are zonal resources and that is why I added labels to PVs. After PV has zone label, the pod is automatically provisioned in same zone(I tested it with shutdowning all nodes from zone and after that it could not provision pod because there were no available nodes with those labels).

The bad thing which exist in current solution is that, dynamic pvs does not have labels in cinder. In our case it means that pv can be provisioned for example zone x, and then pod itself is started in zone y. However, because our cinder is zonal resource it does not work at all. Although this can be allowed in cinder, so in some usecases people can use cinder resources from another zone. In my opinion that is bad way of doing things, it creates unnecessary network traffic between zones.

If people really want provision pv to zone x and run pod in zone y, they need remove PV labels. But as I see this is really bad way of doing things? What you think? Should we have way to define is that code adding pv labels or not?

[rootfs]

nit: volumeId, volumeAZ

[rootfs]

@k8s-bot ok to test

[rootfs]

Cinder fix lgtm.

Anybody from @kubernetes/sig-openstack-pr-reviews want to check gophercloud change?

@xsgordon @anguslees

[zetaab]

@pospispa I can update it, it should be something like:

availability: Availability Zone. If not specified, the zone is chosen by algorithm from those where Kubernetes cluster has a node.

zetaab/kubernetes.github.io@b5b384d

I will do PR after this is merged

[dims]

/lgtm

[dims]

/assign @jsafrane @mikedanese

[mikedanese]

/approve

[zetaab]

@jsafrane could you please approve

[jsafrane]

/approve

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, jsafrane, mikedanese, pospispa, zetaab

Needs approval from an approver in each of these OWNERS Files:

• pkg/cloudprovider/OWNERS [mikedanese]
• pkg/volume/cinder/OWNERS [jsafrane]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[k8s-github-robot]

Automatic merge from submit-queue

[pospispa]

@zetaab please, will you create the corresponding documentation PR for this PR?

[zetaab]

@pospispa its done already kubernetes/website#3693