Make CAS and AC server not to use the same store #1175

aleksdmladenovic · 2024-07-19T04:16:58Z

Description

When CAS and AC uses the same store, it might produce unexpected side-effects and wrong behaviors. To prevent system from such situation, it currently raises an error when CAS and AC uses the same store.

Fixes #768

Type of change

Please delete options that aren't relevant.

Bug fix (non-breaking change which fixes an issue)
This change requires a documentation update

How Has This Been Tested?

Please also list any relevant details for your test configuration

Checklist

Updated documentation if needed
Tests added/amended
bazel test //... passes locally
PR is contained in a single commit, using git amend see some docs

This change is

aleksdmladenovic · 2024-07-19T04:17:30Z

Hope you to review this. cc: @allada , @MarcusSorealheis

allada

Reviewable status: 0 of 1 LGTMs obtained, and 2 discussions need to be resolved

a discussion (no related file):
We should also create an integration test for this.

src/bin/nativelink.rs line 230 at r1 (raw file):

        // Check if CAS and AC uses the same store.
        if let Some(cas_config) = &services.cas {

This is O(N^2). Can we instead create a hashmap, of one of them then index into the other to check? This will make it O(max(N, M)) which is literally exponentially more efficient.

MarcusSorealheis

Reviewable status: 0 of 1 LGTMs obtained, and 2 discussions need to be resolved

a discussion (no related file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

We should also create an integration test for this.

+1

aleksdmladenovic

Reviewable status: 0 of 1 LGTMs obtained, and 2 discussions need to be resolved

a discussion (no related file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

+1

I will add integration test in mins.

src/bin/nativelink.rs line 230 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

This is O(N^2). Can we instead create a hashmap, of one of them then index into the other to check? This will make it O(max(N, M)) which is literally exponentially more efficient.

My algorithm is already O(N * logM).
I do not agree to this.
Well, fyi, cas_config and ac_config are already hashmaps here.
There is only one for loop here for looping through the hashmap cas_config.
And then ac_config.get(instance_name) - this part is O(logM) because ac_config is already HashMap.
Since we have hashmaps by default there's no need to create a new hashmap or something.

Hope you to check the code again. cc: @allada

aleksdmladenovic

Reviewable status: 0 of 1 LGTMs obtained, and 2 discussions need to be resolved

a discussion (no related file):

Previously, aleksdmladenovic (Aleksandar Mladenovic) wrote…

I will add integration test in mins.

cc: @allada , @MarcusSorealheis

MarcusSorealheis · 2024-07-21T17:27:52Z

Oh wait, @aleksdmladenovic you're already working on this issue. I'll check it now.

MarcusSorealheis

We need a test, for sure. All you need to do is include a config JSON where the AC and CAS use the same store type.

Reviewable status: 0 of 1 LGTMs obtained, and 2 discussions need to be resolved

CLAassistant · 2024-07-22T18:15:38Z

All committers have signed the CLA.

aleksdmladenovic

I added an integration test for this. cc: @allada , @MarcusSorealheis

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 2 discussions need to be resolved

MarcusSorealheis

There is some weird failure not related to the PR, I believe, at first glance. It was caused by some permissions issue with Tekton so I re-ran the commit. We should not be able to fail due to Tekton cc @aaronmondal in case it happens again.

I'll dig into the test now.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, integration-tests (20.04), integration-tests (22.04), pre-commit-checks, and 2 discussions need to be resolved

a discussion (no related file):

Previously, aleksdmladenovic (Aleksandar Mladenovic) wrote…

cc: @allada , @MarcusSorealheis

Needs test.

MarcusSorealheis

You also have some pre-commit issues to work out. Probably something like trailing whitespace.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, integration-tests (20.04), integration-tests (22.04), pre-commit-checks, and 2 discussions need to be resolved

aleksdmladenovic

Done. @MarcusSorealheis

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 2 discussions need to be resolved

a discussion (no related file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

Needs test.

Done. cc: @allada , @MarcusSorealheis

aleksdmladenovic

I've made it pass all CI/CD. FYI, I've installed nix environment locally and finally it went through all pre-commit checks.
For my further commits, it won't fall into pre-commit issues again. :)

Waiting for your feedback. cc: @allada, @MarcusSorealheis

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Publish nativelink-worker-init, Remote / large-ubuntu-22.04, and 2 discussions need to be resolved

allada

Reviewable status: 0 of 1 LGTMs obtained, and 3 discussions need to be resolved

integration_tests/store_conflict_test.sh line 26 at r4 (raw file):

EXIT_CODE=0
sudo docker compose --file docker-compose.test.yml up -d

@aaronmondal , this feels like the wrong way to use docker for integration tests. Do you see any low hanging fruit on how to make this less hacky?

aaronmondal

Reviewable status: 0 of 1 LGTMs obtained, and 3 discussions need to be resolved

integration_tests/store_conflict_test.sh line 26 at r4 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

@aaronmondal , this feels like the wrong way to use docker for integration tests. Do you see any low hanging fruit on how to make this less hacky?

First I thought no, but actually yes!

For things that only need to test specific config options we can start the executable directly without wrapping it in Docker. The nativelink-is-installable test does this:

The test: https://github.com/TraceMachina/nativelink/blob/main/tools/nativelink-is-executable-test.nix (you could create the nativelink config inline in the script).
Import it in the flake:

nativelink/flake.nix

Line 142 in 3574149

nativelink-is-executable-test = import ./tools/nativelink-is-executable-test.nix {inherit pkgs nativelink;};
Export it as flake output:

nativelink/flake.nix

Line 232 in 3574149

inherit publish-ghcr local-image-test nativelink-is-executable-test nativelink nativelink-debug native-cli lre-cc nativelink-worker-init;
Add it as a github action or in this case just add it as a step above the step here so it can reuse the cache:

nativelink/.github/workflows/nix.yaml

Line 135 in 3574149

- name: Test nix run

MarcusSorealheis · 2024-07-25T01:14:54Z

Magicians.

aleksdmladenovic

Reviewable status: 0 of 1 LGTMs obtained, and 3 discussions need to be resolved

integration_tests/store_conflict_test.sh line 26 at r4 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

First I thought no, but actually yes!

For things that only need to test specific config options we can start the executable directly without wrapping it in Docker. The nativelink-is-installable test does this:

The test: https://github.com/TraceMachina/nativelink/blob/main/tools/nativelink-is-executable-test.nix (you could create the nativelink config inline in the script).

Import it in the flake:

nativelink/flake.nix

Line 142 in 3574149

nativelink-is-executable-test = import ./tools/nativelink-is-executable-test.nix {inherit pkgs nativelink;};

Export it as flake output:

nativelink/flake.nix

Line 232 in 3574149

inherit publish-ghcr local-image-test nativelink-is-executable-test nativelink nativelink-debug native-cli lre-cc nativelink-worker-init;

Add it as a github action or in this case just add it as a step above the step here so it can reuse the cache:

nativelink/.github/workflows/nix.yaml

Line 135 in 3574149

- name: Test nix run

Thanks. @aaronmondal.

Let me try that way. :)

aleksdmladenovic

Hi, I added an integration test for checking this feature in a non-hacky way using nix and github nix workflow in a way as @aaronmondal has mentioned. :)

I hope you to check this and work on other issues.

cc: @aaronmondal . @allada , @MarcusSorealheis

Reviewable status: 0 of 2 LGTMs obtained, and 0 of 6 files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 3 discussions need to be resolved

MarcusSorealheis · 2024-08-03T15:05:00Z

This feature is something we'd ideally like to get into the next release.

MarcusSorealheis

Reviewable status: 0 of 2 LGTMs obtained, and 0 of 6 files reviewed, and 5 discussions need to be resolved

src/bin/nativelink.rs line 254 at r5 (raw file):

                            return Err(make_err!(
                                Code::InvalidArgument,
                                "CAS and AC use the same store '{}' in the config",

The error text here should be:

"CAS and AC cannot use the same store '{}' in the config"

It's open question for @bclark8923 and @adam-singer as to whether or not we should also link to the documentation on this issue.

tools/nativelink-prevent-store-conflict-test.nix line 62 at r5 (raw file):

  print_error_output=$(cat <<EOF
  Error: Error { code: InvalidArgument, messages: ["CAS and AC use the same store 'FILESYSTEM_STORE' in the config"] }

again, we should be clear that users cannot use

aleksdmladenovic

Addressed your changes. cc: @MarcusSorealheis, @allada

Reviewable status: 0 of 2 LGTMs obtained, and 0 of 6 files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 5 discussions need to be resolved

src/bin/nativelink.rs line 254 at r5 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

The error text here should be:

"CAS and AC cannot use the same store '{}' in the config"

It's open question for @bclark8923 and @adam-singer as to whether or not we should also link to the documentation on this issue.

Done.

tools/nativelink-prevent-store-conflict-test.nix line 62 at r5 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

again, we should be clear that users cannot use

Done.

aleksdmladenovic · 2024-08-08T13:47:35Z

Rebased. cc: @MarcusSorealheis

aleksdmladenovic · 2024-08-09T09:29:45Z

Hope you to merge this. cc: @allada, @MarcusSorealheis, @aaronmondal

When CAS and AC uses the same store, it might produce unexpected side-effects and wrong behaviors. To prevent system from such situation, it currently raises an error when CAS and AC uses the same store. An integration test is added for this using nix test file and github's nix workflow.

aleksdmladenovic · 2024-08-16T14:11:00Z

Rebased and handled conflicts.

Hope you to check this. cc: @MarcusSorealheis, @allada, @aaronmondal

MarcusSorealheis · 2024-12-29T23:03:52Z

#1555 obviates this PR

allada requested changes Jul 19, 2024

View reviewed changes

MarcusSorealheis reviewed Jul 19, 2024

View reviewed changes

aleksdmladenovic commented Jul 20, 2024

View reviewed changes

MarcusSorealheis reviewed Jul 21, 2024

View reviewed changes

aleksdmladenovic force-pushed the prevent-store-share branch 2 times, most recently from 452ba6b to 5aba799 Compare July 22, 2024 18:15

aleksdmladenovic force-pushed the prevent-store-share branch from 5aba799 to eb06c5f Compare July 22, 2024 18:18

aleksdmladenovic requested review from MarcusSorealheis and allada July 22, 2024 18:18

aleksdmladenovic commented Jul 22, 2024

View reviewed changes

MarcusSorealheis reviewed Jul 22, 2024

View reviewed changes

aleksdmladenovic force-pushed the prevent-store-share branch from eb06c5f to d9e2ce8 Compare July 23, 2024 08:20

aleksdmladenovic requested a review from MarcusSorealheis July 23, 2024 08:20

aleksdmladenovic commented Jul 23, 2024

View reviewed changes

aleksdmladenovic force-pushed the prevent-store-share branch 2 times, most recently from 89c5ba1 to 9d5e661 Compare July 24, 2024 02:08

aleksdmladenovic commented Jul 24, 2024

View reviewed changes

allada requested changes Jul 24, 2024

View reviewed changes

aaronmondal reviewed Jul 25, 2024

View reviewed changes

aleksdmladenovic commented Jul 25, 2024

View reviewed changes

aleksdmladenovic force-pushed the prevent-store-share branch 2 times, most recently from f35455b to 36b0491 Compare August 2, 2024 04:25

aleksdmladenovic commented Aug 2, 2024

View reviewed changes

aleksdmladenovic requested a review from aaronmondal August 2, 2024 04:27

aleksdmladenovic requested a review from allada August 2, 2024 04:27

aleksdmladenovic force-pushed the prevent-store-share branch from 36b0491 to 101fbbc Compare August 2, 2024 15:41

MarcusSorealheis requested changes Aug 3, 2024

View reviewed changes

aleksdmladenovic force-pushed the prevent-store-share branch from 101fbbc to cc1d398 Compare August 5, 2024 01:52

aleksdmladenovic commented Aug 5, 2024

View reviewed changes

aleksdmladenovic requested a review from MarcusSorealheis August 5, 2024 11:50

aleksdmladenovic force-pushed the prevent-store-share branch from cc1d398 to 22de268 Compare August 8, 2024 13:46

aleksdmladenovic force-pushed the prevent-store-share branch from 22de268 to ae50187 Compare August 9, 2024 09:28

aleksdmladenovic force-pushed the prevent-store-share branch from ae50187 to 57b63e8 Compare August 16, 2024 14:10

MarcusSorealheis closed this Dec 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CAS and AC server not to use the same store #1175

Make CAS and AC server not to use the same store #1175

aleksdmladenovic commented Jul 19, 2024 •

edited

Loading

aleksdmladenovic commented Jul 19, 2024

allada left a comment

MarcusSorealheis left a comment

aleksdmladenovic left a comment

aleksdmladenovic left a comment

MarcusSorealheis commented Jul 21, 2024

MarcusSorealheis left a comment •

edited

Loading

CLAassistant commented Jul 22, 2024 •

edited

Loading

aleksdmladenovic left a comment •

edited

Loading

MarcusSorealheis left a comment

MarcusSorealheis left a comment

aleksdmladenovic left a comment

aleksdmladenovic left a comment

allada left a comment

aaronmondal left a comment

MarcusSorealheis commented Jul 25, 2024

aleksdmladenovic left a comment

aleksdmladenovic left a comment

MarcusSorealheis commented Aug 3, 2024

MarcusSorealheis left a comment

aleksdmladenovic left a comment

aleksdmladenovic commented Aug 8, 2024

aleksdmladenovic commented Aug 9, 2024

aleksdmladenovic commented Aug 16, 2024

MarcusSorealheis commented Dec 29, 2024

Make CAS and AC server not to use the same store #1175

Make CAS and AC server not to use the same store #1175

Conversation

aleksdmladenovic commented Jul 19, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist

aleksdmladenovic commented Jul 19, 2024

allada left a comment

Choose a reason for hiding this comment

MarcusSorealheis left a comment

Choose a reason for hiding this comment

aleksdmladenovic left a comment

Choose a reason for hiding this comment

aleksdmladenovic left a comment

Choose a reason for hiding this comment

MarcusSorealheis commented Jul 21, 2024

MarcusSorealheis left a comment • edited Loading

Choose a reason for hiding this comment

CLAassistant commented Jul 22, 2024 • edited Loading

aleksdmladenovic left a comment • edited Loading

Choose a reason for hiding this comment

MarcusSorealheis left a comment

Choose a reason for hiding this comment

MarcusSorealheis left a comment

Choose a reason for hiding this comment

aleksdmladenovic left a comment

Choose a reason for hiding this comment

aleksdmladenovic left a comment

Choose a reason for hiding this comment

allada left a comment

Choose a reason for hiding this comment

aaronmondal left a comment

Choose a reason for hiding this comment

MarcusSorealheis commented Jul 25, 2024

aleksdmladenovic left a comment

Choose a reason for hiding this comment

aleksdmladenovic left a comment

Choose a reason for hiding this comment

MarcusSorealheis commented Aug 3, 2024

MarcusSorealheis left a comment

Choose a reason for hiding this comment

aleksdmladenovic left a comment

Choose a reason for hiding this comment

aleksdmladenovic commented Aug 8, 2024

aleksdmladenovic commented Aug 9, 2024

aleksdmladenovic commented Aug 16, 2024

MarcusSorealheis commented Dec 29, 2024

aleksdmladenovic commented Jul 19, 2024 •

edited

Loading

MarcusSorealheis left a comment •

edited

Loading

CLAassistant commented Jul 22, 2024 •

edited

Loading

aleksdmladenovic left a comment •

edited

Loading