Move getter/setter methods to Ci::Metadatable module and apply feature flags
Problem
The code relies strictly on data to be written and read on specific tables. For example exit_code= always delegates to Ci::BuildMetadata. Since some data is moved to new destinations (e.g. intrinsic data moved to ci_builds and immutable processing data to ci_job_prototypes) we need to be able to use feature flags to read/write data to specific destination.
Proposal
Move getter/setter methods from Ci::Build and Ci::BuildMetadata to Metadatable module so we can switch the source based on the feature flag / state (options, yaml_variables, timeout, id_tokens, exit_code, etc.).
By leveraging Metadatable we would depend upon the abstraction rather than the specific table/model Ci::Build or Ci::BuildMetadata.
Checkout the PoC Draft: PoC: split build metadata into `job_prot... (!193948 - closed) to see what methods need to be moved and how. Methods like timeout_human_readable, timeout_value, exit_code, exit_code=, enable_debug_trace!, etc. needs to be moved.
The idea is to have:
- getters will use a feature flag
read_from_new_ci_destinationsto determine whether to serve the value from the newci_job_prototypestable or from the legacyci_builds_metadata. - setters will write to 2 destinations:
-
either
ci_buildsfor intrinsic data orci_job_prototypesfor immutable processing data. -
ci_builds_metadatato maintain behavior backwards compatible and allowing us to make 2-way-door decisions. We will use a feature flagstop_writing_builds_metadatato control when to stop writing to this table.
-
Implementation
1. Introduce the feature flag read_from_new_ci_destinations.
- Apply to
optionsandyaml_variables. - Update all places that preload
:metadatato also preload:job_definition, to avoid N+1 queries.
-> MR: Introduce FF `read_from_new_ci_destinations` an... (!199971 - merged)
2. Prepare for introduction of FF stop_writing_builds_metadata for job processing data.
- Update job factories to write to both
ci_builds_metadataandci_job_definitions. Stop mutatingoptionsandyaml_variablesin spec tests. - We'll need to update the
degenerate!method since all factory-created job objects will havejob_definitionpresent, so we need to destroy it. - Update all factories/specs affected per !199971 (comment 2675309188):
@fabiopitino:@lma-git- in a discussion with@mbobinI recall we discussed this problem. We cannot mutate the config inci_job_definitionsbecause it would impact other jobs sharing the same job definition. E.g. 2 jobs A and B are initially created with the same options/definition. They will share the same definition ID. If later we mutate job A's options and update the relative job definition, it would mutate also the definition of job B.IMO we should gradually remove all
options=from the specs and replace them with factories or stubs likeexpect(job).to receive(:options).and_return(...).
-> MR: Update Rspec job factories to write to ci_job_d... (!202317 - merged)
3. Introduce the feature flag stop_writing_builds_metadata.
- Apply the feature flag to an intrinsic data field to start.
-> MR: Introduce FF `stop_writing_builds_metadata` and... (!202462 - merged)
4. Address each field that needs to have its getter/setter added/moved/updated to Ci::Metadatable. Apply feature flags as necessary.
| Field(s) | Status | MR / Notes |
|---|---|---|
options, yaml_variables
|
!199971 (merged), !203321 (merged) | |
scoped_user_id |
!203085 (merged) | |
timeout, timeout_source
|
#551827 (closed), !202970 (merged) | |
exit_code |
!202815 (merged) | |
debug_trace_enabled |
!202462 (merged) | |
downstream_errors |
Now saved to ci_job_messages table, under the feature flag ci_new_downstream_errors_location. See !202194 (merged). |
|
interruptible |
!203212 (merged) | |
secrets |
!202968 (merged) | |
id_tokens |
!202681 (merged) |