Migrate away `ci_builds.trace`

changed the description

@grzesiek Since this is connected with Background Migrations, and you will probably touch that functionality, would you be willing to do that?

changed the description

@ayufan Let me try that! gitlab-ce%"9.4"?

assigned to @grzesiek

It is unlikely to happen in gitlab-ce%"9.4".

I'm going to unassign myself for the time being.

removed assignee

@ayufan and @grzesiek I'd like to pick this up. And while I'm at it, move the traces that depend on project.ci_id to the new location too.

My proposal for doing so will require quite some time during migrations, but eventually we'd end up with 1 way we store traces. Lets start by listing the ways we store the traces now:

the old, old way: in ci_builds.trace
The old way: on disk, depending on projects.ci_id
The current way, File.join(Settings.gitlab_ci.builds_path, job.created_at.utc.strftime("%Y_%m"), job.project_id.to_s)

To start in gitlab-ce%"10.0" we'd introduce the new format. Gitlab::Ci::Trace is already setup to allow adding new directories, I'll just add one, and make sure project removal works as expected. Also in this release we'd move ci_builds.trace to the new location.

%10.1 will include a migration to steal the background migration that migrates the ci_builds.trace, and drop this column. References to Ci::Build#old_trace can be removed than too. Furthermore, we'll include the background migration to move the traces depending on ci_id to the current location. This is the last location in in the code base that depends on that.

%10.2 will steal the background migrations again, after which we drop the projects.ci_id column (if possible). Than we'd move to the last move action, in %10.1 we can see the performance if the smaller move operation, and adjust to that in this release.

%10.3 PROFIT 🤑 💸 💰

One note, I was thinking on introducing ad hoc moving too, so when we load a trace being on an old location, do a move operation, after which we serve it. This might be a good idea in terms of spreading the load and making the migrations better to handle, but now we're increasing request timings for it and are bound to operations taking a few seconds tops. Plus we might run into unicorn being killed. Probably not worth it.

Edit: we should move artifacts to the same directory when using local storage, so we can remove the directory whole when erasing a job.

Sounds good to me @zj!

To start in 10.0 we'd introduce the new format.

Can you explain why do we need a new format?
Can you explain why we can't ship two migrations in 10.1 (the one that migrates ci_id and the one that migrates trace from the database) and steal both in the 10.2?

@grzesiek Thanks for your response.

With the new format I mean File.join(Settings.gitlab_ci.builds_path, job.project_id.to_s, job.created_at.utc.strftime("%Y_%m")). So the project id first, allowing batch deletion. Later we can add artifacts in the same directory.
That is an option too, I'm just a bit afraid for creating a lot of IOPS on that NFS mount. I read the code on these types of migrations and it seems that I could schedule it in a way that we take enough time not to intervene with each other, but we can not guarantee this for our customers. They might update from gitlab-ce%"10.0" to %10.3 which will steal these migrations right away. But if you and @ayufan think this is ok, it would like to include it in one release too.

I think that we can do that in a one release, but we would need to count jobs that need migration first, to get the full picture.

The actual storage path (even of today) is not a big problem, as we have limited number of directories to traverse: 12 every year.

I would prefer us to:

Migrate artifacts out of builds into separate table, where one row is one attached file, thus effectively going into direction of: https://gitlab.com/gitlab-org/gitlab-ce/issues/13227,
Once we do it, migrate all traces (including the oldest one) to behave as artifacts, one extra attached file with content.

This would unify how we handle trace, which is another artifact. This would make trace to be stored on Object Storage. This would make trace to count against disk space. This would make explicit for which, and how we store each trace. Since files need to be deleted separately we could have a routine that would walk this table and delete them one-by-one, thus solve the above problem too.

We have to do it as we have to fix the old ci_id problem of traces, migrating them to be stored as artifacts make it explicit and possible.

mentioned in issue gitlab-com/migration#23 (closed)

mentioned in issue gitlab-com/migration#36 (closed)

changed milestone to %Backlog

added [deprecated] Accepting merge requests label

added 1 deleted label and removed 1 deleted label

added Category:Continuous Integration label

added grouppipeline execution label

changed milestone to %Backlog

added pipeline processing label

added sectionops label

@grzesiek @ayufan Is this issue still needed and if not can you please close this issue?

@thaoyeager This needs to be part of the ci_builds story / blueprint, and this is a really important part of it. /cc @rickywiens

@grzesiek @thaoyeager I changed the epic for this issue to be &4685 (closed). Please feel free to change it back if that was incorrect. Thanks!

changed epic to &4685 (closed)

I was looking at this column today and we have only 40 builds that use it. Data from database-lab:

explain select * from ci_builds where trace is not null;

 Gather  (cost=1000.00..481491594.50 rows=1 width=1621) (actual time=44354.344..15617521.346 rows=40 loops=1)
   Workers Planned: 2
   Workers Launched: 2
   Buffers: shared read=119457924 dirtied=492836 written=435852
   I/O Timings: read=45939175.238 write=19133.816
   ->  Parallel Seq Scan on public.ci_builds  (cost=0.00..481490594.40 rows=1 width=1621) (actual time=83048.200..15617484.769 rows=13 loops=3)
         Filter: (ci_builds.trace IS NOT NULL)
         Rows Removed by Filter: 292941164
         Buffers: shared read=119457924 dirtied=492836 written=435852
         I/O Timings: read=45939175.238 write=19133.816

I think that we should drop this column. Please note that explain might not be accurate.

changed epic to &5417

Can we now just drop this data after properly storing it in a safe place, perhaps object storage (if it is not there already)?

changed the description

changed title from Migrate away ci_builds.trace to Migrate away {++}ci_builds.trace{++}

@jheimbuck_gl I added this to Partition CI/CD pipelines data (&5417) epic.

@mbobin @grzesiek

Is this column still being used at all now that traces are persisted as an artifact type? 🤔

40 rows!

@morefice it should not be used for years already.

@morefice almost two years ago we had 40 builds with data stored in this column: #18605 (comment 473065020) and this should have not increase since we're raising an error if someone tries to write to it. But we're still presenting it if someone tries to read it: https://gitlab.com/gitlab-org/gitlab/-/blob/ac720ee7941bdde82cf0628c66ef65bea15ad807/lib/gitlab/ci/trace.rb#L165-166, https://gitlab.com/gitlab-org/gitlab/-/blob/ac720ee7941bdde82cf0628c66ef65bea15ad807/app/models/ci/build.rb#L717-719

Should we have a closer look at the data stored in those records and see if we can migrate them to artifacts? 🤔

@mbobin @grzesiek

this should have not increase

Wonderful, we still have those 40 builds.

Should we have a closer look at the data stored in those records and see if we can migrate them to artifacts? 🤔

If we want to get rid of this column then yes.

We need to write a migration to convert them to artifacts.

We need to write a migration to convert them to artifacts.

I wonder if it would be better to do that in a production change, with a simple Rails script, because migrations are supposed to be isolated - in this case we might be unable to isolate object storage communication sufficiently, and it seems that self-managed instances might not need it. Is that a valid assumption?

I also think a production change might be better because otherwise we would also need a partial index on ci_builds to efficiently identify the ones without trace.

Maybe we should start by creating a thin clone and extract the ids that need to be migrated. If we're here we could also check the projects that they belong to and their data because there could be something wrong with them since they failed the previous migration.

Yeah, good point @mbobin. We should first check them, and see if perhaps these do belong to gitlab-org, and we can just drop the column.

I'll try to start a clone and see what info I can get.

@morefice how can we quantify the impact of removing this column on the database? 40 rows with data probably do not store a lot, what is the total storage size consumed by this column?

Also a question: the issue says that we used to have 87636 builds like this, and only 40 remains as for now. Why is that? Did we do something to remove them / migrate them already?

mentioned in merge request !96865 (merged)

changed the description

changed milestone to %15.5

added 1 deleted label

set weight to 2

👋 @mbobin - please see the following guidance and update this issue.

	1 Error
❌	Please add typebug typefeature, typemaintenance and a subtype label to this issue.

If you do not feel the purpose of this issue matches one of the types, you may apply the typeignore label to exclude it from type tracking metrics and future prompts.