[prometheus-node-exporter] prevent node exporter from being scheduled on fargate or virtual nodes

[mariuskimmina]

What this PR does / why we need it

This PR prevents node-exporter from being scheduled on fargate

Which issue this PR fixes

• fixes [prometheus-node-exporter] Prevent node exporter from being scheduled on fargate #4735

Special notes for your reviewer

If my understanding is correct, then for people that have already added this nodeAffinity this would not be a breaking change, it would simply be redundant - but this I am not completely sure about.

Checklist

• DCO signed
• Chart Version bumped
• Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

[zeritti]

Thank you, @mariuskimmina, for your PR. That would be a useful change for users of mixed EKS clusters. Similarly in AKS with virtual nodes (type: virtual-kubelet) which could also be supported in your change.

Now, affinity is a user settable field and may already include nodeAffinity. In your current implementation, the field would eventually get overwritten by the user's nodeAffinity. Both user's configuration and the template's field have to be merged. I'd suggest using a helper template for the purpose.

The other option is inserting the field directly in the values file's affinity. You probably meant to do this. Personally, I'd prefer doing it as described above, though.

[mariuskimmina]

Hey @zeritti, thanks for taking the time to look at it.

I updated the PR with a template approach now.
So far I've only tested it using helm template prometheus-node-exporter ./prometheus-node-exporter so the default value is working fine, I haven't tested the merging with user defined nodeAffinity yet.

[mariuskimmina]

I found the Azure one you were talking about, my only concern here is that key: type seems so generic. If you want to I am fine with adding this as a default as well.

  affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
          - matchExpressions:
            - key: type
              operator: NotIn
              values:
                - virtual-kubelet

[mariuskimmina]

Added the Azure version and tested a few more cases, let me know what you think @zeritti

[monotek]

i don't see why is this needed if there is already a generic solution available.

[mariuskimmina]

i don't see why is this needed if ther is already a genric solution available.

It's a common problem for people that run EKS with a mixture of Fargate and EC2 nodes, as seen by the number of opened issues around this in the past. I think this would be a sane default configuration to provide that can safe some people the trouble of having to search for this and then add it themselves.

[mariuskimmina]

Any more thought on this @monotek @zeritti @zanhsieh ?

I think adding this as default value would save quite a few people the time of having to search for this and add it themselves. I think if you are using a mixture of EC2 nodes and Fargate you'll always want to have this nodeAffinity and if you are not using Fargate it shouldn't have any negative impact either.

[mariuskimmina]

Any update @monotek @zeritti @zanhsieh ?
Since the issue #4735 got some support I wanted to bring this topic up again.

Any more thought on this @monotek @zeritti @zanhsieh ?

I think adding this as default value would save quite a few people the time of having to search for this and add it themselves. I think if you are using a mixture of EC2 nodes and Fargate you'll always want to have this nodeAffinity and if you are not using Fargate it shouldn't have any negative impact either.

[z0rc]

i don't see why is this needed if there is already a generic solution available.

There is no generic solution available. Proposed change collects solution that proved to be working. It's better than encourage everyone to copy-paste some values from comment hidden deep in pile of github issues.

[monotek]

I let this decisson to the maintainers of the chart...

[BobDu]

Given the extensive use of EKS and AKS, offering more out-of-the-box default configurations would be a better option.

At least theoretically, it wouldn't cause any inconvenience to users who do not use these two services, right?

let maintainers decide

[mariuskimmina]

Any update on this @zeritti @zanhsieh ?

Just rebased and bumped the chart version again.
So far it seems as time goes on only more people are in favor of this.

[zeritti]

Since there is no indication of the default hard node affinity in the values file, please, enter a comment at the affinity field saying that the default node affinity excludes Fargate nodes and virtual kubelets from scheduling unless overriden by hard node affinity set in the field.

[mariuskimmina]

Thanks again for reviewing @zeritti
I added the requested comment to the affinity field and bumped the minor version instead

[zeritti]

LGTM