Improve server logic #235
Improve server logic #235
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
nadzyah
force-pushed
the
improve-server-logic
branch
from
e6717f2 to
89e4b11
Compare
|
Reading the PR description, I wonder if it's even necessary to match BIOS when checking if a hardware is certified. As in does the BIOS used affect whether Ubuntu would run fine or not? |
| logging.error( | ||
| ( | ||
| "Hardware cannot be found. Machine vendor: %s, model: %s" | ||
| ", board model: %s, board version: %s, bios version: %s" | ||
| ), | ||
| system_info.vendor, | ||
| system_info.model, | ||
| system_info.board.product_name, | ||
| system_info.board.version, | ||
| system_info.bios.version if system_info.bios else None, | ||
| ) |
There was a problem hiding this comment.
This endpoint checks whether a hardware is certified or not. If it's not certified then it responds with NotCertifiedResponse. If so then it is expected behavior to find not certified device where the board doesn't match for instance. Hence it's not an error. So perhaps it's not correct to log an error here. Additionally, this is the API any logs made here are not shown to the client so we shouldn't be logging anything apart from real errors, warnings or perhaps debug info.
There was a problem hiding this comment.
Matias specifically asked for logging hardware mismatch data since we'll probably have to adjust the logic according to, for instance, how many certified machines run a newer bios version, different from which we used for issuing a certificate
There was a problem hiding this comment.
This sounds like we need to store this data to be able to query it rather than just log it. I'm not sure if simply logging will help us get such information. @mz2 wdyt?
There was a problem hiding this comment.
Fair point, it probably is better that way. I was imagining deriving metrics from logs, but explicitly storing the full data is probably a better idea.
There was a problem hiding this comment.
After discussing it with @omar-selo, we've decided to add this change in another PR and create a table for storing hardware mismatches
Here is the Jira ticket for it: https://warthogs.atlassian.net/browse/C3-945
| assert "Hardware cannot be found" in caplog.text | ||
| assert "bios version: 1.0.15" in caplog.text |
There was a problem hiding this comment.
I see that the logs are being used to test that the reason hardware is "Not Seen" is what's expected of this test. It's great that we're testing this. But I suggest that instead of depending on logs, we test it directly. This can be done by refactoring the logic that checks if hardware is certified in it's own function. This function would be used by the controller. It can return not only the status but also the reason. Then we can write tests for that function to make sure the reason matches what we expected.
There was a problem hiding this comment.
Matias asked to check that if there is a hardware mismatch, we log this information
nadzyah
force-pushed
the
improve-server-logic
branch
from
f0bd6c5 to
4969058
Compare
nadzyah
force-pushed
the
improve-server-logic
branch
from
ce66ff9 to
57ce302
Compare
…hile looking up the data in the DB
nadzyah
force-pushed
the
improve-server-logic
branch
from
57ce302 to
70a7a44
Compare
…ake tests more readable
This PR updates the server logic after testing it on certified machines using scripts in #234. The patches contain the following fixes:
first(). Since we match only bios vendor and version, there might be multiple BIOSes with different revisions but the same version. One of them may belong to the matching machine. So, now we use all matching BIOSes instead of getting the first one. Additionally, we don't match other fields likerevisionsince firmware may receive revision updates pretty frequently, but it still allows us to consider hardware as potentially certified since we're still working with the same BIOS versionlsb_releasecommand used by the client to collect the OS version returns the string in format22.04without theLTSsuffix. However, C3 stores it with theLTSsuffix. The logic was added to remove this suffix while importing the data from C3Also, the tests were revised/added to cover the implemented logic