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

fix(node-installer): avoid false positive via grep process itself #152

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

vdice
Copy link
Contributor

@vdice vdice commented Jun 27, 2024

I was testing with the latest v0.15.0 node-installer image and stumbled upon the following issue.

My K8s cluster happened to be a KinD cluster and yet the node-installer script thought I was using k0s and thus failed like so:

$ k -n kwasm logs pod/kind-worker2-provision-kwasm-724lt
touch: /mnt/node-root/etc/k0s/containerd.d/spin.toml: No such file or directory

It turns out that the current logic here will exit 0 because the latter grep process itself is successfully found -- whereas it should not be counted.

For example, on my kind-worker node:

root@kind-worker:/# ps aux | grep kubelet | grep /var/lib/k0s/bin/kubelet
root        1820  0.0  0.0   3076  1280 pts/1    S+   21:13   0:00 grep /var/lib/k0s/bin/kubelet
root@kind-worker:/# echo $?
0

There are a couple ways to fix this. In this first iteration, I've elected to use pgrep which won't report its own process as an eligible result. However, I'm not 100% certain that this binary is present in all of the supported distros... (pgrep exists in the node-installer image, so we are good there)

Another option would be to tweak the logic to add an additional grep -v grep at the end of the pipe, eg ps aux | grep kubelet | grep /var/lib/k0s/bin/kubelet | grep -vq grep. Happy to go with this approach if preferred.


PS I'm still not totally sure why the similar microk8s detection method doesn't also produce a false positive. eg on my kind-worker, it exits 1 as expected:

root@kind-worker:/# ps aux | grep kubelet | grep -q snap/microk8s
root@kind-worker:/# echo $?
1

Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

lgtm.

I'm puzzled by the microk8s scenario as well.

@vdice
Copy link
Contributor Author

vdice commented Jul 1, 2024

Thanks for the review @devigned.

I'd propose this warrants a v0.15.1 release once in, even if it only represents a fix to the node-installer image and not the shim itself.

Copy link
Collaborator

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

LGTM!

@Mossaka
Copy link
Collaborator

Mossaka commented Jul 1, 2024

I'm still not totally sure why the similar microk8s detection method doesn't also produce a false positive. eg on my kind-worker, it exits 1 as expected:

The key difference between ps aux | grep kubelet | grep /var/lib/k0s/bin/kubelet and ps aux | grep kubelet | grep -q snap/microk8s is that the latter does not contain kubelet in its grep arguments.

For example, running ps aux | grep kubelet | grep /var/lib/k0s/bin/kubelet will at least run two processes:

  1. grep kubelet
  2. grep /var/lib/k0s/bin/kubelet

Then you feed the output of ps aux to grep kubelet, which will filter out the above two processes, and then pipe that to the last grep which will find itself.

However, running ps aux | grep kubelet | grep -q snap/microk8s will contain two processes that look like:

  1. grep kubelet
  2. grep snap/microk8s

The first grep kubelet will not be able to find the second grep process since the second grep does not contain the keyword "kubelet". When it's output is piped to the second grep, it will not find itself, and hence why it exits with exit code 1.

@Mossaka Mossaka merged commit 57c38e7 into spinframework:main Jul 1, 2024
9 checks passed
@vdice vdice deleted the fix/node-installer-grep branch July 1, 2024 17:58
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.

3 participants