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 killall as a subcommand #8670

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

osodracnai
Copy link
Contributor

Proposed Changes

#4181

Types of Changes

Verification

Testing

Linked Issues

User-Facing Change


Further Comments

@brandond
Copy link
Member

We need to figure out what's going on with this before updating the PR again; somehow it managed to fork-bomb on the s390x runner and take it completely out of service.

cmd/k3s/main.go Outdated Show resolved Hide resolved
pkg/cli/cmds/killall.go Outdated Show resolved Hide resolved
scripts/package-cli Outdated Show resolved Hide resolved
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

We should also clean up unnecessary/inconsistent whitespace in the script.

@dereknola dereknola self-assigned this Jan 4, 2024
@dereknola dereknola marked this pull request as ready for review February 6, 2024 20:33
@dereknola dereknola requested a review from a team as a code owner February 6, 2024 20:33
@brandond
Copy link
Member

brandond commented Feb 6, 2024

If we're going to bundle this here, do we want to remove the version that's embedded in the install script? I guess that might negatively affect folks that are still installing older releases. I just hate maintaining parallel copies.

At the very least, it feels confusing for new releases that include this as a subcommand to include two different copies that may drift. Is there some way we can skip creating the standalone killall script when installing newer k3s releases?

@dereknola
Copy link
Member

I have an idea for that, but it will need to wait for a future release. I can't test a PR commit (because #9185 is not merged), so I can't modify the install.sh to simply copy out the embedded script YET. After this PR is merged and either a release goes out with it (or the above PR commit PR gets merged) I can modify the install script.

@dereknola
Copy link
Member

dereknola commented Feb 6, 2024

But yeah the idea is that the install.sh will either:

  • Find the k3s-kill.sh script already in /var/lib/rancher/ and copy it out (in the case of a newer release its installing)
    OR
  • Pull down the latest version from master ( in the case of install an older version that does not have the script embedded)
    OR
  • generate the killall script as it currently does (in the case of airgap)

Basically we need to have some comments in the install.sh FUTURE changes to the killall script should not be placed in install.sh it will be a legacy thing for a while.

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 43.16%. Comparing base (358c4d6) to head (f079104).
Report is 185 commits behind head on master.

Files Patch % Lines
pkg/cli/cmds/killall.go 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8670      +/-   ##
==========================================
+ Coverage   43.14%   43.16%   +0.01%     
==========================================
  Files         151      155       +4     
  Lines       13342    13389      +47     
==========================================
+ Hits         5757     5779      +22     
+ Misses       6498     6447      -51     
- Partials     1087     1163      +76     
Flag Coverage Δ
e2etests ?
inttests 39.75% <0.00%> (?)
unittests 15.91% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@warmwaffles
Copy link

What is the status of this? The kill all script is no where to be found in this repository and it's not clear where I should get it. I can't have k3s leaving tons of container shims and stuff laying around on my machine.

@brandond
Copy link
Member

The kill all script is no where to be found in this repository and it's not clear where I should get it

It's built into the install script. Run the install script to install K3s and you will also get the killall and uninstall scripts.

@warmwaffles
Copy link

It's built into the install script. Run the install script to install K3s and you will also get the killall and uninstall scripts.

I figured that out about an hour after posting that comment. I ended up just copying it out. Unfortunately with k3s-bin in the Arch User Repository it just grabs the binary and puts it in place without much fanfare. Perhaps I need to just install it instead of relying on the AUR.

In the mean time I solved my issues with that script.

@brandond
Copy link
Member

We don't support the Arch package. If they're going to build and package it their own way, they should also include the utility scripts.

@warmwaffles
Copy link

warmwaffles commented May 17, 2024

We don't support the Arch package. If they're going to build and package it their own way, they should also include the utility scripts.

Yes, and that is apparent since it is in the AUR not the official repo. It's just sad that the killall script that is referenced in the help docs is not easy to find.

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.

4 participants