puppetlabs-kubernetes icon indicating copy to clipboard operation
puppetlabs-kubernetes copied to clipboard

Make sure kubelet gets restarted after package version change

Open danifr opened this issue 3 years ago • 22 comments

Fix https://github.com/puppetlabs/puppetlabs-kubernetes/issues/524

danifr avatar Jul 09 '21 14:07 danifr

kubernetes::packages is a class

that may have no external impact to Forge modules.

kubernetes::service is a class

that may have no external impact to Forge modules.

This module is declared in 0 of 576 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

Hmm I am not sure why the tests failed.

I am doing something similar to: https://github.com/puppetlabs/puppetlabs-kubernetes/blob/main/manifests/packages.pp#L357 (referencing a Service declared in the service.pp class)

I someone can give me a hint I'd appreciate it.

danifr avatar Jul 09 '21 15:07 danifr

Well, I don't know what I can do to make the tests pass... I tried a few different approaches but I always get an error. I wonder if the tests are OK.

Just so you know, same patch, running on my infra, works as expected (kubelet services gets restarted after a package update):

Notice: /Stage[main]/Kubernetes::Packages/Package[kubelet]/ensure: ensure changed '1.20.8-0' to '1.21.0' (corrective)
Info: /Stage[main]/Kubernetes::Packages/Package[kubelet]: Scheduling refresh of Service[kubelet]
Notice: /Stage[main]/Kubernetes::Packages/Package[kubectl]/ensure: ensure changed '1.20.8-0' to '1.21.0' (corrective)
Notice: /Stage[main]/Kubernetes::Packages/Package[kubeadm]/ensure: ensure changed '1.20.8-0' to '1.21.0' (corrective)

@daianamezdrea can you please review it? Thanks

danifr avatar Jul 12 '21 14:07 danifr

Hi @danifr, thank you ! Yes, I'll have a look, I'll try to debug it locally and see what is not working fine

daianamezdrea avatar Jul 13 '21 07:07 daianamezdrea

Hi @danifr, I pushed some changes to see if the specs are going to pass

daianamezdrea avatar Aug 10 '21 07:08 daianamezdrea

@daianamezdrea OMG thanks a lot!!!

danifr avatar Aug 10 '21 07:08 danifr

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 01 '21 18:09 CLAassistant

Hello again, can we get this change merged and have it available within the next release? Thank you!

danifr avatar Sep 21 '21 12:09 danifr

@daianamezdrea Hi! sorry for insisting, did you have some time to look at this? I think we can safely merge it into main

danifr avatar Sep 28 '21 13:09 danifr

Hi @danifr, I required @carabasdaniel review on the changes I've made and he said that the dependencies added transform the unit tests in acceptance (with the changes I did) so I need to re-think the issue, sorry for the late response, looking now again how to approach it

daianamezdrea avatar Oct 11 '21 08:10 daianamezdrea

Hi @daianamezdrea happy new year! is there something I can do to have this merged?

danifr avatar Jan 03 '22 11:01 danifr

This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

github-actions[bot] avatar May 09 '22 02:05 github-actions[bot]

Is there something else I can do so we can get this one merged?

danifr avatar May 09 '22 07:05 danifr

kubernetes::packages is a class

that may have no external impact to Forge modules.

kubernetes::service is a class

that may have no external impact to Forge modules.

This module is declared in 0 of 579 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

Looks like there were some failing tests on the last run.

I've re kicked them so we can get a fresh view of what is happening.

chelnak avatar May 09 '22 10:05 chelnak

kubernetes::packages is a class

that may have no external impact to Forge modules.

kubernetes::service is a class

that may have no external impact to Forge modules.

This module is declared in 0 of 579 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

Looks like we have some conflicts. Could you rebase on current main please?

chelnak avatar May 09 '22 10:05 chelnak

Hello! 👋

This pull request has been open for a while and has had no recent activity. We've labelled it with attention-needed so that we can get a clear view of which PRs need our attention.

If you are waiting on a response from us we will try and address your comments on a future Community Day.

Alternatively, if it is no longer relevant to you please close the PR with a comment.

Please note that if a pull request receives no update for 7 after it has been labelled, it will be closed. We are always happy to re-open pull request if they have been closed in error.

github-actions[bot] avatar Jul 09 '22 02:07 github-actions[bot]

Hi @chelnak, sorry I missed you comment. I just rebased from main and the tests are still failing...

I don't understand why though... the only relevant change that this PR introduces is:

https://github.com/puppetlabs/puppetlabs-kubernetes/pull/525/files#diff-98b7734749457d0f7b07bfccf04f7316e31a48cdf3e02a46cb3cb8209daecb29R128

Maybe you know what's going on.

danifr avatar Jul 10 '22 17:07 danifr

@danifr For failing tests see #565

deric avatar Jul 19 '22 14:07 deric

Still failing... I will try removing @daianamezdrea commit from this PR. Let's see

danifr avatar Jul 27 '22 17:07 danifr

Blows my mind that is fails with:

# Puppet::Parser::Compiler::CatalogValidationError:
#   Could not find resource 'Service[kubectl]' in parameter 'notify' (file: /home/runner/work/puppetlabs-kubernetes/puppetlabs-kubernetes/spec/fixtures/modules/kubernetes/manifests/packages.pp, line: 368)

Because of: https://github.com/puppetlabs/puppetlabs-kubernetes/pull/525/files#diff-7395f2f3aeb84af1465802a40aed5e8226f466887bccbf9a8e4599ea73f50964R386

But it does not say anything about: https://github.com/puppetlabs/puppetlabs-kubernetes/blob/main/manifests/packages.pp#L357 that is basically the same thing.

danifr avatar Jul 27 '22 17:07 danifr

@danifr looks like there are some merge conflicts. Could you resolve these so we can assess how to proceed with this.

jordanbreen28 avatar Sep 26 '22 13:09 jordanbreen28

I just did it. Thanks for the heads-up :)

danifr avatar Sep 29 '22 09:09 danifr

hey @danifr. I pulled your PR and ran the tests locally. The tests fail on your branch but pass on the master branch so the failures are due to your changes.

The failing tests complain about missing k8s-related files e.g. expected to contain File[/etc/systemd/system/kubelet.service.d/20-cloud.conf] with content =~ /--cloud-provider=openstack/ etc. This may be a good starting point.

Hopefully this helps!

GSPatton avatar Oct 10 '22 14:10 GSPatton

@GSPatton sorry did not have time to work on this. I can open a new PR in the future if I managed to get it working.

I need to figure out how to run the test locally, so I can make sure the test pass before opening a PR. Do you have some docs or info on how to run them locally?

danifr avatar Oct 24 '22 10:10 danifr