Skip to content

Commit 3236d01

Browse files
committed
PR 7: Test Artifacts and API Cleanup
1 parent 2c211ce commit 3236d01

12 files changed

Lines changed: 323 additions & 21 deletions

File tree

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
name: CI-VM scripts
2+
3+
# Static analysis for the CI-VM runner scripts (the scripts that run on the
4+
# throwaway test VMs). Runs only when those scripts change. shellcheck lints the
5+
# bash runners; PSScriptAnalyzer lints the PowerShell startup script.
6+
7+
on:
8+
pull_request:
9+
paths:
10+
- 'install/ci-vm/**'
11+
- '.github/workflows/ci-vm-scripts.yml'
12+
push:
13+
paths:
14+
- 'install/ci-vm/**'
15+
- '.github/workflows/ci-vm-scripts.yml'
16+
17+
jobs:
18+
shellcheck:
19+
runs-on: ubuntu-latest
20+
steps:
21+
- uses: actions/checkout@v4
22+
- name: Install shellcheck
23+
run: sudo apt-get update && sudo apt-get install -y shellcheck
24+
- name: shellcheck (bash runner scripts)
25+
run: shellcheck --severity=error install/ci-vm/ci-linux/ci/runCI install/ci-vm/ci-linux/startup-script.sh
26+
27+
psscriptanalyzer:
28+
runs-on: windows-latest
29+
steps:
30+
- uses: actions/checkout@v4
31+
- name: PSScriptAnalyzer (startup-script.ps1)
32+
shell: pwsh
33+
run: |
34+
Set-PSRepository PSGallery -InstallationPolicy Trusted
35+
Install-Module -Name PSScriptAnalyzer -Force -Scope CurrentUser
36+
$issues = Invoke-ScriptAnalyzer -Path install/ci-vm/ci-windows/startup-script.ps1 -Severity Error
37+
if ($issues) { $issues | Format-Table -AutoSize; exit 1 }

install/ci-vm/ci-linux/ci/runCI

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,21 @@ function executeCommand {
103103
fi
104104
}
105105

106+
# Send a build artifact to the server (best-effort: a failed upload must never
107+
# abort the run, so this always returns 0). Reuses the reportURL/curl pattern.
108+
function sendArtifact {
109+
local artifactType="$1"
110+
local filePath="$2"
111+
if [ ! -f "${filePath}" ]; then
112+
echo "Artifact ${artifactType}: no file at '${filePath}', skipping" >> "${logFile}"
113+
return 0
114+
fi
115+
echo "Uploading ${artifactType} artifact (${filePath})" >> "${logFile}"
116+
curl -s -A "${userAgent}" --form "type=artifactupload" --form "artifact_type=${artifactType}" \
117+
--form "file=@${filePath}" "${reportURL}" >> "${logFile}" 2>&1
118+
return 0
119+
}
120+
106121
# Source variables
107122
. "$DIR/variables"
108123

@@ -125,7 +140,25 @@ if [ -e "${dstDir}/ccextractor" ]; then
125140
echo "=== End Version Info ===" >> "${logFile}"
126141
postStatus "testing" "Running tests"
127142
executeCommand cd ${suiteDstDir}
128-
executeCommand ${tester} --debug --entries "${testFile}" --executable "ccextractor" --tempfolder "${tempFolder}" --timeout 600 --reportfolder "${reportFolder}" --resultfolder "${resultFolder}" --samplefolder "${sampleFolder}" --method Server --url "${reportURL}"
143+
144+
# Enable core dumps and capture the test run's combined stdout/stderr so
145+
# both can be uploaded as artifacts (the API serves them per run).
146+
ulimit -c unlimited 2>/dev/null
147+
echo "core.%p" | sudo tee /proc/sys/kernel/core_pattern >/dev/null 2>&1
148+
combinedLog="${reportFolder}/combined_stdout.log"
149+
${tester} --debug --entries "${testFile}" --executable "ccextractor" --tempfolder "${tempFolder}" --timeout 600 --reportfolder "${reportFolder}" --resultfolder "${resultFolder}" --samplefolder "${sampleFolder}" --method Server --url "${reportURL}" > "${combinedLog}" 2>&1
150+
testerStatus=$?
151+
cat "${combinedLog}" >> "${logFile}"
152+
153+
# Upload artifacts before any failure-halt so crash data survives.
154+
sendArtifact "binary" "${dstDir}/ccextractor"
155+
sendArtifact "combined_stdout" "${combinedLog}"
156+
sendArtifact "coredump" "$(ls -1t core.* 2>/dev/null | head -n1)"
157+
158+
if [ ${testerStatus} -ne 0 ]; then
159+
haltAndCatchFire ""
160+
fi
161+
129162
sendLogFile
130163
postStatus "completed" "Ran all tests"
131164

install/ci-vm/ci-linux/startup-script.sh

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ curl -L -O https://github.com/GoogleCloudPlatform/gcsfuse/releases/download/v3.2
44
dpkg --install gcsfuse_3.2.0_amd64.deb
55
rm gcsfuse_3.2.0_amd64.deb
66

7-
apt install gnupg ca-certificates
7+
apt install -y gnupg ca-certificates
88
gpg --homedir /tmp --no-default-keyring --keyring /usr/share/keyrings/mono-official-archive-keyring.gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys 3FA7E0328081BFF6A14DA29AA6A19B38D3D831EF
99
echo "deb [signed-by=/usr/share/keyrings/mono-official-archive-keyring.gpg] https://download.mono-project.com/repo/ubuntu stable-focal main" | sudo tee /etc/apt/sources.list.d/mono-official-stable.list
1010
sudo apt update
@@ -14,7 +14,8 @@ mkdir repository
1414
cd repository
1515

1616
# Use gcsfuse and import required files
17-
mkdir temp TestFiles TestResults vm_data reports
17+
# TempFiles is used by the tester (--tempfolder) and must exist
18+
mkdir temp TestFiles TestResults TempFiles vm_data reports
1819

1920
gcs_bucket=$(curl http://metadata/computeMetadata/v1/instance/attributes/bucket -H "Metadata-Flavor: Google")
2021

@@ -31,6 +32,9 @@ mount vm_data
3132
mount TestFiles
3233
mount TestResults
3334

35+
# Give gcsfuse mounts time to become ready
36+
sleep 10
37+
3438
cp temp/* ./
3539

3640
chmod +x bootstrap

install/ci-vm/ci-windows/ci/runCI.bat

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@ for /F %%R in ('curl http://metadata/computeMetadata/v1/instance/attributes/repo
1515
SET userAgent="CCX/CI_BOT"
1616
SET logFile="%reportFolder%/log.html"
1717

18-
call :postStatus "preparation" "Loaded variables, created log file and checking for CCExtractor build artifact" >> "%logFile%"
18+
rem NB: no outer ">> %logFile%" here. postStatus already appends to %logFile%
19+
rem internally; an outer redirect to the same file self-locks on Windows
20+
rem (the inner append cannot open a file the outer redirect holds open).
21+
call :postStatus "preparation" "Loaded variables, created log file and checking for CCExtractor build artifact"
1922

2023
echo Checking for CCExtractor build artifact
2124
if EXIST "%dstDir%\ccextractorwinfull.exe" (
@@ -27,7 +30,17 @@ if EXIST "%dstDir%\ccextractorwinfull.exe" (
2730
echo === End Version Info === >> "%logFile%"
2831
call :postStatus "testing" "Running tests"
2932
call :executeCommand cd %suiteDstDir%
30-
call :executeCommand "%tester%" --debug True --entries "%testFile%" --executable "ccextractorwinfull.exe" --tempfolder "%tempFolder%" --timeout 600 --reportfolder "%reportFolder%" --resultfolder "%resultFolder%" --samplefolder "%sampleFolder%" --method Server --url "%reportURL%"
33+
34+
rem Capture the test run's combined stdout/stderr for upload as an artifact.
35+
"%tester%" --debug True --entries "%testFile%" --executable "ccextractorwinfull.exe" --tempfolder "%tempFolder%" --timeout 600 --reportfolder "%reportFolder%" --resultfolder "%resultFolder%" --samplefolder "%sampleFolder%" --method Server --url "%reportURL%" > "%reportFolder%/combined_stdout.log" 2>&1
36+
if errorlevel 1 (set "testerFailed=1") else (set "testerFailed=")
37+
type "%reportFolder%/combined_stdout.log" >> "%logFile%"
38+
39+
rem Upload artifacts (best-effort; never aborts the run). Windows skips coredump for v1.
40+
call :sendArtifact "binary" "%dstDir%\ccextractorwinfull.exe"
41+
call :sendArtifact "combined_stdout" "%reportFolder%/combined_stdout.log"
42+
43+
if defined testerFailed call :haltAndCatchFire ""
3144

3245
call :sendLogFile
3346

@@ -144,3 +157,18 @@ if !sl_attempt! LEQ %sl_max_retries% (
144157
echo ERROR: Failed to upload log after %sl_max_retries% attempts >> "%logFile%"
145158
endlocal
146159
EXIT /B 1
160+
161+
rem Send a build artifact to the server (best-effort; never aborts the run)
162+
:sendArtifact
163+
setlocal
164+
set "sa_type=%~1"
165+
set "sa_file=%~2"
166+
if NOT EXIST "%sa_file%" (
167+
echo Artifact %sa_type%: no file at "%sa_file%", skipping >> "%logFile%"
168+
endlocal
169+
EXIT /B 0
170+
)
171+
echo Uploading %sa_type% artifact (%sa_file%) >> "%logFile%"
172+
curl -s -A "%userAgent%" --form "type=artifactupload" --form "artifact_type=%sa_type%" --form "file=@%sa_file%" "%reportURL%" >> "%logFile%" 2>&1
173+
endlocal
174+
EXIT /B 0

mod_api/routes/auth.py

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ def revoke_current_token():
147147

148148

149149
@mod_api.route('/auth/tokens', methods=['GET'])
150-
@require_roles(['admin', 'contributor', 'tester'])
150+
@require_roles(['admin'])
151151
@require_scope('tokens:manage')
152152
@validate_offset_pagination()
153153
def list_tokens(limit=50, offset=0):
@@ -199,9 +199,8 @@ def revoke_specific_token(token_id):
199199
if not token or (not is_admin and not is_own):
200200
return make_error_response('not_found', 'Token not found.', http_status=404)
201201

202-
if not is_own and not (is_admin or g.api_token.has_scope('tokens:manage')):
203-
return make_error_response('forbidden', 'Cross-user revocation requires tokens:manage scope.', http_status=403)
204-
202+
# Reaching here means the caller is either the owner or an admin (any other
203+
# caller was already given a 404 above), so the revocation is authorized.
205204
if not token.is_revoked:
206205
token.revoke()
207206
g.db.add(token)

mod_api/routes/runs.py

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,9 @@ def create_run(validated_data=None):
342342
main_owner = config.get('GITHUB_OWNER', '')
343343
main_repo = config.get('GITHUB_REPOSITORY', '')
344344
main_repo_full = f'{main_owner}/{main_repo}'
345-
target_repo = repository or main_repo_full
345+
# repository is a required field (RunCreateRequestSchema), so it is always
346+
# present; a main-repo run passes the main repo's "owner/repo" explicitly.
347+
target_repo = repository
346348

347349
err = _validate_run_permissions(g.api_user, target_repo, main_repo_full)
348350
if err:
@@ -360,10 +362,7 @@ def create_run(validated_data=None):
360362
http_status=422,
361363
)
362364

363-
if repository:
364-
fork_url = f'https://github.com/{repository}.git'
365-
else:
366-
fork_url = f"https://github.com/{main_owner}/{main_repo}.git"
365+
fork_url = f'https://github.com/{repository}.git'
367366

368367
fork, err = _get_or_create_fork(fork_url)
369368
if err:

mod_api/routes/samples.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,11 @@
3232
from mod_test.models import (Test, TestPlatform, TestProgress, TestResult,
3333
TestResultFile)
3434

35-
# Valid per-sample status values accepted by the ?status filter.
35+
# Valid per-sample status values accepted by the ?status filter. Limited to the
36+
# statuses derive_sample_status can actually emit, so filtering can't silently
37+
# return empty for a value that never occurs.
3638
_VALID_SAMPLE_STATUSES = frozenset({
37-
'pass', 'fail', 'skipped', 'missing_output',
38-
'running', 'not_started', 'canceled', 'incomplete',
39+
'pass', 'fail', 'missing_output', 'not_started',
3940
})
4041

4142

mod_api/services/error_service.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,8 +235,12 @@ def derive_infrastructure_errors(test_id: int) -> List[Dict[str, Any]]:
235235
).all()
236236

237237
for p in progress_rows:
238-
msg_lower = (p.message or '').lower()
239-
error_type = _classify_infra_error(msg_lower)
238+
message = p.message or ''
239+
# User-initiated cancellations (cancel_run writes "... via API") are not
240+
# infrastructure failures, so they must not be reported here.
241+
if 'via API' in message:
242+
continue
243+
error_type = _classify_infra_error(message.lower())
240244
errors.append({
241245
'error_id': f'infra_{test_id}_{p.id}',
242246
'run_id': test_id,

mod_api/services/log_service.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,8 @@ def _read_lines(encoding):
9999

100100

101101
def _matches_level(line: str, target_level: str) -> bool:
102-
"""Check if a log line matches the requested severity."""
103-
return _extract_level(line) == target_level
102+
"""Check if a log line matches the requested severity (case-insensitive)."""
103+
return _extract_level(line) == (target_level or '').lower()
104104

105105

106106
def _extract_level(line: str) -> str:

mod_ci/controllers.py

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2346,6 +2346,11 @@ def progress_reporter(test_id, token):
23462346
if not upload_type_request(log, test_id, repo_folder, test, request):
23472347
return "EMPTY"
23482348

2349+
elif request.form['type'] == 'artifactupload':
2350+
log.info(f'[PROGRESS_REPORTER][Test: {test_id}] Artifact upload')
2351+
if not upload_artifact_type_request(log, test_id, repo_folder, test, request):
2352+
return "EMPTY"
2353+
23492354
elif request.form['type'] == 'finish':
23502355
log.info(f'[PROGRESS_REPORTER][Test: {test_id}] Test finished')
23512356
finish_type_request(log, test_id, test, request)
@@ -2675,6 +2680,78 @@ def upload_type_request(log, test_id, repo_folder, test, request) -> bool:
26752680
return False
26762681

26772682

2683+
# Artifact types the CI VM may upload, mapped to the fixed filenames the REST
2684+
# API expects under <SAMPLE_REPOSITORY>/test_artifacts/<run_id>/ (see the
2685+
# system route _get_gcs_artifacts in mod_api).
2686+
ARTIFACT_TYPES = frozenset({'binary', 'coredump', 'combined_stdout'})
2687+
2688+
2689+
def _artifact_target_name(artifact_type: str, platform) -> Optional[str]:
2690+
"""Map an artifact type + platform to the exact filename the API resolves."""
2691+
if artifact_type == 'binary':
2692+
return 'ccextractor' if platform == TestPlatform.linux else 'ccextractorwinfull.exe'
2693+
if artifact_type == 'coredump':
2694+
return 'coredump'
2695+
if artifact_type == 'combined_stdout':
2696+
return 'combined_stdout.log'
2697+
return None
2698+
2699+
2700+
def upload_artifact_type_request(log, test_id, repo_folder, test, request) -> bool:
2701+
"""
2702+
Handle the artifactupload request type for the progress reporter.
2703+
2704+
Stores a CI artifact (binary, coredump, or combined stdout log) under
2705+
``<SAMPLE_REPOSITORY>/test_artifacts/<test_id>/`` with the fixed name the
2706+
REST API expects. The target name is derived server-side from
2707+
``artifact_type`` and the test platform, never from the uploaded filename,
2708+
so a crafted filename cannot escape the artifact directory.
2709+
2710+
:param log: logger
2711+
:type log: Logger
2712+
:param test_id: the id of the test the artifact belongs to
2713+
:type test_id: int
2714+
:param repo_folder: SAMPLE_REPOSITORY path
2715+
:type repo_folder: str
2716+
:param test: the concerned test
2717+
:type test: Test
2718+
:param request: request parameters
2719+
:type request: Request
2720+
:return: True on success, False on validation failure
2721+
:rtype: bool
2722+
"""
2723+
artifact_type = request.form.get('artifact_type', '')
2724+
if artifact_type not in ARTIFACT_TYPES:
2725+
log.warning(f'[Test: {test_id}] Rejected artifact upload: bad artifact_type {artifact_type!r}')
2726+
return False
2727+
2728+
if 'file' not in request.files:
2729+
log.warning(f'[Test: {test_id}] Artifact upload missing file')
2730+
return False
2731+
2732+
uploaded_file = request.files['file']
2733+
if secure_filename(uploaded_file.filename or '') == '':
2734+
log.warning(f'[Test: {test_id}] Artifact upload has empty filename')
2735+
return False
2736+
2737+
target_name = _artifact_target_name(artifact_type, test.platform)
2738+
if target_name is None:
2739+
return False
2740+
2741+
artifact_dir = os.path.join(repo_folder, 'test_artifacts', str(test.id))
2742+
temp_dir = os.path.join(repo_folder, 'TempFiles')
2743+
os.makedirs(artifact_dir, exist_ok=True)
2744+
os.makedirs(temp_dir, exist_ok=True)
2745+
2746+
temp_path = os.path.join(temp_dir, f'artifact_{test.id}_{target_name}')
2747+
uploaded_file.save(temp_path)
2748+
final_path = os.path.join(artifact_dir, target_name)
2749+
os.replace(temp_path, final_path)
2750+
2751+
log.info(f'[Test: {test_id}] Stored {artifact_type} artifact at {final_path}')
2752+
return True
2753+
2754+
26782755
def finish_type_request(log, test_id, test, request):
26792756
"""
26802757
Handle finish request type for progress reporter.

0 commit comments

Comments
 (0)