Skip to content

Prevent unnecessary jsonb updates for builds_metadata

Marius Bobin requested to merge mb-replace-serialize-with-attributes-api into master

What does this MR do and why?

While investigating gitlab-com/gl-infra/production#7907 (closed) we've noticed that the requests for job assignment to /api/:version/jobs/request were updating the config_options column on ci_builds_metadata table:

Process 3091143: /*application:web,correlation_id:eb6a2d49457ed84be0c62fb87dbf97cf,endpoint_id:POST /api/:version/jobs/request,db_config_name:ci*/ UPDATE "ci_builds_metadata" SET "config_options" = '[REDACTED]' WHERE "ci_builds_metadata"."id" = 3028893490

This should not happen because we don't have any business logic to change the values stored in that column after a pipeline is created.

This is started by transitioning the job into the running state which triggers a transition callback:

      after_transition pending: :running do |build|
        build.ensure_metadata.update_timeout_state
      end

This callback calls update on the Ci::BuildMetadata record:

    def update_timeout_state
      timeout = timeout_with_highest_precedence

      return unless timeout

      update(timeout: timeout.value, timeout_source: timeout.source)
    end

Accessing the record would trigger config_options to be registered as changed:

[1] pry(main)> m.assign_attributes(timeout: 120, timeout_source: 1)
=> nil
[2] pry(main)> m.changes
=> {"timeout"=>[nil, 120],
 "config_options"=>
  [{"image"=>{"name"=>"busybox:latest"},
    "script"=>["echo \"Do another parallel test here\"", "echo \"For example run a lint test\""],
    "after_script"=>["echo \"After script section\"", "echo \"For example you might do some cleanup here\""],
    "before_script"=>["echo \"Before script section\"", "echo \"For example you might run an update here or install a build dependency\"", "echo \"Or perhaps you might print out some debugging details\""]},
   {"image"=>{"name"=>"busybox:latest"},
    "script"=>["echo \"Do another parallel test here\"", "echo \"For example run a lint test\""],
    "after_script"=>["echo \"After script section\"", "echo \"For example you might do some cleanup here\""],
    "before_script"=>["echo \"Before script section\"", "echo \"For example you might run an update here or install a build dependency\"", "echo \"Or perhaps you might print out some debugging details\""]}]}
[3] pry(main)> m.changes["config_options"].reduce(:==)
=> true

If the timeout and timeout_source values do not change, we end up updating only the serialized JSONb values.

Screenshots or screen recordings

Before

[1] pry(main)> metadata = Ci::BuildMetadata.last; nil
  Ci::BuildMetadata Load (0.9ms)  SELECT "ci_builds_metadata".* FROM "ci_builds_metadata" ORDER BY "ci_builds_metadata"."id" DESC LIMIT 1 /*application:console,db_config_name:ci,line:(pry):1:in `__pry__'*/
=> nil
[2] pry(main)> metadata.changes
=> {}
[3] pry(main)> metadata.config_options; nil
=> nil
[4] pry(main)> metadata.changes.keys
=> ["config_options"]

After

[2] pry(main)> metadata = Ci::BuildMetadata.last; nil
  Ci::BuildMetadata Load (0.7ms)  SELECT "ci_builds_metadata".* FROM "ci_builds_metadata" ORDER BY "ci_builds_metadata"."id" DESC LIMIT 1 /*application:console,db_config_name:ci,line:(pry):2:in `__pry__'*/
=> nil
[3] pry(main)> metadata.changes
=> {}
[4] pry(main)> metadata.config_options; nil
=> nil
[5] pry(main)> metadata.changes.keys
=> []

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Marius Bobin

Merge request reports