[Android V4 api Migration] Creates Feature Flag#5373
Conversation
dylanjew
left a comment
There was a problem hiding this comment.
thanks! just one question
| """Boot cuttlefish instance using stable build id fetched from gcs.""" | ||
| build_info = fetch_artifact.get_latest_artifact_info( | ||
| branch, target, stable_build=True) | ||
| if not build_info: |
There was a problem hiding this comment.
what happens if this returns without the build? Does the device get stuck in a flash cycle?
There was a problem hiding this comment.
Trueeeee forgot about what i told you in this doc's appendix.
I performed an extensive investigation with jetski(TMI to digest by myself), and heres what i found out:
- All non uworker bots can update their codebase themselves trough update_task
- When a PHYSICAL android bot needs to flash and for some reason, the artifact/build is not found, we reach this bad_state method, which causes the android device to sleep for 1 hour.
- After that the bot wakes up and since we are still in a loop it updates itself and tries to work again, so if lets say we introduce a defect in the code then the bot will just update itself each hour till we eventually publish the fix
- If there's a none code issue(like a failing api) or something is inherently wrong with the bot then the bot will just continue to operate till it hits another timeout
So the way i see it here is to ensure that if a bot fails it calls the bad_state function to trigger the whole self-healing process, which we already do(see changes in flash.py)
For emulated uworker devices(like in swarming), we are 100% sure that they wont try to flash themselves, so theres no worry in here. The other place where we could run into an android api v4 issue is at symbols_downloader.py where it tries to get artifacts to process a stack-trace of errors, but i also investigated if this could cause the bot to loop, and the validations that we have in place avoid the uworker bot from crashing and just ignores the stack_trace symbolization.
There was a problem hiding this comment.
Thanks! This flag is fine as long as the bot is in the retry loop but isn't actually making a bunch of calls that we know will fail.
…oops When the Android Build API feature flag is disabled, or when artifacts are missing, bots can enter a cycling state. Previously, `fetch_artifact.py` explicitly called `adb.bad_state_reached()`. For uworkers, this forced an un-trapped `sys.exit(-1)` that caused them to silently abandon tasks and cycle. This commit delegatess the error handling of `fetch_artifact` failures to its callers. We also updated `flash.py` to only trigger `adb.bad_state_reached()` on physical devices if the API is disabled and no local images are present, forcing the host to wait 1 hour rather than spinning. Guarded `download_latest_build` checks allow devices with local images to reuse them when the API is toggled off.TAG=agyCONV=773a0adb-dc53-4706-b185-52bf9391df88
5f6c11e to
629a1a6
Compare
| """Boot cuttlefish instance using stable build id fetched from gcs.""" | ||
| build_info = fetch_artifact.get_latest_artifact_info( | ||
| branch, target, stable_build=True) | ||
| if not build_info: |
There was a problem hiding this comment.
Thanks! This flag is fine as long as the bot is in the retry loop but isn't actually making a bunch of calls that we know will fail.
|
LGTM! last question is more of a nit |
| if flag is None: | ||
| return True | ||
| return flag.enabled |
There was a problem hiding this comment.
| if flag is None: | |
| return True | |
| return flag.enabled | |
| return flag.enabled if flag else True |
## Overview Bug: b/422775458 Our previous implementation of the android builv v4 api was wrong, it failed always because the api no longer worked on discovery service url. In other [PR](#5370) we focused on creating the new api to interact with the v4 requirements. In this PR we focus on using this api class in the `fetch_artifact.py` module, ## Changes - Made private all the methods that were only used internally in `fetch_artifact.py` - Refactors previous usage of the v4 api to now use the current version with minimal changes in the `fetch_artifact.py` script to avoid breaking anything ## Tests performed - Created unit tests for `feth_artifact.py` - Tested this script using [a custom script](https://github.com/google/clusterfuzz/compare/android-build-v4-testing) (test details in [this PR description](#5370) ) #### PR review 1. [**#5370** Add Android Build API V4 REST client and tests ](#5370) 2. 👉 #5371 Migrate fetch_artifact to use new V4 API *(This PR)* 3. [#5373 Creates Feature Flag for Android V4 API](#5373) Please only approve, I'll merge this PR's to avoid any possible merge conflict that can appear in the process :D
## Changes We previously migrated the v3 api to use v4 in [fetch_artifact.py](https://github.com/google/clusterfuzz/blob/master/src/clusterfuzz/_internal/platforms/android/fetch_artifact.py) but after testing the implementation we discovered that we can't continue the same approach of using a client created by service discovery url , if we want to continue using the android build api we needed to change our strategy and start using raw http requests. - Creates the new android v4 api class - Instantiated trough a static method that recieves service account credentials to avoid silent failures - Updates the `utils.fetch_url` to unify requests and reuse it in this new api while maintaining previous functionality For more info in the v4 api see: go/build-api-v4-clients-migration-guide & b/422775458 ## Tests - Created unit tests for this new class - Tested this class implementation using [a custom script](https://github.com/google/clusterfuzz/compare/android-build-v4-testing) ### Testing script details The [custom script](https://github.com/google/clusterfuzz/compare/android-build-v4-testing) performs various tests that imitate how clusterfuzz would use the `fetch_artifact.py` module in conjunction with our new api. The script test the following: - Correct API Version selection - Correct API instantiation by checking that the credentials in the db are valid - Queries latest artifact info(like build_id) for the given branch and target - branch and target come from a adb device (see where [we got this from](https://paste.googleplex.com/5462351594782720)) - Uses the retrieved build_id to get android builds artifact metadata - Downloads the given artifact Test steps: - Chery-pick the scripot commit into my branch - Run the script using butler: ``` pipenv run python butler.py run -c /usr/local/google/home/ibarba/projects/clusterfuzz-config/configs/chrome-staging test_fetch_artifact \ --script_args \ --branch=git_main \ --target=cf_x86_64_phone-next-userdebug ``` ## Dev verification After the initial tests ^ i tested this changes once more in dev(including all 3 prs), looking specifically for `utils.fetch_url` or errors inherent to the return value from this method. Basically since my changes landed in dev the following error groups where seen: <img width="1735" height="1240" alt="image" src="https://github.com/user-attachments/assets/0591fb5c-d728-406a-b888-4f2534b2daab" /> Discarded the errors following this logic: - 403 Errors are expected to appear as per the changes that were previously there to avoid them are no longer in dev - Some error groups only appeared on previous clusterfuzz versions - Other errors are expected(like the queue size limit or bad revision errors) - There are errors from corpus pruning & fuzzer not found, this have been seen for a while now, not related to my changes - Lastly there are some errors in a cron_job, specifically in [this file ](https://github.com/google/clusterfuzz/blob/master/src/clusterfuzz/_internal/cron/load_bigquery_stats.py)but this one doesn't make usage of `fetch_url` and nor does the method that triggered the error ## Note: ### PR review Stack 1. 👉 **#5370** Add Android Build API V4 REST client and tests *(This PR)* 2. [#5371 Migrate fetch_artifact to use new V4 API](#5371) 3. [#5373 Creates Feature Flag for Android V4 API](#5373) Please only approve, I'll merge this PR's to avoid any possible merge conflict that can appear in the process :D
Overview
As per requested, we created a new feature flag to disable definitely the API build use cases(used trough
fetch_artifact.py), this feature flag is enabled by default, because that's the current API & ClusterFuzz workflow.But can be disabled by adding the feature flag to the DB and marking it as enabled=false.
Changes
fetch_artifact.pyfetch_artifact.pysymbols_downloaderandflash.pywhich consume thefetch_artifact.pyadb.bad_state_reach()method from thefetch_artifact.pymodule to its callers, this way we can now handle better the None values and raised errors from the API being turned off by the feature flag.Tests
After the initial tests in the PR parents, i tested this changes once more in dev(including all 3 prs), looking specifically for

utils.fetch_urlor errors inherent to the return value from this method. Basically since my changes landed in dev the following error groups where seen:Discarded the errors following this logic:
fetch_urland nor does the method that triggered the errorPR review
Please only approve, I'll merge this PR's to avoid any possible merge conflict that can appear in the process :D