Coverage decreased (-0.06%) to 36.934% when pulling bdad446 on kkourt:valuetester into c4b2c1e on cilium:master.

test-me-please

test-me-please

test-me-please

Commit 6c8907e8b195748c2a23e3a7b519db9badf95e71 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

Commits 6c8907e8b195748c2a23e3a7b519db9badf95e71, ee15973bc54aafe669e332469aae4ee2f8f7265a do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

test-me-please

test-me-please

test-me-please

Latest version adds a check for the loadbalancer, a minor checkpatch fix, and removes the object file for the constchecker self-test.

@qmonnet I added a small checkpatch change here so that it does not complain about using inline comments for SPDX, since using SPDX seems to be the preferred way (#12436 (comment)).

I later noticed that you had sent a (more complete) patch upstream for this issue (https://lore.kernel.org/patchwork/patch/1265784/#1461076).

Not sure what the proper thing to do here, but it's a bit confusing to get conflicting checkpatch errors, so maybe we should carry the SPDX change locally regardless of whether it's upstreamed?

Not sure what the proper thing to do here, but it's a bit confusing to get conflicting checkpatch errors, so maybe we should carry the SPDX change locally regardless of whether it's upstreamed?

(Thanks for asking!) I agree this is confusing. I was hoping the patch would be merged upstream, but it didn't get a warm reception and now this seems unlikely to happen. We discussed that during a datapath meeting (last week maybe), and we agreed on the following: There is some follow-up I intend to do on checkpatch, to package it as a Docker image instead of keeping it in the Cilium repo (on Ilya's suggestion). Once we have that, we can apply local patches (patch -p1 < ...) when building the image: this way we keep the mainline version from Linux repo, making it easy to update, but apply the desired local changes before using it.

In the meantime, I would as well keep the current version untouched. Unless you feel this is too much confusing and really prefer to patch, in which case we can start changing checkpatch.pl now and then report the changes later when packaging it as an image.

I opened #12467 about packaging checkpatch.

In the meantime, I would as well keep the current version untouched. Unless you feel this is too much confusing and really prefer to patch, in which case we can start changing checkpatch.pl now and then report the changes later when packaging it as an image.

As long as people don't mind the red checkmark in BPF checks, it's fine by me :)
I will remove the patch from this PR.

As a side note, there is also:

+++ b/contrib/checkpatch/checkpatch.sh
@@ -23,6 +23,8 @@ ignore_list=(
     LONG_LINE_COMMENT
     # Ignore tolerance that comes by default
     C99_COMMENT_TOLERANCE
+    # We do not have a MAINTAINERS file, so we can ignore this
+    FILE_PATH_CHANGES
 )

This seems like something that can be applied directly (or maybe not?)

We could either ignore the FILE_PATH_CHANGES reports as you suggest or patch it in the future to say CODEOWNERS (that we do have) instead of MAINTAINERS. Either is fine by me.

test-me-please

rebased

test-me-please

test-me-please

This seems like a reasonable way to perform such checks. At a high level, I wonder whether we would benefit from moving more towards a model where the Golang implementation generates all of these constants from the Golang definition into a header file instead, then the BPF code simply includes those headers. That way, the constants would be ensured to be the same without having to perform additional checks afterwards.

I do think that the approach in this PR is problematic in the sense that it's too much work for a developer to add new checks. They would have to edit two (go test, bpf file with constants) or more (test script) files, and probably would end up just adding a comment instead. And, if people are not going to use it, I'm not sure if it's worth merging.

Generating constants from Go would be better in that sense, but I'm also not sure if it's worth doing it, since it adds some level of complexity, and I do like the fact that the bpf code is self-contained.

Also, I'm not sure if constants checks solve a real problem, i.e., having a chance to catch actual bugs. Maybe a comment in Go and BPF side that these constants should match is the best solution at this point.

test-me-please

retest-net-next

Generating constants from Go would be better in that sense, but I'm also not sure if it's worth doing it, since it adds some level of complexity, and I do like the fact that the bpf code is self-contained.

In practice we're already doing this via the headerfiles for ep_config.h, netdev_config and node_config.h so it's very much in line with our existing templating approaches.

Also, I'm not sure if constants checks solve a real problem, i.e., having a chance to catch actual bugs. Maybe a comment in Go and BPF side that these constants should match is the best solution at this point.

It's always possible, but given that both (Go/C) constants are usually introduced at the same time it's typically fairly easy to catch mistakes. I can't specifically recall hitting an issue like this in the past with Cilium.

Unless someone suggests differently, I would opt to drop this one. (I know it's my PR and I'm the one supposed to argue for it, but as I said I think it's too much hassle for people to use it.) We can always resurrect it if we think it makes sense.