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

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

[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

[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 https://github.com/prometheus-community/helm-charts/issues/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?

[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.

[mariuskimmina]

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