Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add features #24

Merged
merged 8 commits into from
Mar 28, 2023
Merged

add features #24

merged 8 commits into from
Mar 28, 2023

Conversation

rwaffen
Copy link
Sponsor Member

@rwaffen rwaffen commented Mar 20, 2023

I hope this I not to much. what do you think about it?

  • I wanted to have a simple working network thats why I added the simple_cni class to have a bridged network to start with. Then I went on and tried to install cilium, but this is in a early state.
  • I added crictl to debug containers on notes. so I can do crictl ps and stuff. In the past we did this with docker, but we don't have docker now anymore.
  • I added containerd because I wanted to use this as cri implementation.
  • I moved cni_plugins into an own class
  • I moved the cri section from the main class into an own module

- prepare for cilium
- add crictl debugging tool
- add containerd as cri option
- add simple bridged cni as alternative to flannel and cilium
- add more doku
- restructure classes
Copy link
Member

@ananace ananace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got to do some more digging in various documentation, but I have some questions so far.

I'm still unsure about if the module should even attempt to manage more complex solutions - the resources that it currently offers for networking and the like are mainly there for solving the MVP part, with more complex choices (like cilium, calico, kube-router, linkerd, etc) being up to the admin to manage.

files/containerd/config.toml Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
manifests/install/cilium.pp Outdated Show resolved Hide resolved
manifests/node/simple_cni.pp Outdated Show resolved Hide resolved
manifests/node.pp Show resolved Hide resolved
manifests/install/cni_plugins.pp Outdated Show resolved Hide resolved
types/container_runtimes.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
manifests/server/resources.pp Outdated Show resolved Hide resolved
manifests/install/cilium.pp Outdated Show resolved Hide resolved
- simplify containerd config
- make if statement os indipendent
- remove docker support as CRI
- document simple_cni
@rwaffen
Copy link
Sponsor Member Author

rwaffen commented Mar 27, 2023

@SimonHoenscheid @ananace

how do we proceed here? should i remove cilium for the moment? i really would need that containerd part atm. (i'm using my fork right now, but would like to have it upstream :) )

@ananace
Copy link
Member

ananace commented Mar 27, 2023

My personal feeling is that offering more network implementation isn't necessarily a bad thing, but using a tool to 'install' them into the cluster as a non-idempotent exec could lead to far too many issues far too easily.
Might be better to split out the cilium pieces into a separate PR for now, it's definitely the component that's going to require the most design/thought.

- will be added again later
- simplify pr
Copy link
Member

@ananace ananace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got another couple of nitpicks about ensure not being used properly all the way, but nothing that should stop this PR from being merged.

@rwaffen
Copy link
Sponsor Member Author

rwaffen commented Mar 27, 2023

then lets talk. i myself are not sure what to do with the ensure. sometimes its on $ensure, but i also saw here and in other modules that it is now better to use something like stdlib::ensure($ensure, 'package'),?

what your opinion on that?

@ananace
Copy link
Member

ananace commented Mar 27, 2023

@rwaffen The stdlib::ensure function exists in order to support mapping the regular ensure values (absent/present) into type-specific ones (absent/ìnstalled for packages, stopped/running for services, absent/link for symlink files, etc)

If the type uses a regular absent/present, then passing $ensure directly is the best thing to do, but for specialized types like packages/services/files/directories/links/etc then using stdlib::ensure is the correct thing to do.

In the case of this PR I mainly meant that there are classes where $ensure is a parameter, but it's only applied to some of the resources. k8s::install::cni_plugins for example, where it's only applied onto the archive resource.
Amusingly enough, I find myself actually more okay with e.g. k8s::install::container_runtime in this nitpick, since there's no ensure at all on it.

Regardless, it's a nitpick that shouldn't block the PR further since the module itself isn't doing it right across the line either.

@rwaffen
Copy link
Sponsor Member Author

rwaffen commented Mar 27, 2023

okay, now i understand. 😄

@rwaffen rwaffen merged commit daf5586 into voxpupuli:master Mar 28, 2023
@rwaffen rwaffen deleted the patching branch March 28, 2023 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants