Skip to content

Conversation

@quaresmajose
Copy link
Contributor

@quaresmajose quaresmajose commented Sep 15, 2025

Since [1], kas-container is downloaded by a job and stored in the persistent
storage (as KAS_CONTAINER). However this seems unsafe since each job will override
kas-container in the persistent storage. We should download kas-container in a
workflow specific folder instead.

[1] #846

Fixes: #1015

@quaresmajose quaresmajose changed the title [WIP] ci: store the kas-container the temporary directory ci: store the kas-container the temporary directory Sep 15, 2025
@quaresmajose quaresmajose force-pushed the kas-container branch 2 times, most recently from be12532 to 248141c Compare September 15, 2025 10:40
@quaresmajose quaresmajose changed the title ci: store the kas-container the temporary directory ci: store the kas-container on the runner workspace Sep 15, 2025
@quaresmajose quaresmajose force-pushed the kas-container branch 2 times, most recently from 2f93e7e to a41eb53 Compare September 15, 2025 14:36
@quaresmajose quaresmajose changed the title ci: store the kas-container on the runner workspace ci: do not save the kas-container in the persistent space Sep 15, 2025
@quaresmajose quaresmajose force-pushed the kas-container branch 9 times, most recently from 964fe84 to 4739748 Compare September 16, 2025 09:08
@quaresmajose quaresmajose marked this pull request as ready for review September 16, 2025 09:09
Copy link
Contributor

@lumag lumag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing commit messages. Please always start commit message with the description of the problem that you are trying to solve

@quaresmajose
Copy link
Contributor Author

I rewrote commit messages and added the problem description

@lumag
Copy link
Contributor

lumag commented Sep 16, 2025

I rewrote commit messages and added the problem description

only for the second patch.

@github-actions
Copy link

github-actions bot commented Sep 16, 2025

Test Results

 14 files  ±0   28 suites  ±0   31m 3s ⏱️ + 1m 32s
 50 tests ±0   50 ✅ ±0  0 💤 ±0  0 ❌ ±0 
304 runs  ±0  304 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f5b3b71. ± Comparison against base commit 24b5ed2.

♻️ This comment has been updated with latest results.

@quaresmajose quaresmajose requested a review from mwasilew as a code owner October 3, 2025 10:39
@quaresmajose quaresmajose force-pushed the kas-container branch 3 times, most recently from 414bd39 to cd98190 Compare October 3, 2025 10:59
@quaresmajose
Copy link
Contributor Author

please include Fixes: #1015 in the commit log.

The commit already mention that:

This change will fix [2] avoiding possible race condiction in multiple PR's.

[1] https://github.com/qualcomm-linux/meta-qcom/pull/846
[2] https://github.com/qualcomm-linux/meta-qcom/issues/1015

Does that work with Github's auto-close logic that parses commit messages?

No, it requires (more or less) exact wording as documented by GH.

Lets see if the auto-close will works, added:

Fixes: #1015

- name: Update kas-container
- name: Setting up kas-container
run: |
KAS_CONTAINER=$RUNNER_TEMP/kas-container
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can KAS_CONTAINER be defined in the env section at the top of the yml file? so that it is set only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The answer is no, that would actually be ideal and that's where I started. But it is not possible because variable expansion is not supported, so the following gives an error:

env:
  KAS_CONTAINER: ${{ runner.temp }}/kas-container

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid workflow file: .github/workflows/build-yocto.yml#L1

(Line: 13, Col: 18): Unrecognized named-value: 'runner'. Located at position 1 within expression: runner.temp

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass the path or the temp path as an input?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a variable is also good when we want to add some default command line args to the kas-container

name: kas-lockfile
path: ci/

- name: Download kas-container
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that needed here? we should download the artifact in build-yocto.yml instead, and leave that outside of the action file. it would be slightly nicer, no?

Copy link
Contributor Author

@quaresmajose quaresmajose Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is done in the action, it only needs to be done in a single location, which is what is currently implemented. If it is done in build-yocto.yml, it will have to be done every time a new worker is created. Then in build-yocto.yml it will have to be implemented in compile_warm_up and compile.

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

This pull request has been marked as stale due to 30 days of inactivity. To prevent automatic closure in 5 days, remove the stale label or add a comment. You can reopen a closed pull request at any time.

@github-actions
Copy link

Test run workflow

Test jobs for commit 5a54768

@mwasilew
Copy link
Contributor

Last set of tests failed due to infrastructure issue

ricardosalveti
ricardosalveti previously approved these changes Nov 17, 2025
@github-actions
Copy link

Test run workflow

Test jobs for commit f5b3b71

@github-actions
Copy link

Test run workflow

Test jobs for commit 59b66f3

@test-reporting-app
Copy link

test-reporting-app bot commented Nov 25, 2025

Test Results

 14 files  ±0   26 suites   - 2   49m 6s ⏱️ + 11m 55s
 52 tests +2   48 ✅  -  2  0 💤 ±0  4 ❌ +4 
296 runs   - 8  288 ✅  - 16  0 💤 ±0  8 ❌ +8 

For more details on these failures, see this check.

Results for commit 1f8614f. ± Comparison against base commit 864453d.

♻️ This comment has been updated with latest results.

Next patch will introduce new artifact downloads and uploads in the steps.
It is better to start naming that steps of the job so that they are easier
to identify when we look at the logs.
Also rename the artifact to kas-lockfile since it is more obvious
that it represents a kas locking file.

Signed-off-by: Jose Quaresma <jose.quaresma@oss.qualcomm.com>
Since [1], kas-container is downloaded by a job and stored in the persistent
storage (as KAS_CONTAINER). However this seems unsafe since each job will override
kas-container in the persistent storage. We should download kas-container in a
workflow specific folder instead.

[1] qualcomm-linux#846

Fixes: qualcomm-linux#1015

Signed-off-by: Jose Quaresma <jose.quaresma@oss.qualcomm.com>
@github-actions
Copy link

Test run workflow

Test jobs for commit 1f8614f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kas-container is potentially downloaded in an unsafe way

6 participants