Closed sholdee closed 9 months ago
@timothystewart6 Tested working with your upstream version tags, including your recent update to k3s version. Let me know what you think.
Wow! Thank you for this! Sorry for the delay, I have been trying to get CI running again to be sure that PRs pass tests...
A couple of thoughts:
First, thank you! This opens the doors for many possibilities in the future, so thank you!
Second, it's unfortunate that this is a breaking change. We've prided ourselves on not breaking the interface / contract and moving from flannel_interface
to container_interface
is a breaking change, albeit necessary one.
I'm wondering if something like cni
that takes a string of calico
or flannel
(and defaults to flannel
) makes more sense than a boolean of use_calico
. This would open the doors for more CNIs in the future and not have another boolean flag when doing so e.g.
use_calico: false
use_multus: false
etc...
The other consideration I would love to see is to create another test suite (like default) for molecule to use this CNI and run tests against it. This will ensure that 1) it works 2) future changes don't break it. It will probably be a lot of copying and pasting the way the tests are organized but I think it would be a huge benefit to making this maintainable going forward.
Again thank you! Let me your thoughts!
@timothystewart6 You're welcome! Thank you for building out and maintaining this repository. Yes, I agree. The added option should not be breaking and should remain as unobtrusive as possible. It should also be part of the CI.
I have refactored to enable based on the presence of calico_iface as a defined variable, rather than using a boolean, and cleaned up some items. I also added a new molecule scenario for calico and updated the gh actions workflow, using the existing single node scenario as the template. I have tested this locally and it seems to be checking out: calico-molecule-verify.log
The scenario could be updated to HA, but considering that essentially only k3s_server_post role is modified from default, I figured single-node was probably sufficient to test and also desirable from a system resource usage / time perspective.
I was also thinking about the future with possibility for another CNI option. My current "prod" home automation cluster is running pure routed cilium, so I would probably try cilium next, keeping the base config fairly universal and simple, while still enabling extensive customization by the user after cluster initialization. I was thinking perhaps that adding this "dynamic-ness" to the alternative CNI support could be tackled as part of the potential 3rd CNI automation build out.
Looking forward to hearing your thoughts. I'm glad to hear you were able to resolve the issues with the CI. Cheers!
@sholdee Awesome! Nice work! No breaking changes! š. Thank you! Also, thank you for the test! I will enable CI for this PR. It's pretty backlogged now but will be in the queue! That's awesome about cilium, I would love to see it! Also about CI š ... It was pretty complicated to figure out, but I am glad I stuck with it because it's running and stable now.
Looks like lint, but this can be easily fixed with pre-commit
Looks like lint, but this can be easily fixed with pre-commit
Should be good now.
Thank you @sholdee !
Proposed Changes
calico_iface
is definedWhy Calico?
Considerations
Checklist
site.yml
playbook (also tested running a second time against existing cluster)reset.yml
playbook