Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SpinApp: Support nodeSelector config #217

Open
vdice opened this issue Apr 24, 2024 · 9 comments
Open

SpinApp: Support nodeSelector config #217

vdice opened this issue Apr 24, 2024 · 9 comments
Labels
enhancement New feature or request needs discussion

Comments

@vdice
Copy link
Contributor

vdice commented Apr 24, 2024

Feature request to support sending in user-defined nodeSelector config to the pod(s) representing a given SpinApp.

@endocrimes
Copy link
Contributor

We already somewhat support that (and require it in most production deployments) via RuntimeClasses - I'd love to hear more about where you envision wanting those and why on an app/executor level?

@endocrimes endocrimes added enhancement New feature or request needs discussion labels Apr 24, 2024
@vdice
Copy link
Contributor Author

vdice commented Apr 24, 2024

Selection via RuntimeClass is probably sufficient for my general use case, which is having control over which subset of nodes in a cluster SpinApps can run on. I don't currently need a finer-grained scheduling strategy than that, so unless others chime in, we can use the RuntimeClass approach for now.

@kate-goldenring
Copy link
Contributor

I think we are still missing the ability to ensure Spin pods are only scheduled to nodes that were provisioned by the runtime class manager. For example, in my setup, i have a 3 node cluster. I've only annotated the one named aks-apps-59045239-vmss000000 (kubectl annotate node --all runtime=containerd-shim-spin), so only that one is provisioned with the Spin shim.

However, spin apps were still scheduled there and left in ContainerCreating state:

density-0-4-7fc4dd8f78-862j6         1/1     Running             0          4m51s   10.10.2.183   aks-apps-59045239-vmss000000     <none>           <none>
density-0-5-6f8c9f9855-mqkqr         1/1     Running             0          4m51s   10.10.2.128   aks-apps-59045239-vmss000000     <none>           <none>
density-0-6-7fcb448876-7l5f2         1/1     Running             0          4m51s   10.10.2.170   aks-apps-59045239-vmss000000     <none>           <none>
density-0-7-5f6775cc4f-cdvv8         0/1     ContainerCreating   0          4m51s   <none>        aks-system-15560169-vmss000000   <none>           <none>
density-0-8-54676f889d-pxh8s         0/1     ContainerCreating   0          4m51s   <none>        aks-system-15560169-vmss000000   <none>           <none>
density-0-9-698557c7d8-l64sn         1/1     Running             0          4m51s   10.10.2.214   aks-apps-59045239-vmss000000     <none>           <none>

Which is due to the shim not existing on that node:

Tolerations:                 node.kubernetes.io/not-ready:NoExecute op=Exists for 300s
                             node.kubernetes.io/unreachable:NoExecute op=Exists for 300s
Events:
  Type     Reason                  Age                  From               Message
  ----     ------                  ----                 ----               -------
  Normal   Scheduled               4m39s                default-scheduler  Successfully assigned default/density-0-7-5f6775cc4f-cdvv8 to aks-system-15560169-vmss000000
  Warning  FailedCreatePodSandBox  2s (x22 over 4m39s)  kubelet            Failed to create pod sandbox: rpc error: code = Unknown desc = failed to get sandbox runtime: no runtime for "spin" is configured

@kate-goldenring
Copy link
Contributor

It looks like we need to make some more considerations with scheduling with RuntimeClass: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/585-runtime-class/README.md#runtimeclass-scheduling

@kate-goldenring
Copy link
Contributor

I just noticed the call out on this in the SpinKube installation docs. Thanks @endocrimes

@kate-goldenring
Copy link
Contributor

Assuming you provisioned the shim with kwasm/runtime-class-manager, the following works well:

  1. Label the provisioned nodes:
kubectl annotate node -l runtime=containerd-shim-spin kwasm.sh/kwasm-node=true
  1. Apply the runtime class with the nodeSelector for the label:
apiVersion: node.k8s.io/v1
kind: RuntimeClass
metadata:
  name: wasmtime-spin-v2
handler: spin
scheduling:
  nodeSelector:
    runtime: containerd-shim-spin

@chokosabe
Copy link

Doing this via the RuntimeClass feels a lot like an anti pattern.

Its basically saying the user would be editing the RuntimeClass to set this. For anything else they are running this wont be the case.

Setting via the SpinApp CRD makes a lot more sense.

@endocrimes
Copy link
Contributor

@chokosabe We already have to have a runtime class in SpinKube (for the CRI to correctly detect that a shim should be used) - so we also rely on the runtimeClass expressing that "these nodes meet this class" (I've done the same with gvisor in the past, but it's as-designed).

It feels a bit awkward the first time you come across it (esp if you haven't really used RuntimeClasses for things that don't come with your k8s distribution / or on bare metal) but it has a nice benefit of not having to duplicate cluster-specific scheduling constraints across apps for portability, when already saying "i have this dependency".

In the future we should probably handle a lot of that for you in the runtime-class-manager (which would be super cool for also being able to say "at least version X of the shim" at the same time 😅 ).


More generally we probably do want to also support more fine-grained scheduling options in the operator in the future tho (e.g if the shim gets support for GPUs), but I was hesitant to add it to cover things that should be handled with the RuntimeClass

@kate-goldenring
Copy link
Contributor

I think it is worth considering adding node selectors to the SpinApp. I think we could see them as additive, such that node selectors could be added to the runtime class and the SpinApp and we'd define some precedence

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs discussion
Projects
None yet
Development

No branches or pull requests

4 participants