✨ (helm/v2-alpha) add tolerations, nodeselector and affinity

[robinlioret]

Adds nodeSelector, affinity and tolerations to the manager deployment template and values of the helm chart (v2-alpha).

Related issue: #5246

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: robinlioret / name: Robin LIORET (a417039acf11e7f7a6e7ec3ebd5831db890010ad)

[k8s-ci-robot]

Hi @robinlioret. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[vitorfloriano]

/ok-to-test

[k8s-ci-robot]

@robinlioret: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[robinlioret]

The PR looks ready to be merged.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, robinlioret

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[robinlioret]

Thanks for approving the PR. I see two failing tests though.

• Test GoReleaser and CLI Version / go-releaser-test (pull_request)

Fails while fetching https://github.com/anchore/syft/releases/latest (ERR 503) Is it a rate limitation ? Can we restart this specific test to confirm ?

• E2E Testdata Sample / e2e-tests-project-v4 (pull_request)

Fails like nothing was listening on the metric port. I tried to generate the test-data again. No change.

manager:
    Port:          9443/TCP (webhook-server)
    Host Port:     0/TCP (webhook-server)
    Args:
      --metrics-bind-address=:8443

Is it a bug in the installation file ? Shouldn't it expose the 8443 port as well ?

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[camilamacedo86]

Hi @robinlioret,

Thanks for the contribution.

We can’t accept this change.

The Helm chart is only used to package and distribute a project. It is not meant to define or change what the project supports.

Things like:

• nodeSelector
• affinity
• tolerations
• supported architectures / OS

are decisions made by the project maintainers, based on how the controller image is built and tested. That must be configured in the manager and should appear in the manager file of the helm chart but not exposed via values.

If we expose these as Helm values, end users could deploy the controller on nodes or platforms that the project does not support. That would break the project’s support guarantees and may not work at all.

If a project needs specific scheduling rules, they must be defined by the project itself (for example in the manifests or kustomize), not configured by Helm values.

For this reason, we don’t expose these options in the generated Helm chart.

Thanks again for the proposal.

/hold

[robinlioret]

If I understand correctly, from your point of view. Configurations like the manager pod's affinity, nodeSelector and tolerations should not be exposed to end-users. This is the project maintainer responsability because any inconsiderate change to these values may lead to a broken deployment.

I disagree with that statement, here's why.

Regarding the generated Helm chart. We are currently exposing the value manager.image.repository where any modification can easily break deployments. Following your logic, it should not be exposed either. Then, you could argue than end-users may have an internal registry for security reasons. This would be totally legit.

The same reasoning is applicable to many (if not all) values exposed.

However, regarding toleration and nodeSelector, an end-user may need to assign the operator to a specific set of nodes.

As a matter of fact, it is my case and the reason why I opened this pull request in the first place. We have a node pool dedicated to tools and core capabilities. This nodepool have a taint. Therefore, as an end user, I need to be able to configure the nodeSelector and the toleration values. We also need to customize affinity to guarantee high availability if replicaCount is higher than 2. We currently have to re-implement affinity, nodeSelector and tolerations every time we regenerate the chart.

affinity, nodeSelector and tolerations are exposed in many charts for the same reason, even in the default chart created by helm when you run helm create test-chart.

This way, I see no valid reason to forbid end-users to customize these values.

We should provide:

• A transparent behaviour that ignores affinity, nodeSelector and tolerations if not specified (keep the behaviour we have today)
• But keep the possibility for the end-user to customize them. Like we do for image repository and other values.
• If values are defined by the operator developper in the Kustomize files then it should be use as a default value for the helm chart

[camilamacedo86]

Hi @robinlioret,

Changing manager.image.repository is typically about mirroring the same image (private or air-gapped registries). It doesn’t change what the project supports. I do not think that we can use this an as example.

Exposing affinity, nodeSelector, and tolerations is different. It doesn’t add platform support, but it allows consumers to deploy the controller outside the authors’ validated and supported environments (including OS/arch). That support boundary must be owned by the operator authors, not end users. However, I can see the cases where it might be required and useful.

The fact that helm create exposes these by default isn’t a strong argument here. That scaffold assumes the chart is authored and consumed by the same party. Kubebuilder’s Helm chart is meant only to package and distribute a solution, not to expose additional end-user knobs that can result in unsupported deployments.

However, you might be right, and we may need to add it.

I see there are cases where placement customization is needed, but exposing these by default in a packaging-only scaffold is something I’m still thinking about. Can you please give me a few days to think more about this one?

[robinlioret]

I'm glad we understand each other.

I agree that this give more freedom to the end-users than the author might expect.

Nonetheless, I think that when an end-user needs such configuration possibilities, they know their requirements and are aware of what they're doing. For most users, the default configuration will be enough.

Take all the time you need, this is not urgent.