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

[common] Add affinity to common chart #27352

Open
2 tasks done
alexmorbo opened this issue Jun 6, 2024 · 11 comments
Open
2 tasks done

[common] Add affinity to common chart #27352

alexmorbo opened this issue Jun 6, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@alexmorbo
Copy link

alexmorbo commented Jun 6, 2024

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 #2266

Thanks

I've read and agree with the following

  • I've checked all open and closed issues and my request is not there.
  • I've checked all open and closed pull requests and my request is not there.
@alexmorbo alexmorbo added the enhancement New feature or request label Jun 6, 2024
@stavros-k stavros-k transferred this issue from truecharts/public Jun 6, 2024
@stavros-k
Copy link
Collaborator

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

Try this:
"\"false\""

@alexmorbo
Copy link
Author

@stavros-k this works, thanks!

@stavros-k stavros-k self-assigned this Jun 6, 2024
@stavros-k
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Contributor

Kampe commented Jul 11, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

Kampe commented Jul 11, 2024

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
Copy link
Collaborator

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
Copy link
Member

@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
Copy link
Member

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

@PrivatePuffin
Copy link
Member

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.

@truecharts truecharts locked as too heated and limited conversation to collaborators Jul 11, 2024
@truecharts truecharts unlocked this conversation Oct 7, 2024
@PrivatePuffin PrivatePuffin changed the title Add affinity to common chart [common] Add affinity to common chart Oct 7, 2024
@PrivatePuffin PrivatePuffin transferred this issue from another repository Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants