Add aws-s3-csi-controller component
#299
Conversation
| log.V(debugLevel).Info("Pod pending to be scheduled") | ||
| case corev1.PodRunning: | ||
| log.V(debugLevel).Info("Pod is running") | ||
| case corev1.PodSucceeded: |
There was a problem hiding this comment.
We need to verify that MP exists with an error exit code on all crashes. I'm not sure it does right now
There was a problem hiding this comment.
So, depending on Mountpoint's exit code, non-zero for failures and zero for success, Mountpoint Pod will either restart (due to RestartPolicy: corev1.RestartPolicyOnFailure) or transition into PodSucceeded state respectively. This part deletes PodSucceeded Mountpoint Pods.
There was a problem hiding this comment.
I know, we just need to ensure that Mountpoint exists with non-zero exit codes on failure.
There was a problem hiding this comment.
I've manually verified while testing Mountpoint with incorrect credentials that this is the case. I think we also have some end-to-end tests where it tests failed mount, but I'm not sure if we also want to have a specific test case for exit code.
| ) | ||
|
|
||
| func TestGeneratingMountpointPodName(t *testing.T) { | ||
| t.Run("Consistency", func(t *testing.T) { |
There was a problem hiding this comment.
These tests seem like they'd be better by being property based tests
|
|
||
| expectNoMountpointPodFor(pod, vol) | ||
|
|
||
| vol.bind() |
There was a problem hiding this comment.
Semantically, what does this mean in terms of k8s runtime? Where the binding isn't explicit in the PV, but gets bound at pod schedule time?
There was a problem hiding this comment.
This is actually the default behavior if you don't pre-bind your PVC and PV. You specify a PVC in your Pod with some capacity, and once your Pod scheduled, Kubernetes tries to bind a PV for your PVC that fulfills your capacity requests. In case of static provisioning, cluster admin must pre-provision PVs for this to work, in case of dynamic provisioning another controller like external-provisioner sees that and dynamically creates a PV and binds that to that PVC. See for more detail https://kubernetes.io/docs/concepts/storage/persistent-volumes/#binding
|
|
||
| var _ = Describe("Mountpoint Controller", func() { | ||
| Context("Static Provisioning", func() { | ||
| Context("Scheduled Pod with pre-bound PV and PVC", func() { |
There was a problem hiding this comment.
Are we effectively testing that if there's a volume with the S3 CSI Driver, and it is bound to a pod that is scheduled, waitAndVerifyMountpointPodFor, otherwise, expectNoMountpointPodFor?
There was a problem hiding this comment.
If so, then a property based test using a state machine might be useful here to ensure all combinations are reached
There was a problem hiding this comment.
Yes, those are the expectations we assert during the tests. I think we can look into using property tests, but I think I'd also like to keep these known/expected cases here
unexge
force-pushed
the
unexge/podmounter-controller
branch
from
6fed7e3 to
0d2c5cb
Compare
|
|
||
| log.V(debugLevel).Info("Found bound PV for PVC", "pvc", pvc.Name, "volumeName", pv.Name) | ||
|
|
||
| err = r.spawnOrDeleteMountpointPodIfNeeded(ctx, pod, pvc, pv, csiSpec) |
There was a problem hiding this comment.
This can be called multiple times for the same volume. I don't think this is a problem though, but is it worth migrating this outside the for loop and only calling if we have no requeue and no errors? That way we could call in parallel for each PVC, but that might just be premature optimisation.
There was a problem hiding this comment.
I think I'd like this to be called even some other volume for this Pod requires requeueing, so one poisonous volume won't block other volumes to progress. Yeah, I think calling this in parallel would be a pre-mature optimization and would also be problematic if we want to share Mountpoint instances between multiple volumes. They would be racing to spawn Mountpoint Pods and that might cause duplicate Pods.
unexge
force-pushed
the
unexge/podmounter-controller
branch
from
0d2c5cb to
77c6842
Compare
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
…r envtest Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
volume types Signed-off-by: Burak Varlı <[email protected]>
Signed-off-by: Burak Varlı <[email protected]>
unexge
force-pushed
the
unexge/podmounter-controller
branch
from
77c6842 to
5303a67
Compare
This is part of #279.
This new component,
aws-s3-csi-controller, will be the entry point for our controller component. It's using controller-runtime, specifically, it implementsReconcilerinterface to reconcile Pods in the cluster. It schedules Mountpoint Pods in turn to cluster events such as a new workload Pod using a PV backed by S3 CSI Driver getting scheduled into the cluster. It'd then schedule a Mountpoint Pod for that workload Pod in the same node to provide volume for that Pod.#279 is still WIP and this component contains some TODOs and it's not in use anywhere except in tests at the moment.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.