fix master taint implementation - linting problems

[ioagel]

Adds the kube-vip IP as a Subject Alternative Name in the TLS cert.
Prevents kubectl erroring out when connecting to cluster due to virtual ip missing in the SANs of the api server certificate.

[timothystewart6]

Hey! Thanks for the fixes! Looks like the tests are failing.

[ioagel]

Oh well... typo was the reason!!! another advantage of having tests ;-)

[ioagel]

I have fixed the bugs, but linting is failing outside my commits. I fixed a couple of them and I will try to hack some more fixes.
Take a look at my fork: pull request

[ioagel]

• consolidates handling of master taints in group_vars, all.yml

...

k3s_node_exists: "{{ 'true' if groups['node'] | default([]) | length >= 1 else 'false' }}"

...

extra_server_args: >-
  {{ extra_args }}
  {{ '--node-taint node-role.kubernetes.io/master=true:NoSchedule' if k3s_node_exists | bool else '' }}
  --tls-san {{ apiserver_endpoint }}
  --disable servicelb
  --disable traefik

There is no need for users to mess with the roles even for the taint functionality. The addition to extra_server_args avoids duplication in files: roles/k3s/master/tasks/main.yml and roles/k3s/master/templates/k3s.service.j2. For k3s anyway, taints are another extra server arg.
Now even if a user wants to run for example, 3 master and 2 worker nodes with all of them being schedulable, they just have to delete the taint entry in extra_server_args variable.

The new addition in extra_server_args: --tls-san {{ apiserver_endpoint }} is mandatory, because the ip of our virtual ip needs to be in the SANs of the api server certificate, otherwise we cannot access our cluster.

About testing and linting

In the ipv6 molecule test, because we override the extra_server_args, I added the 2 new entries:

• --tls-san {{ apiserver_endpoint }}
• {{ '--node-taint node-role.kubernetes.io/master=true:NoSchedule' if k3s_node_exists | bool else '' }}

The rest of the changes were to make the linter happy, despite my commits had nothing to do with breaking the linter. I wonder if anyone could make any pull request without the linting action failing... ;-)
I think it is fixed even in a hacky way!
By the way I noticed intermittent failures in the metallb wait task in GitHub actions and I had to increase it to 120s. After that I had no failures.

[sleiner]

@ioagel

The rest of the changes were to make the linter happy, despite my commits had nothing to do with breaking the linter. I wonder if anyone could make any pull request without the linting action failing... ;-)

No, the lint is currently just plain broken 😅 {{ansible-lint}} 6.6.0 (which was released a few days ago) has introduced some new false positives. I have issued a separate PR for that problem: #96.

[ioagel]

The default molecule seems to have failed due to the infamous GitHub rate limiting problem…

[jurlwin]

So, I think tainting is both smart and a curse - with where it's going. The first time I built a cluster on under-powered (lack of memory) RPI-3, and the master node took on load and became unresponsive, it was a huge problem ;)

My current build / setup is 4 RPI CM4s with enough memory (more than 1G) to run loads and I wanted to test the HA etcd, add longhorn and test rolling patches for the cluster once I get going, so tainting the masters would be counter productive.

In 1 master, 3 nodes, taint the heck out of the master :). but with masters & 1 node - i don't want to taint.

How about we set a variable - k3s_taint_master_nodes and let the users set it?

I think the "computed" tainting will typically be backwards for each use case. Those wanting to test HA, with a 3 or 4 node cluster, you need 3 nodes for HA, so you will want workload there...whereas others...may want to taint, even in a 2 node system, 1 master, 1 worker...

[timothystewart6]

So, I think tainting is both smart and a curse - with where it's going. The first time I built a cluster on under-powered (lack of memory) RPI-3, and the master node took on load and became unresponsive, it was a huge problem ;)

My current build / setup is 4 RPI CM4s with enough memory (more than 1G) to run loads and I wanted to test the HA etcd, add longhorn and test rolling patches for the cluster once I get going, so tainting the masters would be counter productive.

In 1 master, 3 nodes, taint the heck out of the master :). but with masters & 1 node - i don't want to taint.

How about we set a variable - k3s_taint_master_nodes and let the users set it?

I think the "computed" tainting will typically be backwards for each use case. Those wanting to test HA, with a 3 or 4 node cluster, you need 3 nodes for HA, so you will want workload there...whereas others...may want to taint, even in a 2 node system, 1 master, 1 worker...

Yeah, I tend to agree. At first I thought it might be nice to apply the taint according to typical uses however I am quickly seeing how this can get out of hand. I am starting to think we might just revert and not apply any taint and just let those taint them how they want.

[ioagel]

So, I think tainting is both smart and a curse - with where it's going. The first time I built a cluster on under-powered (lack of memory) RPI-3, and the master node took on load and became unresponsive, it was a huge problem ;)

My current build / setup is 4 RPI CM4s with enough memory (more than 1G) to run loads and I wanted to test the HA etcd, add longhorn and test rolling patches for the cluster once I get going, so tainting the masters would be counter productive.

In 1 master, 3 nodes, taint the heck out of the master :). but with masters & 1 node - i don't want to taint.

How about we set a variable - k3s_taint_master_nodes and let the users set it?

I think the "computed" tainting will typically be backwards for each use case. Those wanting to test HA, with a 3 or 4 node cluster, you need 3 nodes for HA, so you will want workload there...whereas others...may want to taint, even in a 2 node system, 1 master, 1 worker...

This pull requests fixes most of the problems. It consolidates handling of the taint in a single file: all.yml.
Now it is very easy to manually disable it by setting: k3s_node_exists = 'false'.

By default if even a single node exists , taint is applied. However, by changing the length check, it is fully customizable: k3s_node_exists: "{{ 'true' if groups['node'] | default([]) | length >= 2 else 'false' }}".
Now you need at least 2 nodes for the taint to be applied.

[sleiner]

I agree with @ioagel here: This PR provides a sensible default for the "best-practice HA" setup, but users are in full control via the group vars. To make that more clear, I would propose renaming k3s_node_exists to k3s_taint_servers (or k3s_taint_control_nodes or whatever).

If we want to simplify even further, we could get rid of the variable altogether and just include the required arguments for server tainting in the docstring of extra_server_args (but we would also lose the automatism for tainting servers if you have dedicated nodes).

In any case, I think it's great that we can now use the same VIP manifest regardless of tainting or not 🎉 So it really only comes down to the server args now.

[sleiner]

If this is to be merged, could you please remove the parts of the patch that were only supposed to make the linter happy? :-)

[ioagel]

I agree with @ioagel here: This PR provides a sensible default for the "best-practice HA" setup, but users are in full control via the group vars. To make that more clear, I would propose renaming k3s_node_exists to k3s_taint_servers (or k3s_taint_control_nodes or whatever).

If we want to simplify even further, we could get rid of the variable altogether and just include the required arguments for server tainting in the docstring of extra_server_args (but we would also lose the automatism for tainting servers if you have dedicated nodes).

In any case, I think it's great that we can now use the same VIP manifest regardless of tainting or not tada So it really only comes down to the server args now.

@sleiner check this pull request to my fork, do you agree with the naming? I was working on that already!!!! hehe...

I added a comment to instruct how to override the auto check and change the taint manually. By the way I used a boolean value, no need to convert string to bool.

The pull request is ready to be merged.

[sleiner]

@ioagel Very nice :-) Still, could you please remove the linter stuff?

[ioagel]

@ioagel Very nice :-) Still, could you please remove the linter stuff?

@sleiner I removed them ;-)

[timothystewart6]

Thank you! I like the flexibility: computed for best practice and overrides for anything else.

TODO:https:// in a future PR (or this one):

I feel like we should also check for these taints either via molecule, or in process with ansible + kubectl while applying in the post step (preferred).

[ioagel]

Thank you! I like the flexibility: computed for best practice and overrides for anything else.

TODO:https:// in a future PR (or this one):

I feel like we should also check for these taints either via molecule, or in process with ansible + kubectl while applying in the post step (preferred).

Very good idea... Using molecule maybe we can expand the existing deploy example test case and assert that when k3s_master_taint is true, all pods are assigned to worker nodes only, or we can try a new deployment with node affinity to a master and expect to fail (Pending state) or for the false case we use again node affinity to deploy to a master successfully etc...

[timothystewart6]

Yes! Or even test with the existing metallb pods, that way we can do it in process too! This is nice because it will fail CI and also alert the user who is using it (2 for 1) :)

[timothystewart6]

Thank you all so much!