truecharts / library-charts

Helm Library Charts for TrueCharts
Other
29 stars 37 forks source link

Add affinity to common chart #826

Open alexmorbo opened 3 months ago

alexmorbo commented 3 months ago

Is your feature request related to a problem?

I use some charts from truecharts in my home k8s cluster with some nodes. I need not to schedule some apps to nodes, which got gpu. So affinity is the best way to do this.

Describe the solution you'd like

  {{- with .Values.affinity }}
  affinity:
    {{- toYaml . | nindent 8 }}
  {{- end }}

And in chart something like this:

affinity = {
      nodeAffinity = {
        requiredDuringSchedulingIgnoredDuringExecution = {
          nodeSelectorTerms = [
            {
              matchExpressions = [
                {
                  key = "node.home.lab/capability"
                  operator = "NotIn"
                  values = ["gpu"]
                },
                {
                  key = "home.lab/proxmox-node"
                  operator = "In"
                  values = ["r730xd-1"]
                }
              ]
            }
          ]
        }
      }
    }

Describe alternatives you've considered

I tried:

node_selector = {
    "home.lab/proxmox-node" = "r730xd-1",
    "home.lab/has-gpu-node" = "false"
  }

but got error:

json: cannot unmarshal bool into Go struct field PodSpec.spec.template.spec.nodeSelector of type string

Additional context

I found, that it was in plan, but not realized in https://github.com/truecharts/charts/issues/2266

Thanks

I've read and agree with the following

stavros-k commented 3 months ago

Quick work around until this is implemented (and also the string -> bool issue is fixed)

Try this: "\"false\""

alexmorbo commented 3 months ago

@stavros-k this works, thanks!

stavros-k commented 3 months ago

Affinity will need a bit more work, as I will have to template the spec and add validations and tests. It will take some time, as I have some other things on my plate.

PrivatePuffin commented 3 months ago

Affinity will need a bit more work, as I will have to template the spec and add validations and tests. It will take some time, as I have some other things on my plate.

I suggest not going over affinity for now, we've WAY more things with a higher prio, both in common and in the new required tooling.

Kampe commented 2 months ago

This is pretty important feature for any and all charts with multiple pods that mount the same PVC, which many truecharts do, see almost any of the *arr stack.

This makes deploying these charts on multi node clusters very painful.

If you're going to implement a common chart you should finish up the k8s spec so you can actually utilize these charts in more then just your own homelabs on truenas.

stavros-k commented 2 months ago

This is pretty important feature for any and all charts with multiple pods that mount the same PVC, which many truecharts do, see almost any of the *arr stack.

This makes deploying these charts on multi node clusters very painful.

If you're going to implement a common chart you should finish up the k8s spec so you can actually utilize these charts in more then just your own homelabs on truenas.

TrueNAS is deprecated since couple of months now.

Importance is relative. There are other things currently that have priority. If you feel this feature is important to you, consider opening a discussion on discord to discuss specifics on the implementation and then follow up with a PR. Otherwise please subscribe to this issue for upgrades.

I don't get the "Finish up the k8s spec". We are not looking to implement every little corner of k8s spec unless there is a need for it. Even if we did, not everything would be exposed to user to change. Because if we do, it would be pretty close to defining your own k8s objects.

Kampe commented 2 months ago

Sane defaults are needed, the whole idea of a helm chart is to expose the relevant bits and allow them to be configurable.

it would be pretty close to defining your own k8s objects.

this is helm charts in a nutshell

stavros-k commented 2 months ago

Well if you check our common, almost everywhere there is a bit of "magic" involved to actually reduce boilerplate and/or added validation.

So there won't be a plain "take this and put it there" approach. At least by my side. I can't say when this feature will be added, as I currently don't have much free time.

But anyone is free to implement it (after discussing it first on the dev channel in discord)

PrivatePuffin commented 2 months ago

@Kampe I don't get your attitude. We're a small opensource project and we don't have unlimited resources.

Just because you want something, doesn't mean its a priority for us, the magic of opensource is that you can add it yourself. The whole comment about TrueNAS makes even less sense as we don't even build for TrueNAS anymore nor do most of our devs even use TrueNAS at all at this point.

I also completely don't get the need for this in, for example the *arr stack. I use the Arr stack just fine. They all share one single NFS PVC reference and yes, I use 3 cluster nodes.


The actual issue seems to be that you don't run RWX compatible storage and want node-affinitiy to deal with that.

That's completely fine and understandable, but don't go run in here having an attitude while we're working our asses off just because your precious feature wasn't added in a month.

PrivatePuffin commented 2 months ago

The only thing you have accomplicated here, is that this issue is now getting locked, preventing other people from responding. Congrats.

PrivatePuffin commented 2 months ago

Also: we don't allow discussions on Github, that's what we use discord for. Next time we will give you a temporary ban for this.