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

Add gitlab pipeline for host injection #2941

Merged
merged 43 commits into from
Jul 24, 2023
Merged

Conversation

TonyCTHsu
Copy link
Contributor

What does this PR do?

Add gitlab pipeline for host injection

@TonyCTHsu TonyCTHsu marked this pull request as ready for review July 3, 2023 15:31
@TonyCTHsu TonyCTHsu requested a review from a team July 3, 2023 15:31
.gitlab/build-deb-rpm.sh Outdated Show resolved Hide resolved
.gitlab/build-deb-rpm.sh Show resolved Hide resolved
.gitlab/install_ddtrace_deps.rb Outdated Show resolved Hide resolved
.gitlab/install_ddtrace_deps.rb Outdated Show resolved Hide resolved
lib-injection/host_inject.rb Outdated Show resolved Hide resolved
lib-injection/host_inject.rb Show resolved Hide resolved
lib-injection/host_inject.rb Outdated Show resolved Hide resolved
lib-injection/host_inject.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,97 @@
return if ENV["DD_TRACE_SKIP_LIB_INJECTION"] == "true"
Copy link
Member

Choose a reason for hiding this comment

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

This file seems like it copies a lot over from auto_inject.rb. Which in this case... may be easier than shipping multiple files.

Nevertheless, I suggest trying to refactor the common bits so that we could keep them in-sync by a simple copy/paste, as right now they're in a similar-but-not-the-same situation, which is a slightly dangerous middle ground to be in, because we can easily introduce bugs when trying to keep them in-sync, or forget to update one of them.

@@ -0,0 +1,17 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

Is the intention here to build a rpm (or deb) with the gems already installed and the dependencies pre-compiled?

Because that usually means that we need to have variants for every Ruby supported, it's usually problematic to pick a native extension that was compiled for some Ruby version and then use it a different Ruby version.

.gitlab-ci.yml Show resolved Hide resolved
.gitlab-ci.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@randomanderson randomanderson left a comment

Choose a reason for hiding this comment

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

As far as .gitlab-ci.yml and the bash scripts are concerned, LGTM. I can't comment on the ruby code

.gitlab-ci.yml Show resolved Hide resolved
.gitlab-ci.yml Show resolved Hide resolved
.gitlab/Dockerfile-2.7.8 Show resolved Hide resolved
.gitlab/build-deb-rpm.sh Outdated Show resolved Hide resolved
| Environment| version |
|---|---|
| Ruby | `2.7`, `3.0`, `3.1`, `3.2`|
| OS | Debian 10+ |
Copy link
Member

Choose a reason for hiding this comment

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

Are we still building .rpm? Because Debian doesn't use rpm, so it looks like this list is out-of-sync with what we're actually generating?

@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/gitlab-packaging branch 8 times, most recently from ae05c87 to b93d630 Compare July 20, 2023 13:09
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Left a few notes!

image: $RUBY_CUSTOM_IMAGE_BASE/$RUBY_VERSION:current
parallel:
matrix:
- RUBY_VERSION: ["2.7.8", "3.0.6", "3.1.4"]
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Are we missing 3.2.2 here?

Copy link
Member

Choose a reason for hiding this comment

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

(If not, may be worth leaving a comment explaining why?)

@@ -0,0 +1,95 @@
#
# NOTE: THIS DOCKERFILE IS GENERATED VIA "apply-templates.sh"
Copy link
Member

Choose a reason for hiding this comment

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

Hey, I don't see this apply-templates.sh anywhere -- mind adding a note/link to where it can be found?

Comment on lines 58 to 60
FileUtils.cd(versioned_path.join("extensions/#{Gem::Platform.local.to_s}"), verbose: true) do
FileUtils.ln_sf Gem.extension_api_version, ruby_api_version
end
Copy link
Member

Choose a reason for hiding this comment

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

Maybe leave here a comment explaining why this is needed? It's very subtle :)


In order to ship `ddtrace` and its dependencies as a pre-install package, we need a few tweaks in our build pipeline.

* Use multiple custom built Ruby images to build native extensions. Those images are based on Debian `buster` to support older distribution and Ruby is compiled as a static library with `--disbale-shared` option which disables the creation of shared libraries (also known as dynamic libraries or DLLs).
Copy link
Member

Choose a reason for hiding this comment

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

Minor:

Suggested change
* Use multiple custom built Ruby images to build native extensions. Those images are based on Debian `buster` to support older distribution and Ruby is compiled as a static library with `--disbale-shared` option which disables the creation of shared libraries (also known as dynamic libraries or DLLs).
* Use multiple custom built Ruby images to build native extensions. Those images are based on Debian `buster` to support older distribution and Ruby is compiled as a static library with `--disable-shared` option which disables the creation of shared libraries (also known as dynamic libraries or DLLs).

Comment on lines 34 to 37
unless Bundler::CLI.commands["add"] && Bundler::CLI.commands["add"].options.key?("require")
debug_log "[ddtrace] You are currently using Bundler version #{Bundler::VERSION} which is not supported by host injection. Please upgrade >= 2.3, check https://github.com/rubygems/rubygems/blob/master/bundler/CHANGELOG.md#enhancements-31"
return
end
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure the link to the changelog adds a lot -- e.g. what do we want customers to learn from that link?

@TonyCTHsu TonyCTHsu merged commit 37d556d into master Jul 24, 2023
162 checks passed
@TonyCTHsu TonyCTHsu deleted the tonycthsu/gitlab-packaging branch July 24, 2023 10:40
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

I gave a final after-merging pass, and added a couple of comments :)

image: $RUBY_CUSTOM_IMAGE_BASE/$RUBY_VERSION:current
parallel:
matrix:
- RUBY_VERSION: ["2.7.8", "3.0.6", "3.1.4"]
Copy link
Member

Choose a reason for hiding this comment

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

(If not, may be worth leaving a comment explaining why?)

@@ -0,0 +1,91 @@
# This is copied from official Docker image, but compile Ruby with `--disable-shared` instead.
Copy link
Member

Choose a reason for hiding this comment

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

May be worth leaving a link here to the upstream file, so we can pick up changes in the future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants