liske / needrestart

Restart daemons after library updates.
GNU General Public License v2.0
420 stars 67 forks source link

Fix AMD Microcode Check #285

Closed fritz-fritz closed 1 month ago

fritz-fritz commented 10 months ago

As discussed in #249, the default behavior for AMD was changed with pull request #226 and as a result, CPUs that do not have microcode patches (because they're up to date) were leading to NRM_UNKNOWN resulting in "Failed to check for processor microcode upgrades." which is inaccurate.

This sets the $vars{AVAIL} to 0 if there are microcode patches to scan and none apply. While this doesn't guarantee microcode is up to date or from a valid source, it restores the previous behavior while accounting for the edge case presented in the previous pull request.

fritz-fritz commented 10 months ago

This should address some more of #274 but also might want to look into expected behavior for Intel as well?

fritz-fritz commented 10 months ago

@liske would be great to have some feedback

anarcat commented 10 months ago

@fritz-fritz why is this marked as draft, do you plan to get anything else done on this PR? otherwise i suggest just marking it as ready. i'll test this on our fleet now.

fritz-fritz commented 10 months ago

@fritz-fritz why is this marked as draft

@anarcat mostly just to try and encourage feedback before merging. I believe I have implemented the expected behavior here but am willing to rework it if need be. When all looks good I’ll be happy to finalize the PR

anarcat commented 10 months ago

the patch doesn't fix the issue for us. before the patch:

root@tb-build-03:~# needrestart -p
UNKN - Kernel: 6.1.0-13-amd64, Microcode: unknown, Services: 1 (!), Containers: none, Sessions: 1 (!!)|Kernel=0;0;;0;2 Services=1;;0;0 Containers=0;;0;0 Sessions=1;0;;0
Services:
- systemd-logind.service
Sessions:
- redacted @ user manager service

after the patch:

root@tb-build-03:~# dpkg -i needrestart_3.6-4+deb12u1_all.deb 
(Reading database ... 110354 files and directories currently installed.)
Preparing to unpack needrestart_3.6-4+deb12u1_all.deb ...
Unpacking needrestart (3.6-4+deb12u1) over (3.6-4) ...
Setting up needrestart (3.6-4+deb12u1) ...
Processing triggers for man-db (2.11.2-2) ...
root@tb-build-03:~# needrestart -p
UNKN - Kernel: 6.1.0-13-amd64, Microcode: unknown, Services: 1 (!), Containers: none, Sessions: 1 (!!)|Kernel=0;0;;0;2 Services=1;;0;0 Containers=0;;0;0 Sessions=1;0;;0
Services:
- systemd-logind.service
Sessions:
- redacted @ user manager service
fritz-fritz commented 10 months ago

@anarcat can you try it with -w -v I’d like to get better visibility at what it sees when ran. Also to note, this PR doesn’t fix the intel microcode checking on AMD I have that in a separate PR. So that could be the issue?

anarcat commented 10 months ago

-v always succeeds, that's the whole problem here. I think i found (and fixed) the root cause, try #288.

anarcat commented 10 months ago

edit: moved this comment into the issue thread instead, in https://github.com/liske/needrestart/issues/249#issuecomment-181326728

anarcat commented 1 month ago

i think this should be closed in favor of #288 and #290, and in any case would require a rebase.

fritz-fritz commented 1 month ago

@anarcat I admittedly have been away from this code for a while, but a quick review of your PRs and I think I agree.

Closing this PR.