Fix flushing and avoid upload caching #519
Conversation
| boost::filesystem::remove(expectedHashFile); | ||
|
|
||
| // Check putting the file | ||
| std::string fileKey = "gamma/delta/function.wasm"; |
There was a problem hiding this comment.
I refactor the upload test to use the S3 API rather than directly access the underlying filesystem.
csegarragonz
force-pushed
the
more-flushing
branch
2 times, most recently
from
fe911e9
to
60b9f91
Compare
csegarragonz
force-pushed
the
more-flushing
branch
11 times, most recently
from
36f7eed
to
c109591
Compare
.github/workflows/tests.yml
Outdated
| - name: "Flush workers" | ||
| run: docker-compose run -T cpp inv func.flush --host=nginx | ||
| - name: "Build echo function" | ||
| run: docker-compose run -T cpp bash -c "inv func demo echo && /usr/bin/cp /code/cpp/build/func/demo/echo.wasm /code/cpp/build/func/demo/hello.wasm && inv func.upload demo hello" |
There was a problem hiding this comment.
There is no nice way to split this into multiple lines without breaking the command. Given that we already break the line limit in other places in this file I have left it like this.
Possible alternatives are:
- Move to a python script/task.
- Try and do some bash hacks like suggested here.
- Add an option to
func.uploadto accept a different file path. This wouldn't make the command much shorter but maybe a bit.
There was a problem hiding this comment.
In general when there's this much CI-specific logic it's best to put it into a bash script somewhere, e.g. bin/run_quick_start_checks.sh. This has two advantages: (i) avoids gnarly syntax like this; (ii) can be run locally to debug issues.
.github/workflows/tests.yml
Outdated
| - name: "Flush workers" | ||
| run: docker-compose run -T cpp inv func.flush --host=nginx | ||
| - name: "Build echo function" | ||
| run: docker-compose run -T cpp bash -c "inv func demo echo && /usr/bin/cp /code/cpp/build/func/demo/echo.wasm /code/cpp/build/func/demo/hello.wasm && inv func.upload demo hello" |
There was a problem hiding this comment.
In general when there's this much CI-specific logic it's best to put it into a bash script somewhere, e.g. bin/run_quick_start_checks.sh. This has two advantages: (i) avoids gnarly syntax like this; (ii) can be run locally to debug issues.
src/storage/FileLoader.cpp
Outdated
| { | ||
| static thread_local FileLoader fl; | ||
| static thread_local FileLoader fl(useLocalFsCache); |
There was a problem hiding this comment.
Although this works in our current use-case, I think this can happen:
// This works, I get a FileLoader with useLocalCache == true
FileLoader &fl = getFileLoader(true):
// Here I get a ref to the same static object, so even though I requested
// useLocalFsCache == false, I get one with true
FileLoader &otherFl = getFileLoader(false):Instead we could have two methods:
// Normal one using FS cache
FileLoader &getFileLoader();
// Cache-less version
FileLoader &getFileLoaderWithoutLocalCache();The other option is to add a new subclass called FileLoaderNoCache, which makes it more explicit and perhaps more obvious to users which one they're using. However, in this case I think just having two methods makes sense.
tests/test/upload/test_upload.cpp
Outdated
| UploadTestFixture() {} | ||
| ~UploadTestFixture() {} | ||
| UploadTestFixture() { faabric::util::setTestMode(false); } | ||
| ~UploadTestFixture() { faabric::util::setTestMode(true); } |
There was a problem hiding this comment.
Why do we need to turn off test mode here? It's risky as it can mess up the local environment and cause quite hard to debug issues. Can we be more targeted in what we're switching on/ off?
There was a problem hiding this comment.
You are right, this was not needed in the end.
csegarragonz
force-pushed
the
more-flushing
branch
from
c109591
to
a25c15c
Compare
Closes #518