Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate a system_id for the runner so gitlab can differentiate between instances #24

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

sjoerdsimons
Copy link
Collaborator

No description provided.

@pinchartl
Copy link
Contributor

I think the stateless part for the random system IDs may be problematic.

The point of the system ID is to uniquely identify the "machine" on which the runner runs. When using containers, the "machine" is the container, so we need to generate a random system ID as /etc/machine-id is usually not available (and if it was copied from the host, multiple runners would have the same system ID). The official gitlab-runner creates the random system ID on first run, and stores it to persistent storage. We may not want to do so in this crate, but making it possible for lava-gitlab-runner to do the same is important I think.

@sjoerdsimons
Copy link
Collaborator Author

I think the stateless part for the random system IDs may be problematic.

The point of the system ID is to uniquely identify the "machine" on which the runner runs. When using containers, the "machine" is the container, so we need to generate a random system ID as /etc/machine-id is usually not available (and if it was copied from the host, multiple runners would have the same system ID). The official gitlab-runner creates the random system ID on first run, and stores it to persistent storage. We may not want to do so in this crate, but making it possible for lava-gitlab-runner to do the same is important I think.

Yeah I've been working on a separate/follow-up patchset to move the runner initialisation to a builder pattern so users can add e.g. a custom system_id themselves more easily as well as filling in some more metadata to pass to gitlab (e.g. runner "architecture" , version, etc).. That should solve your concern here.

For reference we normally run our gitlab runners in k8s for which i think it makes sense for each instance to be unique (whether that's due to scaling or restart). As gitlab GC's stale instances after a while purely random on startup should be good enough there. Anything more fancy would need operator involvement (there is no persitant storage in the pods for these typically), which folds back to this patchset being a good default but potentially allowing the operators to override would be beneficial

README.md Show resolved Hide resolved
gitlab-runner/src/client.rs Outdated Show resolved Hide resolved
gitlab-runner/src/client.rs Outdated Show resolved Hide resolved
gitlab-runner/src/client.rs Outdated Show resolved Hide resolved
@sjoerdsimons sjoerdsimons deleted the sjoerd/systemd_id_generation branch February 17, 2024 10:09
@sjoerdsimons sjoerdsimons restored the sjoerd/systemd_id_generation branch February 17, 2024 10:11
@sjoerdsimons sjoerdsimons reopened this Feb 17, 2024
@sjoerdsimons sjoerdsimons force-pushed the sjoerd/systemd_id_generation branch 3 times, most recently from 3396f2a to 0f2c2a6 Compare February 17, 2024 13:19
@sjoerdsimons sjoerdsimons force-pushed the sjoerd/systemd_id_generation branch from 0f2c2a6 to 51d7d5f Compare February 17, 2024 13:26
gitlab-runner/src/lib.rs Outdated Show resolved Hide resolved
gitlab-runner/src/lib.rs Outdated Show resolved Hide resolved
gitlab-runner/src/client.rs Show resolved Hide resolved
gitlab-runner/src/client.rs Show resolved Hide resolved
@sjoerdsimons sjoerdsimons force-pushed the sjoerd/systemd_id_generation branch from 51d7d5f to c66e570 Compare February 17, 2024 21:13
@sjoerdsimons sjoerdsimons mentioned this pull request Feb 18, 2024
refi64
refi64 previously approved these changes Feb 28, 2024
Copy link
Collaborator

@refi64 refi64 left a comment

Choose a reason for hiding this comment

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

just threw some super minor nits, other than that LGTM

gitlab-runner/src/lib.rs Outdated Show resolved Hide resolved
gitlab-runner/src/lib.rs Outdated Show resolved Hide resolved
gitlab-runner/src/lib.rs Outdated Show resolved Hide resolved
gitlab-runner/tests/integration.rs Outdated Show resolved Hide resolved
sjoerdsimons and others added 4 commits February 28, 2024 19:47
In Recent gitlab the runners are identified not only by token but also a
system_id. This allows gitlab to differantiate between different
instances of a single runner.

Gitlab allows the system_id to be an arbitrary text strings
of up to 64 characters. However the gitlab runner implementation creates
an id that's either prefixed by s_ (for machine-id based ids) or r_ (for
random ids) followed by 12 characters with the unique id. This
implements the same algorithm to create a system_id for consistency. As
opposed to gitlab-runner no persistant storage is used to keep the crate
stateless.
The runner crate uses tracing to log to gitlab, for this a tracing layer
is provided which should be part of the global subscriber. However
tracing makes it tricky to change the global subscriber at runtime, it's
somewhat assumed to be initialized early once.

Which makes runner creation also doing the layer creation slightly
awkward as it makes it hard to start logging before or during layer
creation.

Switch this around by letting the user first create the layer to output
to gitlab and associated job list. As the layer is independant of any
configuration (it just glues tracing to jobs), this can be done very
early to start logging asap.

In principle this also makes it possible for one process to create
multiple runners, though the assumption is made job identifiers are
unique, which fundamentally constrains all runners to a single gitlab
server.

Signed-off-by: Sjoerd Simons <[email protected]>
To allow users to optionally set a custom system_id start moving the
runner creation to a builder pattern. This will also come in handy when
starting to add more version information for the runner.

Signed-off-by: Sjoerd Simons <[email protected]>
@sjoerdsimons sjoerdsimons force-pushed the sjoerd/systemd_id_generation branch from 484269f to 4afe7a1 Compare February 28, 2024 18:51
@sjoerdsimons sjoerdsimons added this pull request to the merge queue Feb 28, 2024
Merged via the queue into main with commit 3649c17 Feb 28, 2024
14 checks passed
@sjoerdsimons sjoerdsimons deleted the sjoerd/systemd_id_generation branch February 28, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants