Distributed provisioning

[pohly]

This changes the architecture of PMEM-CSI such that the controller part only implements the webhooks and provisioning happens on the nodes.

This PR is based on some pending changes (PR #825, PR #836) and uses the external-provisioner canary image because v2.1.0 has been tagged, but the versioned image is not available yet. Therefore the PR is ready for review and further testing, but merging should wait.

Fixes: #823, #733, #729, #594, #526, #507

[pohly]

I've rebased, so only new commits are present now.

I also re-enabled the stress test. It passed in local testing.

[pohly]

This is a bit more complicated. When deploying via OLM, we need OLM to create the certificates. My understanding is that this can be done for "normal" webhooks, just not for conversion webhooks.

For deployment of the operator via YAML files we need to document how to set up those certs.

In both cases, re-configuring the cluster to use the webhooks is something that needs to be done manually.

If the user of the operator doesn't need the scheduler extensions, then it's still okay to start the statefulset unconditionally. The pod simply won't run.

[avalluri]

This is a bit more complicated. When deploying via OLM, we need OLM to create the certificates. My understanding is that this can be done for "normal" webhooks, just not for conversion webhooks.

I doubt if OLM can creates certificates for services created by the operator for the CRs, though it provides service certificates for operator itself.

[avalluri]

@pohly How this scheduler extension and the Pod mutation works in the context of multiple PmemCSIDeployments? I think we should /cannot support multiple deployments.

[pohly]

For the mutating pod webhook: it must use a resource name which includes the driver name. I thought it already did this, but currently it doesn't:

pmem-csi/pkg/scheduler/mutate_pod.go

Line 31 in 692818b

Resource = "pmem-csi.intel.com/scheduler"

For the scheduler extender: it must be triggered by the same resource name (driver name to be added) and then check only pods that belong to that instance - already implemented:

pmem-csi/pkg/scheduler/capacity.go

Lines 103 to 106 in 82bcde1

	if label.GetName() == "driver_name" &&
	label.GetValue() != c.driverName {
	return 0, wrongPod
	}

[pohly]

I doubt if OLM can creates certificates for services created by the operator for the CRs, though it provides service certificates for operator itself.

When we discussed it this morning, my conclusion was that the user needs to supply TLS credentials and that the webhooks need to be disabled if not provided.

[pohly]

In both cases, re-configuring the cluster to use the webhooks is something that needs to be done manually.

Have I mentioned how much I hate configuring a scheduler extension? 😢

The way we currently do it is by creating the certificates first, then setting up the "kubeadm init" configuration so that those certificates can be used by kube-scheduler. This no longer works when creating those certificates inside the cluster with a tool like cert-manager.

It is possible to set up the cluster without the extension enabled, then create certificates, and then reconfigure kube-scheduler. kubeadm upgrade supports that. But this looks complicated. I think I'll stick with the current approach.

[pohly]

We still need the external-provisioner in the controller as fallback for kubernetes-csi/external-provisioner#544

[pohly]

... or a custom controller which removes the selected-node annotation.

[pohly]

... or just set AllowedTopologies in our storage classes for late binding (kubernetes-csi/external-provisioner#544 (comment)).

[avalluri]

What does this "webhooks" mode does? Isn't it that valid mods are 'controller' and 'node'?

[avalluri]

Okay, after going through the design documentation changes, I understood that the new 'webhooks' mode replaces the existing 'controller' mode. But the driver command-line flags in pkg/pmem-csi-dirver/main.go is not synced with this.

[pohly]

Fixed.

[avalluri]

Why can't we do theses(remove external provisioner from controller) changes to our base driver YAML instead of a kustomize layer.

[pohly]

We can and I thought I had made that change when squashing. I'll check again.... it looks like I simply forgot to remove this file. It shouldn't be needed anymore.

Initially it wasn't done because distributed and centralized provisioning were both supported for a while.

[pohly]

Fixed.

[avalluri]

The reference is not removed from the table of contents.

[avalluri]

Wouldn't it be better to link to the documentation if any, instead of the issue.

[pohly]

Yes, now that that it is merged. Will change it.

[pohly]

Fixed.

[avalluri]

This makes 'enabling the metrics support' a hard dependency on 'scheduler extension'. The code should implement this by always enabling node metrics when scheduler extension is enabled.

[pohly]

Does the node part know whether scheduler extensions are enabled? I don't think so, because those parameters are passed to the controller part.

[pohly]

Or did you mean some other code than the main.go and pmem-csi-driver.go?

[avalluri]

Does the node part know whether scheduler extensions are enabled? I don't think so, because those parameters are passed to the controller part.

Yes, your are true, it is not possible to enforce this in the code. Would be good to add a note about "enabling metrics while using scheduler extension" in the documentation.

[pohly]

It's documented that the scheduler extension code uses metrics. I think that is enough. We don't describe how to disable metrics support (it's always on in YAML and operator deployments), so the risk that someone accidentally deploys incorrectly is low.

[avalluri]

Links in Table of Contents has broken.

[pohly]

Fixed, also in the body.

[avalluri]

I feel like offloading the webhook and scheduler extension part from the CSI driver to a separate binary makes much sense and cleaner than treating it as a mode of the driver.

[pohly]

What's the benefit? Have you considered the drawbacks?

"Cleaner" is subjective.

[pohly]

Is this perhaps about publishing one image that has multiple purposed vs. two different images with different content and different purpose?

I prefer to stick with a single image until someone has some strong argument against it. I doubt that the size of an image that only runs once in a cluster matters that much and publishing a single image is simpler, so I prefer that.

When we have a single image, then a single binary is likely to be smaller overall.

What that binary is called and how it is invoked hardly matters for end-users - they never invoke the binary. Renaming the binary is possible, but makes this PR more complex for (IMHO) little gain.

[avalluri]

My point was not about different images, more about a different binary. As now there is no central controller driver component, IMHO the scheduler code is not suitable to be part of the driver code.

[pohly]

We discussed this in the standup meeting.

"pmem-csi-driver" has already before served multiple different purposes and we agreed to not change that in this PR because doing so would make our image larger for not much gain (basically cleaner command line arguments of the binary which isn't that important).

We also raised the question whether "pmem-csi-operator" should be a separate binary. Probably not. But again, that is a different issue.

[pohly]

I've pushed a major update: now the operator supports configuration of the webhooks and distributed provisioning. Some E2E tests passed (in particular the ones for the operator), but for full coverage I let the CI do its work over night.

I'll double-check tomorrow, but I think there is only one minor TODO left (the one about reading the node port from the shell script in deploy.go).

I've not tried to ensure that all tests pass for all commits, so strictly speaking I would have to squash "distributed provisioning", "deploy: consistent object naming, app.kubernetes.io labels" and "operator: distributed provisioning, scheduler extensions". But I think it's easier to review when they are separate.

[pohly]

I'll double-check tomorrow, but I think there is only one minor TODO left (the one about reading the node port from the shell script in deploy.go).

That referred to TODOs inside the patch itself. I still need to address review comments.

[avalluri]

RegistryEndpoint no more needed and could be removed.

[pohly]

Removed (but not pushed yet).

[pohly]

I've pushed an update with various test fixes. Several tests still used the old labels and/or had issues with the optional controller part.

[avalluri]

I think the check supposed to be csid..cfg.Mode == Webhook, in that case we could remove this check and move the client creation under webhooks section in switch case.

[pohly]

Indeed, good catch. The initialization of cmm can also be moved.

[avalluri]

Is keeping this label(and also rest of this change) intentional ?

[pohly]

Yes. The usage of the "pmem-csi.intel.com/deployment" to mark different kinds of deployments in deploy.go doesn't change.

[avalluri]

Wouldn't be better to mention that the secret/public key is for both webhook and scheduler extender.

[pohly]

The scheduler extender is also a webhook. Both run in the controller.

With that in mind, it's better to say above that "The controller needs TLS certificates" - i'll use that instead. The name of the API was already chosen accordingly.

[avalluri]

Node metrics service cant be enabled regardless of the controller?

[pohly]

Node metrics are always enabled. There's nothing in the operator API to control that.

[pohly]

Looks like this breaks skew testing; I've not looked into it yet, but probably it is related to the older deployment not setting this label.

I wanted to ensure that only nodes where the driver actually runs (or at least ran) is used, vs. nodes where it is supposed to run. The latter is what we would get when using the label that admins need to set for the nodes with PMEM.

Will have to think about this some more...

[pohly]

The problem is that the 0.8.0 deployment doesn't handle the new topology:

pmem-csi-controller-0/pmem-driver@pmem..ker1: I0118 11:38:34.703703       1 tracing.go:19] GRPC call: /csi.v1.Controller/CreateVolume
pmem-csi-controller-0/pmem-driver@pmem..ker1: I0118 11:38:34.703770       1 tracing.go:20] GRPC request: {"accessibility_requirements":{"preferred":[{"segments":{"pmem-csi.intel.com/driver":"started"}}],"requisite":[{"segments":{"pmem-csi.intel.com/driver":"started"}}]},"capacity_range":{"required_bytes":115343360},"name":"pvc-269396e3-8ad6-4b4a-8296-9944cdd3acf0","parameters":{"eraseafter":"true"},"volume_capabilities":[{"AccessType":{"Mount":{"fs_type":"ext4"}},"access_mode":{"mode":1}}]}
pmem-csi-controller-0/pmem-driver@pmem..ker1: I0118 11:38:34.704225       1 controllerserver-master.go:181] Controller CreateVolume: Name:pvc-269396e3-8ad6-4b4a-8296-9944cdd3acf0 required_bytes:115343360 limit_bytes:0
pmem-csi-controller-0/pmem-driver@pmem..ker1: I0118 11:38:34.704251       1 controllerserver-master.go:86] Controller CreateVolume: Create VolumeID:pvc-26-328dad453a5df7f8348d57f9f7422e6b084a58303f5400af6f92906a based on name:pvc-269396e3-8ad6-4b4a-8296-9944cdd3acf0
pmem-csi-controller-0/pmem-driver@pmem..ker1: W0118 11:38:34.704273       1 controllerserver-master.go:236] failed to connect to : No node registered with id: 

I'm a bit surprised that "pmem-csi.intel.com/node" doesn't show up at all; might be related to how we configure the external-provisioner. Anyway, we can't change our topology labels without breaking compatibility, so I'll switch to using the node selector labels that we use for the PMEM-CSI node pods also for the storage classes.

[pohly]

Using our node selector label also doesn't work because the external-provisioner only seems to allow node labels that were set by the CSI driver:

pmem-csi-intel-com-node-q94vv/external-provisioner@pmem..ker2: I0118 14:46:54.540845       1 event.go:282] Event(v1.ObjectReference{Kind:"PersistentVolumeClaim", Namespace:"skew-4425", Name:"direct-testingv4nl9", UID:"fb94e3a9-62e5-44ec-b48b-33471452142c", APIVersion:"v1", ResourceVersion:"1204", FieldPath:""}): type: 'Warning' reason: 'ProvisioningFailed' failed to provision volume with StorageClass "pmem-pmem-csi-sc-ext4-skew-4425-unused": error generating accessibility requirements: selected node '"pmem-csi-pmem-govm-worker2"' topology 'map[pmem-csi.intel.com/node:pmem-csi-pmem-govm-worker2]' is not in allowed topologies: [map[feature.node.kubernetes.io/memory-nv.dax:true]]

[pohly]

At this point, running a "deprovisioner" in the PMEM-CSI controller looks like the simplest solution.

[pohly]

I've implemented that, albeit with the name "(PVC) rescheduler" because that's what it is.

I decided to keep the commits for the previous two attempts because it is something that we might want to refer to. I could also squash.

I used the existing external-provisioner helper lib. It's not a perfect fit because it needs a PV informer without ever doing anything with PVs, but using it was simpler than writing everything from scratch (which probably would have amounted to a lot of copy-and-paste).

I've pushed after some local testing ("make test", the new late binding test, some skew testing).

@avalluri can you take another look?

[pohly]

I've filed kubernetes-csi/external-provisioner#548 to discuss why we can't use the node label in AllowedTopology. With a modified external-provisioner we might be able to restore that solution, which would be better than the current solution alone because then a bad node selection should happen a lot less. Even if we can enable that again, rescheduling as a second line of defense will still be useful (think "node selected base on labels", "labels change", "provisioning no longer possible" -> "must reschedule").

[pohly]

I intentionally left #857 out of the current implementation. It's not going to be a problem for a while and can be addressed later.

[pohly]

With these changes, the ClusterRole becomes a combination of permissions for the scheduler extensions and for the rescheduler.

I suspect putting "webhooks" into the name of the ClusterRole was a poor choice from the beginning; "controller" would have been better. I didn't change it in this commit because I wanted to keep it small, but I can do it in a separate cleanup commit.

[avalluri]

@pohly Why should we remove allowedTopologies form the storageclass?

[pohly]

Because it didn't work - see the commit message in 7c4ee86

[avalluri]

If I am not wrong That attempt was for using {"pmem-csi.intel.com/driver":"started"} as allowed topology. Isn't it?. But it should work for {storage: "pmem} as before. And anyways your are passing the same label for controller's --nodeSelecter here. Then why exactly this --nodeSelector is needed as we get the same info from from the storage class?

[pohly]

If I am not wrong That attempt was for using {"pmem-csi.intel.com/driver":"started"} as allowed topology.

That's the first attempt (a03a700). The second attempt was {storage: pmem} which also didn't work.

Then why exactly this --nodeSelector is needed as we get the same info from from the storage class?

We can't use AllowedTopology at all at the moment, not with the currently released external-provisioner. Even if we can start using AllowedTopology with some future external-provisioner, then the label check in kube-scheduler may still go wrong when labels get changed (less likely, but can happen). The rescheduler then handles that case and thus remains useful.

[avalluri]

I am bit confused, where exactly the provided nodeSelecter is used in the code, I could only see that this is used to check if the rescheduler should be started or not by the controller. But I couldn't find anywhere the contents of the provided nodeSeleter is used in the code. In this case why can't it be a simple boolean value. @pohly Pardon and please correct me if I miss something.

[pohly]

It's passed to the rescheduler here:

pmem-csi/pkg/pmem-csi-driver/pmem-csi-driver.go

Lines 219 to 223 in 3a7d2cf

	pcp = newRescheduler(ctx,
	csid.cfg.DriverName,
	client, pvcInformer, scInformer, pvInformer, csiNodeLister,
	csid.cfg.nodeSelector,
	serverVersion.GitVersion)

[avalluri]

Yes, but it is not used used by rescheduler?

[pohly]

And it should be used here:

pmem-csi/pkg/pmem-csi-driver/rescheduler.go

Lines 196 to 201 in 3a7d2cf

	driverMightRun := true
	for key, value := range node.Labels {
	if node.Labels[key] == value {
	driverMightRun = false
	}
	}

... but isn't! Duh. That code is of course bogus. Not sure what I was thinking. I'll fix it.

[pohly]

I also need to add unit tests.

[pohly]

Done, see updated commit.

[avalluri]

I think this check is wrong, It is where pcp.nodeSelecter supposed to be used?

[pohly]

Yes.

[pohly]

I've fixed the commit.

[avalluri]

Isn't it that we should make sure that the selected node has enough PMEM capacity instead of just checking if the driver is running on this node?

[pohly]

If the driver is running, then it will do that check itself.

Checking capacity here is redundant and racy (we check capacity, fail, removed selected node, while sometime later the driver processes the PVC and creates a volume although it is no longer assigned to the node).

[pohly]

In other words, the rescheduler only may handle the case where the driver definitely isn't running.

[avalluri]

Looks good to me

[pohly]

There was a test flake (?), filed as #858.

I don't think that it was caused by this PR, so I am going to merge it despite that.