src/tarball: type conversion for version dict

[JenySadadia]

Depends on kernelci/kernelci-core#2382

As the API has enforced strict int data type for some of the version information fields, data type cast is required to make sure int fields are not sent off as str fields. Otherwise, the below error will be encountered:

kernelci-pipeline-tarball | 02/16/2024 07:15:59 AM UTC [ERROR] 2 validation errors for Checkout
kernelci-pipeline-tarball | data -> kernel_revision -> version -> version
kernelci-pipeline-tarball |   value is not a valid integer (type=type_error.integer)
kernelci-pipeline-tarball | data -> kernel_revision -> version -> patchlevel
kernelci-pipeline-tarball |   value is not a valid integer (type=type_error.integer)

[JenySadadia]

Tested OK on staging along with kernelci/kernelci-core#2382 and #425:

checkout node with correct version information:

$ curl https://staging.kernelci.org:9000/latest/node/65cf23bfc3ca6699f521f44f
{"id":"65cf23bfc3ca6699f521f44f","kind":"checkout","name":"checkout","path":["checkout"],"group":null,"parent":null,"state":"done","result":null,"artifacts":{"tarball":"https://kciapistagingstorage1.file.core.windows.net/staging/linux-kernelci-staging-stable-staging-stable-20240216.1.tar.gz?sv=2022-11-02&ss=f&srt=sco&sp=r&se=2024-10-17T19:19:12Z&st=2023-10-17T11:19:12Z&spr=https&sig=sLmFlvZHXRrZsSGubsDUIvTiv%2BtzgDq6vALfkrtWnv8%3D"},"data":{"kernel_revision":{"tree":"kernelci","url":"https://github.com/kernelci/linux.git","branch":"staging-stable","commit":"0125fd7cdea3efdf4b4622046ee84fc1506c42d4","describe":"staging-stable-20240216.1","version":{"version":6,"patchlevel":1,"sublevel":77,"extra":"-1-g0125fd7cdea3e"},"patchset":null}},"debug":null,"created":"2024-02-16T08:58:39.383000","updated":"2024-02-16T09:31:29.318000","timeout":"2024-02-16T09:58:39.349000","holdoff":"2024-02-16T09:10:50.784000","owner":"staging.kernelci.org","user_groups":[]}

kver job logs:

today at 14:30:5102/16/2024 09:00:51 AM UTC [INFO] 65cf2443c3ca6699f521f452 shell shell kver 8
today at 14:31:36Getting kernel source tree...
today at 14:31:36Source directory: /tmp/kci/linux-kernelci-staging-stable-staging-stable-20240216.1
today at 14:31:36Running job...
today at 14:31:36Checking kernel revision
today at 14:31:36Result: pass

[r-c-n]

Some comments:

First: if this is urgent because we need things running and not crashing, consider this approved and go ahead with the merge.

Second: these functions and parts of tarball.py in general could (and should) be dramatically refactored sometime. The problem with this patch is that you're leaking specific information about the model definition (in this case, that the type of those three fields should be an integer) into the client code. This means that if any of those fields ever change in the model we'll likely need to update this code as well.

Proposed solutions:

1. move this model-specific type conversion as close to the model definition as possible. That is, at least in a kernelci-core lib function or, if possible, in the model class itself (although I don't think we're capable of that without a massive refactoring)
2. consider turning these fields into strings, because why not? I know you went through the trouble of enabling strict pydantic checks specifically to allow ints but, why do we want ints in the first place? A good rule of thumb is that if we'll need to make any arithmetic with these, then we want them as numbers. If not, then we're ok with storing strings for everything and that'll save us some headaches.

Then there's the tarball.py and build.py refactorings (why do we have git_describe and git_describe_verbose? why not a single function for that? why does tarball.py use both for essentially the same thing? But these have nothing to do with this PR.

[nuclearcat]

I suspect we might have filters based on version, probably making "data.kernel_revision.version.version > 6" is convinient... but this is a bit far fetching.
I expect filters to be capable define kernel versions newer than 5.10 but older than 6.1 and etc.

[r-c-n]

True, user querying and filtering can make some use of this.

[JenySadadia]

move this model-specific type conversion as close to the model definition as possible. That is, at least in a kernelci-core lib function or, if possible, in the model class itself (although I don't think we're capable of that without a massive refactoring)

I liked this idea. Yes, we already have some model-specific functions in Node model. We can have the same thing for KernelVersion as well. I'll send the changes soon.

consider turning these fields into strings, because why not? I know you went through the trouble of enabling strict pydantic checks specifically to allow ints but, why do we want ints in the first place? A good rule of thumb is that if we'll need to make any arithmetic with these, then we want them as numbers. If not, then we're ok with storing strings for everything and that'll save us some headaches.

Then what is the purpose of having different field types? In addition to the use case that Denys pointed out, another problem will be any string value can be inserted in the version information and not just numbers as strings if we define the fields as strings. And that will cause other issues in the kver job.

[r-c-n]

Then what is the purpose of having different field types? In addition to the use case that Denys pointed out, another problem will be any string value can be inserted in the version information and not just numbers as strings if we define the fields as strings. And that will cause other issues in the kver job.

My thought was not about field types in general, but about the usefulness of dividing the kernel version string into individual typed fields, when a single string + an unpacking function would do for most cases. But, as Denys pointed out (and I agree), it'll be necessary if we want to filter or query by version number and operate on that (eg. search test results for kernels higher than a certain version, etc).

[JenySadadia]

Closing this as an alternate solution got merged.