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

infra: enable building projects using cached images #12597

Merged
merged 6 commits into from
Nov 1, 2024

Conversation

DavidKorczynski
Copy link
Collaborator

No description provided.

@oliverchang oliverchang marked this pull request as ready for review November 1, 2024 02:58
@oliverchang
Copy link
Collaborator

@DavidKorczynski with my changes it should work in the sense that it should use the cached project image in place of building a new one.

It doesn't work as-is with chronos images though, because that expects us to invoke recompile, while our build infra explicitly runs compile.

It's probably easiest for these cached images to completely replace compile with something that supports recompiling. google/oss-fuzz-gen#682)

@DavidKorczynski
Copy link
Collaborator Author

/gcbrun skip

@DavidKorczynski
Copy link
Collaborator Author

DavidKorczynski commented Nov 1, 2024

It doesn't work as-is with chronos images though, because that expects us to invoke recompile, while our build infra explicitly runs compile.

It's probably easiest for these cached images to completely replace compile with something that supports recompiling. google/oss-fuzz-gen#682)

yeah so this is one of the issues I wasn't sure about since we had different entrypoints for all of our different replay methods #12675 (comment)

We can probably just pick one and then adjust accordingly. However, I would prefer to rely on the existing compile functionality and have that execute a perhaps build_replay.sh script (which we can copy in) instead of compiling build.sh. i.e. the only difference we have is

BUILD_CMD="bash -eux $SRC/build.sh"
will use build_replay.sh instead. We can achieve this easily with a simply sed command appended to the Dockerfile using the existing approach of chronos.

The reason I like that we use compile logic is because we use the entire build infra of OSS-Fuzz, e.g. setting up the right environment variables. it makes it also intuitive to develop the build_replay.sh scripts manually for the projects that require this.

experiment=config.experiment)
if use_caching:
# Use cached built image.
build_steps = []
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So here we make the decision that we won't be building anything in the caching run of a OFG run -- we'll rely on builds done asynchronously (as we went over here: #12675 (comment))

Just double confirming that this is the intention here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes -- we will rely on our existing infra to build chronos images daily.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@oliverchang this does not work out of the box with ccache builds. The problem here is that the existing build step is used to copy in the copied harness from OFG.

We still need to build the docker image of a given project, however, the Dockerfiles will now look something like:

FROM us-central1-docker.pkg.dev/oss-fuzz/oss-fuzz-gen/PROJECT-ofg-cached-address
COPY 01.c /target/fuzz/path.c

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think at least

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not 100% sure about this. In google/oss-fuzz-gen#692 some harnesses are not overwritten correctly, whereas others are. I'm trying to debug what's going on there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I think I know why some are succeeding: those that does not have a cached image worked correctly. The other ones did not.

Duckdb works.

However:

$ docker manifest inspect us-central1-docker.pkg.dev/oss-fuzz/oss-fuzz-gen/duckdb-ofg-cached-address
no such manifest: us-central1-docker.pkg.dev/oss-fuzz/oss-fuzz-gen/duckdb-ofg-cached-address:latest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ntpsec however does not work, but it has a cached image. See for example this harness: https://llm-exp.oss-fuzz.com/Result-reports/ofg-pr/2024-11-06-692-oc-20241106-large-generated-20241106/benchmark/output-ntpsec-_src_ntpsec_tests_libntp_decodenetnum_c/index.html

The generated harnesses does not correlate with what is in the coverage report. This is because we're missing copying the harness in for ccached containers.

@DavidKorczynski
Copy link
Collaborator Author

It doesn't work as-is with chronos images though, because that expects us to invoke recompile, while our build infra explicitly runs compile.

Let's land this now as it helps have it settle on some design decisions. If we need to adjust the entrypoint by way of here then we can do that. But as discussed elsewhere it's likely smarter to adjust chronos.

@DavidKorczynski DavidKorczynski merged commit a85eebb into master Nov 1, 2024
19 checks passed
@DavidKorczynski DavidKorczynski deleted the enable-cached-running branch November 1, 2024 20:50
@oliverchang
Copy link
Collaborator

It doesn't work as-is with chronos images though, because that expects us to invoke recompile, while our build infra explicitly runs compile.
It's probably easiest for these cached images to completely replace compile with something that supports recompiling. google/oss-fuzz-gen#682)

yeah so this is one of the issues I wasn't sure about since we had different entrypoints for all of our different replay methods #12675 (comment)

We can probably just pick one and then adjust accordingly. However, I would prefer to rely on the existing compile functionality and have that execute a perhaps build_replay.sh script (which we can copy in) instead of compiling build.sh. i.e. the only difference we have is

BUILD_CMD="bash -eux $SRC/build.sh"

will use build_replay.sh instead. We can achieve this easily with a simply sed command appended to the Dockerfile using the existing approach of chronos.
The reason I like that we use compile logic is because we use the entire build infra of OSS-Fuzz, e.g. setting up the right environment variables. it makes it also intuitive to develop the build_replay.sh scripts manually for the projects that require this.

+1 from a user's perspective, we should just be invoking compile. This makes it easily pluggable into all parts of OSS-Fuzz infra.

What it does under the hood is up to us (e.g. replacing build.sh, or completely replacing the compile script).

@DavidKorczynski
Copy link
Collaborator Author

It doesn't work as-is with chronos images though, because that expects us to invoke recompile, while our build infra explicitly runs compile.
It's probably easiest for these cached images to completely replace compile with something that supports recompiling. google/oss-fuzz-gen#682)

yeah so this is one of the issues I wasn't sure about since we had different entrypoints for all of our different replay methods #12675 (comment)
We can probably just pick one and then adjust accordingly. However, I would prefer to rely on the existing compile functionality and have that execute a perhaps build_replay.sh script (which we can copy in) instead of compiling build.sh. i.e. the only difference we have is

BUILD_CMD="bash -eux $SRC/build.sh"

will use build_replay.sh instead. We can achieve this easily with a simply sed command appended to the Dockerfile using the existing approach of chronos.
The reason I like that we use compile logic is because we use the entire build infra of OSS-Fuzz, e.g. setting up the right environment variables. it makes it also intuitive to develop the build_replay.sh scripts manually for the projects that require this.

+1 from a user's perspective, we should just be invoking compile. This makes it easily pluggable into all parts of OSS-Fuzz infra.

What it does under the hood is up to us (e.g. replacing build.sh, or completely replacing the compile script).

Sounds good, I suggest we do: https://github.com/google/oss-fuzz/pull/12608/files#diff-43e6fb06733014791deb946732e00f7aa84e62bd1d856b363e5be922573a0e0dR242-R246

In this way when the is to be run compile will call replay_build.sh which we can control by copying in the relevant script (manual or auto-generated).

@DavidKorczynski
Copy link
Collaborator Author

Actually, just think about it now we could just change:

BUILD_CMD="bash -eux $SRC/build.sh"

To:

if [ "${REPLAY_ENABLED}" = "1" ]; then
  BUILD_CMD="bash -eux $SRC/replay_build.sh"
else
  BUILD_CMD="bash -eux $SRC/build.sh"
fi 

and just avoid any use of sed and make it more explicit what happens

@oliverchang
Copy link
Collaborator

Actually, just think about it now we could just change:

BUILD_CMD="bash -eux $SRC/build.sh"

To:

if [ "${REPLAY_ENABLED}" = "1" ]; then
  BUILD_CMD="bash -eux $SRC/replay_build.sh"
else
  BUILD_CMD="bash -eux $SRC/build.sh"
fi 

and just avoid any use of sed and make it more explicit what happens

LGTM. Yes being more explicit and having less regex magic is always nicer!

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.

2 participants