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

Update k8s/cri-o repositories #94

Closed
wants to merge 1 commit into from
Closed

Conversation

jorhett
Copy link

@jorhett jorhett commented May 14, 2024

Pull Request (PR) description

  • Remove obsolete path fixes that don't exist in any version of crio packages I can find
  • Remove conditionals around long-obsolete/unsupported versions
  • Allow use of the deprecated kubic repos, default to official k8s packages

This Pull Request (PR) fixes the following issues

Fixes #77
Fixes #47 (for Ubuntu)

Some comments

I included the ability to use the kubic repos for existing implementations (with a value change) based on this comment #77 (comment) however given the number of changes around k8s repos since 1.28 I personally think it would be better to make a major version cut and simply say that it drops support for the kubic repos which are deprecated IMHO YMMV

Obviously this PR only addresses Debian. I didn't want to invest the time until we have some consensus on the approach, at which time I'll update the PR to fix the repos for all platforms.

@@ -8,16 +8,13 @@
Boolean $manage_container_manager = $k8s::manage_container_manager,
K8s::Container_runtimes $container_manager = $k8s::container_manager,
String[1] $crio_version = $k8s::version.split('\.')[0, 2].join('.'),
Boolean $use_kubic_repos = false
Copy link
Member

Choose a reason for hiding this comment

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

please document the new parameter with puppet-strings

Copy link
Author

@jorhett jorhett May 14, 2024

Choose a reason for hiding this comment

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

Happy to do all the tidy bits once I get consensus that this is an approach ya'all agree with. If I can get pre-approval that going this direction is agreeable (dropping all obsolete k8s version support, etc) I'm happy to invest a few hours making it perfect.

So this is only intended to hit the target of "should work for you if you want to test it" and "something for us to discuss"

Copy link
Member

Choose a reason for hiding this comment

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

Personally not against dropping kubic entirely, optionally even with a catalog fail condition on Kubernetes <1.24 versions, since those versions are long since deprecated.
Unfortunately the migration from kubic to the new isv:kubernetes repos can be difficult to automate, since some packages have migrated concerns between them.

On Debian, a complete uninstall of cri-o and conmon is necessary before the isv:kubernetes cri-o package can be installed - as one example.

@bastelfreak
Copy link
Member

thanks for the PR, can you add some unit tests to verify the new behaviour?

Remove path fixes that don't exist in any version of crio packages I can find
Remove conditionals around long-obsolete/unsupported versions
Allow use of the deprecated kubic repos, default to official k8s packages
@jorhett jorhett force-pushed the fix-debian-cri-o-install branch from 84f9347 to 234a796 Compare May 14, 2024 07:29
@ananace
Copy link
Member

ananace commented May 14, 2024

The k8s::install::cni_plugins code will probably need updating as well, the k8s repo looks to be calling the package kubernetes-cni

@jorhett jorhett changed the title Fix debian cri o install Update k8s/cri-o repositories May 14, 2024
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.

Have finally the chance to sit down and test things with the new repos, found a few issues.

} else {
$kubernetes_name = 'kubernetes'
$kubernetes_repo = "https://pkgs.k8s.io/core:/stable:/v${crio_version}/deb"
$kubernetes_key = 'kubernetes-apt-keyring.gpg'
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be kubernetes-apt-keyring.asc

$kubernetes_key = 'kubernetes-apt-keyring.gpg'
$crio_name = 'cri-o'
$crio_url = "https://pkgs.k8s.io/addons:/cri-o:/stable:/v${crio_version}/deb"
$crio_key = 'cri-o-apt-keyring.gpg'
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be cri-o-apt-keyring.asc

@@ -8,16 +8,13 @@
Boolean $manage_container_manager = $k8s::manage_container_manager,
K8s::Container_runtimes $container_manager = $k8s::container_manager,
String[1] $crio_version = $k8s::version.split('\.')[0, 2].join('.'),
Boolean $use_kubic_repos = false
Copy link
Member

Choose a reason for hiding this comment

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

Personally not against dropping kubic entirely, optionally even with a catalog fail condition on Kubernetes <1.24 versions, since those versions are long since deprecated.
Unfortunately the migration from kubic to the new isv:kubernetes repos can be difficult to automate, since some packages have migrated concerns between them.

On Debian, a complete uninstall of cri-o and conmon is necessary before the isv:kubernetes cri-o package can be installed - as one example.

@ananace
Copy link
Member

ananace commented Sep 24, 2024

Superseded by #103

@ananace ananace closed this Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants