chore: migrate gsutil usage to gcloud storage - #15846
Conversation
| command = ['gcloud', 'storage'] | ||
| if parallel: | ||
| command.append('-m') | ||
| pass |
There was a problem hiding this comment.
Can you comment on the point here? If it's just to void the change then could we either leave a comment, perhaps remove the condition?
|
/gcbrun |
There was a problem hiding this comment.
Actually, I just noticed that the CiFuzz image does not have gcloud installed.
$ docker run -it --rm --entrypoint /bin/bash gcr.io/oss-fuzz-base/cifuzz-base
root@25078bdb4242:/# gsutil --version
gsutil version: 5.20
root@25078bdb4242:/# gcloud --version
bash: gcloud: command not found
root@25078bdb4242:/# which gcloud
root@25078bdb4242:/#
From previous discussions, it seems that gcloud was making the image larger, which increases the time to pull it during CI. Please, could you fix the base image and check how much larger it would get? We should compare the time it takes in CI to see if the migration is worth it
|
I've made some tests using the CiFuzz image. The sizes of the
It seems that the size difference is relevant, but it might be negligible on the time it takes to pull the image in a normal CI environment, adding something like 10 seconds to pull the image. We should test with a few projects though |
|
/gcbrun |
| apt-get install -y docker-ce-cli | ||
| apt-get install -y docker-ce-cli google-cloud-cli && \ | ||
| apt-get clean && \ | ||
| rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
Why add
apt-get clean && \
rm -rf /var/lib/apt/lists/*
when they weren't there before?
There was a problem hiding this comment.
these are cleanup commands to remove temp files if any
| # Explicitly remove legacy gsutil and bq CLI binaries | ||
| rm -f /usr/bin/gsutil /usr/bin/bq /opt/gcloud/google-cloud-sdk/bin/gsutil /opt/gcloud/google-cloud-sdk/bin/bq && \ | ||
| apt-get clean && \ | ||
| rm -rf /var/lib/apt/lists/* |
| def filestore(self): | ||
| """Returns the filestore used to store persistent data.""" | ||
| return os.environ.get('FILESTORE', 'gsutil') | ||
| return os.environ.get('FILESTORE', 'gcloud storage') |
There was a problem hiding this comment.
can you elaborate on the reasoning behind this? Specifically, this is not a command but rather a lookup, so have you validated the semantics/impact here?
There was a problem hiding this comment.
I don't know the exact context, I can keep only gcloud if it used only for lookup
There was a problem hiding this comment.
Thanks for catching this @DavidKorczynski !
@gurusai-voleti you probably need to either change the directory paths and class from gsutil to gcloud or keep as is. This is used as map to the correct class here: https://github.com/google/oss-fuzz/blob/master/infra/cifuzz/filestore_utils.py#L25
ViniciustCosta
left a comment
ViniciustCosta
left a comment
There was a problem hiding this comment.
Looks good, but we need to test it to check the image pull overhead due to the gcloud SDK.
| def filestore(self): | ||
| """Returns the filestore used to store persistent data.""" | ||
| return os.environ.get('FILESTORE', 'gsutil') | ||
| return os.environ.get('FILESTORE', 'gcloud storage') |
There was a problem hiding this comment.
Thanks for catching this @DavidKorczynski !
@gurusai-voleti you probably need to either change the directory paths and class from gsutil to gcloud or keep as is. This is used as map to the correct class here: https://github.com/google/oss-fuzz/blob/master/infra/cifuzz/filestore_utils.py#L25
reverted to Gsutil only as it works as folder identifier |
3 participants
No description provided.