Conversation
d884569 to
aa70a5c
Compare
|
I started looking at this new version - one very high level initial comment is that it could use some terminology definitions (BCO? BTO?) at the start to make things easier to follow. |
aa70a5c to
a42a1a8
Compare
Document split between campaign orchestration and build-test execution. Add the BTO design, runtime composition model, build-test plan contracts, runner outcome vocabulary, and the BTO plan-execution diagram. Keep BCO on one bundled plan request/result boundary and leave composition, planner, builder, tester, cache, and artifact handoff details behind BTO. Signed-off-by: Konstantin Belov <konstantin.belov@linaro.org>
Campaigns are long-running, crash-prone, and triggered more than once per regression. With no single owner, restarts lose progress and duplicate triggers run duplicate work. Document BCO as the owner: it admits triggers, collapses duplicates onto one campaign via an equivalence key, and persists state to resume after a crash. Long build/test work resumes on a BTO push, with a scan as fallback. Signed-off-by: Konstantin Belov <konstantin.belov@linaro.org>
Embedding next-commit selection in BCO fixes the search to plain bisection and couples every search-policy change to orchestration code. Document Commit selector as the owner of search policy, candidate selection, and convergence. BCO requests the next commit through a narrow contract; multi-candidate policies stay open questions. Signed-off-by: Konstantin Belov <konstantin.belov@linaro.org>
Build and test output is noisy and shaped by the backend. Reading a verdict straight out of it blurs two questions: could this commit be tested at all, and if so, is it good or bad. A commit that never built and a commit that built and regressed both need a verdict, but the search has to keep them apart. Results Analyzer turns raw output into evidence and nothing more. Decision engine reads that evidence and returns the step decision in two stages. Qualification comes first, so an untestable commit becomes skip rather than a false good or bad. The binary or performance check follows. Baseline and metric direction come from the trigger, so the system never measures a boundary itself. Signed-off-by: Konstantin Belov <konstantin.belov@linaro.org>
Replace it with a component map and catalog, and add the shared terms, data model, and open-question registry the other docs reference. Signed-off-by: Konstantin Belov <konstantin.belov@linaro.org>
a42a1a8 to
02a873c
Compare
Track execution_status, execution_environments, and plan references per attempt, and widen the performance regression's fixed-scope dimensions to include kernel configuration and toolchain. Signed-off-by: Konstantin Belov <konstantin.belov@linaro.org>
Signed-off-by: Konstantin Belov <konstantin.belov@linaro.org>
Signed-off-by: Konstantin Belov <konstantin.belov@linaro.org>
gctucker
left a comment
There was a problem hiding this comment.
My first piece of feedback on the overall design. As it's already very large, I used the glossary or Terms file to capture my comments. The Open Questions would seem like the next section to look at although I thought this was a good place to break down the review for now.
It seems to me overall that there are many lessons learnt from the legacy KernelCI bisection implementation and its real-world use cases that aren't being taken into account here, so the design description has gone into lots of detail before validating the fundamental concepts. A better approach would be to have a more high-level, simpler design document first and then a detailed plan to go through the different implementation stages. Probably each iteration would cause a feedback review of the design as it's hard to make it future-proof from the start, but at least it would clarify what is the long-term goal and how it's being carried out in practice.
| - **Regression** — a change in observed kernel behaviour from passing to failing, or from a known | ||
| metric baseline to a worse value. Detected before bisection starts. |
There was a problem hiding this comment.
Is there any coverage for the opposite of regressions e.g. Fixes?
The VIXI approach is to use Delta objects which describe a change in results. Detecting a Fix is useful to help confirm a Regression is gone, or contribute data history to an unstable test result.
| - **Bisection** — search method (by default binary) applied to kernel commits to identify the change | ||
| that introduced a regression. |
There was a problem hiding this comment.
Likewise, this could be done to find a Fix commit.
There are many assumptions here that the bisection design is only looking for commits that introduced issues. It makes sense to implement this part first, but by not considering a wider scope at the design stage it's going to be more difficult to correct the course later on.
| - **Bisection step** (step) — one tested build inside a campaign. | ||
| - **Triggering evidence** — the record(s) that justify starting a campaign: a detected regression, a first bad build, a filed bug. | ||
| - **Verification** — re-confirming a campaign's conclusion, typically by reverting the culprit and retesting. | ||
| - **Bisection campaign**, campaign, BC — one attempt to identify the culprit by bisection. |
There was a problem hiding this comment.
Listing it as Campain would be better IMHO as I was wondering what this was about and couldn't find it at first in the list of terms.
| - **Triggering evidence** — the record(s) that justify starting a campaign: a detected regression, a first bad build, a filed bug. | ||
| - **Verification** — re-confirming a campaign's conclusion, typically by reverting the culprit and retesting. | ||
| - **Bisection campaign**, campaign, BC — one attempt to identify the culprit by bisection. | ||
| - **Bisection step**,step — one tested build inside a campaign. |
There was a problem hiding this comment.
Bisections don't necessarily require a build. The issue being tested could be coming from static analysis, looking at the code without compiling anything.
The VIXI approach is to use a Replay feature, to reproduce any given job but with varying parameters i.e. kernel revision in the simplest case.
| - **Verification** — re-confirming a campaign's conclusion, typically by reverting the culprit and retesting. | ||
| - **Bisection campaign**, campaign, BC — one attempt to identify the culprit by bisection. | ||
| - **Bisection step**,step — one tested build inside a campaign. | ||
| - **Trigger** — registered external actor that submits campaign requests. |
There was a problem hiding this comment.
Where does the boundary between "internal" and "external" lie? In principle, the backend API can receive tasks submitted by any client from any origin with the right permissions. So as the backend itself shouldn't be dealing with this kind of logic, all triggers are external from this point of view.
Maybe simply changing the wording to "registered client" would avoid this ambiguity.
| - **Metric direction** — property of a performance metric that defines whether larger values are | ||
| better or worse, sourced by the trigger from an external metric registry. | ||
| - **Baseline value** — trigger-supplied performance measurement at the good boundary. The | ||
| bisection system does not measure boundaries. |
There was a problem hiding this comment.
While I agree there needs to be a definition of the values when dealing with measurements, this definition appears a bit ambiguous to me. It doesn't necessarily need to be about performance, for example the bisection could be looking for a change that introduced higher die temperature. This may also be used for pass/fail binary results by providing additional statistics e.g. the distribution of pass and fail when running the same test multiple times. Bisection is especially useful for hard-to-debug issues, and these are often flaky too.
Then the "bisection system" definition isn't too clear to me here. The VIXI approach is to not impose artificial boundaries between orchestrators to enable more advanced use cases, so the logic deciding which jobs to run next can overlap with what we would normally refer to as a bisection. I think the meaning of the wording here is to say that whatever is in charge of running a bisection step is not going to be pulling remote data and analysing the results on the spot but leave this to some form of orchestrator elsewhere, is that right?
| - **binary** — uses structured observation evidence for discrete regression types. | ||
| - **performance** — uses performance evidence and strategy rules for performance regressions. |
There was a problem hiding this comment.
OK I like the fact that this list could be expanded to avoid restricting use cases as the design stage.
| - **Step decision** — the result recorded for a step. One of: | ||
| - `good` — tested build does not exhibit the regression. | ||
| - `bad` — tested build exhibits the regression. | ||
| - `skip` — cannot test at this commit (e.g. pre-existing build break). Advances the search to a different commit. | ||
| - `skip` — cannot test at this commit (e.g. pre-existing build break). Advances the search to a | ||
| different commit. | ||
| - `weak` — tested but evidence uncertain (noise, partial data). Suggests re-test rather than skipping. | ||
| - **Rationale** — the recorded reason for a step decision: which signals supported `good`/`bad`, which failure caused `skip`, which uncertainty caused `weak`. | ||
| - **Rationale** — the recorded reason for a step decision: which evidence supported `good`/`bad`, which | ||
| failure caused `skip`, which uncertainty caused `weak`. |
There was a problem hiding this comment.
Would this be created by the "bisection system" (which I'm not sure how to define) or a more global orchestrator which determined that the initial measured values required a bisection? I'm asking this because if bisection jobs themselves can't determine whether a measurement is good or bad then they won't be able to decide which next step to take on their own.
| - **Single culprit identified** — one culprit found. | ||
| - **Narrowed range** — campaign produced a narrowed range without pinning a culprit. | ||
| - **Unresolved** — evidence inconsistent or incomplete. | ||
| - **Not confirmed** — the originally suspected regression was not reproduced under the fixed campaign scope. |
There was a problem hiding this comment.
Maybe it's worth making the difference between outcomes captured in the raw data and what is presented to end users. The raw data could tell that an infrastructure error prevented the job from being reproduced or that no system matches the requirement, or that the revision doesn't exist anymore etc. These are things that can be precisely described. The outcomes listed here tend to be more about what to include in a report or web UI than an actual campaign outcome - I guess both are needed eventually.
| - **Build-test plan** — request to execute build and test work for one selected candidate. | ||
| - **Plan result** — terminal response for one build-test plan, carrying references to produced | ||
| outputs and the normalized terminal outcome. |
There was a problem hiding this comment.
By the way, in addition to my earlier comments about restricting the bisection design to the particular use case of build-test regressions, does this also mean that bisecting plain builds with no runtime tests wouldn't be included in the current scope either?
Summary
Restructures the bisection design docs into a component architecture: a component catalog, six component docs, the contracts between them, nine ADRs (which I accepted internally), and architecture/workflow diagrams (can be imprecise). Updates terms, data model, and the open-questions registry.
I use two shallow scenarios to develop and test this proposal - functional regression and performance regression. As I'm focused on performance regressions more, so it's expected to see gaps for functional regressions.
Main Changes
Status
Acceptedto show the current proposed direction; review may still change them.Review Focus