-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: build binaries and Docker images in CI #24751
Conversation
4c4e18f
to
5858411
Compare
a3c905a
to
2d78d18
Compare
Okay this is open for review. Before merging I will need to set the CI to only run only on release tags. If you want to test the builds yourself let me know internally on slack and I can get you access to the docker builds and the binary builds. |
I'm having some issues with the defaults I've set for docker via env vars (something about permission denied for the fs) that I don't get just running the binary itself which is a docker issue with how I'm running it I think. Otherwise I think we're in a great spot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mgattozzi - this is a huge lift. I only had a couple questions/comments, but otherwise LGTM. Would probably be good to get @bnpfeife's approval as well.
Hey, I've created a PR that resolves the build issue. Though I'm out of time tonight to test it. |
973c1ce
to
eb49cb4
Compare
I removed the files, included @bnpfeife's changes for making the directory, and I've exposed the right port as well as setting the proper default address to 0.0.0.0 so that docker containers and deployments of influxdb should work as expected. I tagged you for another rereview @hiltontj given there is a code change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
With @hiltontj's changes to where |
.circleci/packages/config.yaml
Outdated
match: '^v[0-9]+.[0-9]+.[0-9]+' | ||
value: '{{env.CIRCLE_TAG[1:]}}' | ||
default: | ||
value: '3.x-{{env.CIRCLE_SHA1[:8]}}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This default value is difficult to work with. Consider:
$ dpkg --compare-versions 3.0.0 lt 3.0.1 && echo yes # good
yes
$ dpkg --compare-versions 3.0.0 lt 3.x-0123abcd && echo yes # maybe not good?
yes
$ dpkg --compare-versions 3.x-gfed9876 lt 3.x-0123abcd && echo yes # but what if 3.x-0123abcd was created before 3.x-gfed9876?
[1]
Part of the reason why I'm commenting on this is that we are uploading to influxdb/releases/influxdb3
, but these aren't releases, they're snapshots. Package managers are going to have trouble dealing with these versions. I would advocate that the influxdb/releases/influxdb3
only contain things that are considered a release and use something else for snapshots or nightlies.
Fyi, telegraf
has telegraf/nightlies/telegraf_nightly_amd64.deb
, which gets overwritten every night. Maybe that isn't what you want right now though (ie, perhaps it is useful to have different snapshots).
As a strawperson, I suggest that:
- someone creates
influxdb/snapshots/influxdb3
and move everything that was uploaded toinfluxdb/releases/influxdb3
there. New snapshots go toinfluxdb/snapshots/influxdb3
and use3.0.0+snapshot-{{env.CIRCLE_SHA1[:8]}}
as the default. - if one of the current uploads was an mvp, tag it with
3.0.0-0.alpha.0.0
, rebuild it and put the rebuilt artifacts ininfluxdb/releases/influxdb3
. For important internal pre-alpha releases, you can tag with3.0.0-0.alpha.0.1
,3.0.0-0.alpha.0.2
, etc. When official alpha 1 is released, tag with3.0.0-0.alpha.1
, then use3.0.0-0.alpha.2
for 2nd official alpha, and so on. Continue with3.0.0-0.beta.1
,3.0.0-0.rc.1
as desired. This versioning scheme works with both semver and packaging. Ie:3.0.0-0.alpha.0.0
<3.0.0-0.alpha.0.1
<3.0.0-0.alpha.0.2
<3.0.0-0.alpha.1
<3.0.0-0.alpha.2
<3.0.0-0.beta.1
<3.0.0-0.rc.1
. All of these tagged releases go toinfluxdb/releases/influxdb3
- when 3.0.0 is released, tag as
3.0.0
. semver is happy since3.0.0-0.rc.1
<3.0.0
and packaging is updated to use3.0.0-1
such that3.0.0-0.rc.1
<3.0.0-1
In the above, we don't worry if 3.0.0+snapshot-{{env.CIRCLE_SHA1[:8]}}
is higher or lower in packaging since people will be installing those manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely a good point. I may go ahead and update these for 1.X OSS/Enterprise and 2.X OSS. As you alluded to, the default version schemes weren't intended to be consumed by the general public. So I had just written them to make apt
and yum
happy. However, I definitely can foresee a future where someone makes these available to the general public (not here, but in 1.X and 2.X land).
Hey, my apologies that I haven't looked at this for some time! I had left some comments but they hadn't appeared in the conversation part of the PR until I started a review. I updated/removed one of my review comments and added another so it aligns better with what @jdstrand suggested. Looking through it, I think everything is just minor changes. Edit: Oh, my comments were "Pending". GitHub should really let you know that those aren't visible. |
.circleci/config.yml
Outdated
commands: | ||
install_rust: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think that this can be removed.
.circleci/packages/config.yaml
Outdated
match: '^v[0-9]+.[0-9]+.[0-9]+' | ||
value: '{{env.CIRCLE_TAG[1:]}}' | ||
default: | ||
value: '3.x-{{env.CIRCLE_SHA1[:8]}}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely a good point. I may go ahead and update these for 1.X OSS/Enterprise and 2.X OSS. As you alluded to, the default version schemes weren't intended to be consumed by the general public. So I had just written them to make apt
and yum
happy. However, I definitely can foresee a future where someone makes these available to the general public (not here, but in 1.X and 2.X land).
publish-packages: | ||
docker: | ||
- image: cimg/python:3.6 | ||
steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're publishing "snapshot" packages, let's change the destination depending on the release type:
publish-packages:
docker:
- image: cimg/python:3.6
+ parameters:
+ # "destination" should be one of:
+ # - releases
+ # - nightlies
+ # - snapshots
+ destination:
+ - type: string
steps:
- attach_workspace:
at: /tmp/workspace
- aws-s3/sync:
arguments: --acl public-read
aws-region: RELEASE_AWS_REGION
aws-access-key-id: RELEASE_AWS_ACCESS_KEY_ID
aws-secret-access-key: RELEASE_AWS_SECRET_ACCESS_KEY
from: /tmp/workspace/artifacts
- to: s3://dl.influxdata.com/influxdb/releases
+ to: s3://dl.influxdata.com/influxdb/<< parameters.destination >>
The workflows will need to be modified to pass destination
along to the jobs.
eb49cb4
to
b2d4bce
Compare
I'll still need to address all of these. I did get the branch rebased to latest main and conflicts fixed. I'll get to these early next week when I'm back from the solar eclipse viewing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these changes! See inline.
.circleci/packages/config.yaml
Outdated
match: '^v[0-9]+.[0-9]+.[0-9]+' | ||
value: '{{env.CIRCLE_TAG[1:]}}' | ||
alpha: | ||
match: '^v3.0.0.alpha.[0-9]+.[0-9]+' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be ^v3.0.0-0.alpha.[0-9]+.[0-9]+
(the -0
is important for package managers).
# We perform ownership change only for Debian-based systems. | ||
# Moving these lines out of this if statement would make `rmp -V` fail after installation. | ||
chown -R -L influxdb:influxdb $LOG_DIR | ||
chown -R -L influxdb:influxdb $DATA_DIR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In pre-install
you define:
USER=influxdb3
GROUP=influxdb3
It would be nice to do that here.
if [[ -f /etc/debian_version ]]; then | ||
# Ownership for RH-based platforms is set in build.py via the `rmp-attr` option. | ||
# We perform ownership change only for Debian-based systems. | ||
# Moving these lines out of this if statement would make `rmp -V` fail after installation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AIUI, this is saying that on Debian-based systems, unconditionally chown and chmod the LOG_DIR and DATA_DIR. Does this happen on upgrades or just new installs? Why is this needed if post-install is already creating the directories correctly. This feels like something cargo-culted in from other packaging that was meant to fix a permissions problem for the other package that doesn't apply here, but maybe I'm missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a straight copy from oss 2.x so maybe @bnpfeife would know more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, unfortunately, this is something else that has existed before I started working here. My suspicion is that someone was having issues with directory permissions being too restrictive. They probably complained so the "solution" was to always reset the permissions in post-inst
. I think since we're starting anew, we should remove this. Mostly because the packaging should attempt to respect the local configuration whenever possible.
Dockerfile
Outdated
|
||
# TODO: Make this and other env vars not specific to IOx | ||
ENV INFLUXDB_IOX_OBJECT_STORE=file | ||
ENV INFLUXDB_IOX_DB_DIR=/usr/local/share/influxdb3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: /usr/local/share
is an odd locations for database files. I would expect /var/lib/influxdb3
in the Dockerfile too. Why this path? Presumably this is a mount point for a writable dir, why not make it in a more standard location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember, I think I wasn't sure or I was copying what we did in the other OSS bits. Happy to make it /var/lib.
There are still some things that need addressing but am waiting on @bnpfeife's feedback to implement them correctly. |
Hey @jdstrand, I've been looking into updating
I mentioned in out Slack that ci-packager executes fpm. fpm then modifies the version string before passing it to rpmbuild or dpkg-buildpackage. So, I experimented with calling rpmbuild and dpkg-buildpackage from ci-packager instead. Ultimately, I ended up consulting both the debian and redhat versioning guidelines quite a bit. Both packaging formats consider the hyphen a separator between the "upstream version" and "package version". These components have slightly different names in the actual version specifications. However, the purpose of each component remains largely the same. I can definitely make hyphens work, but I'm not sure that I like the implications. I interpret the "release version" ( It looks like With all of that being said, I'm wondering if it would be easier just to use # debian version comparisons
$ dpkg --compare-versions '1.0.0' gt '1.0.0~alpha0' # exit-code 0
$ dpkg --compare-versions '1.0.0' gt '1.0.0+snapshot0' # exit-code 1
# redhat version comparisons
$ rpmdev-vercmp '1.0.0' '1.0.0~alpha0'
1.0.0 > 1.0.0~alpha0
$ rpmdev-vercmp '1.0.0' '1.0.0+snapshot0'
1.0.0 < 1.0.0+snapshot0 |
Not having hyphens has packaging implications. In Debian, packages without hyphens are interpreted as Debian 'native' packages that come from Debian itself (eg,
Indeed.
The problem is that in addition to deb and rpm packaging, I was trying to also reconcile semver versioning, and it doesn't allow a
Yes, the alpha, beta and rc versions have rather verbose version, but it works from a packaging perspective and alpha, beta and rc builds are for internal use (eg, testing) so I wanted to also optimize for That leaves In the end, the package versioning I chose is a compromise that:
|
From my perspective, these are the changes:
config.yaml still has:
This should be changed to:
I believe that @bnpfeife fixed these outside of this PR? @bnpfeife, can you comment? |
40e2e45
to
1d427c2
Compare
Okay I pushed those changes @jdstrand. Fixed the cargo deny issue and rebased off main to make sure CI passes. If this all looks good, then I'll add the correct filters so it won't run unless we want to do a release before we merge |
1d427c2
to
1b7cd19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the portions I reviewed before, LGTM. I noticed a new file foo
that seems like a mistake and have a question about the listening port inline. In the interest of time, I'm going to approve, but please remove the foo
file and consider my question about the port.
Please have @bnpfeife also give it a once over.
foo
Outdated
@@ -0,0 +1 @@ | |||
38fd39f0659ac8406de0e8f83c64eb0a15ac0152a8198467407ddb56c2a6e684 /home/michael/1Password Emergency Kit A3-CP6WC8-team-influxdata.pdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover from me testing stuff with sha256sum. I thought I had deleted it, but I think I did that in the web UI and forgot to pull in those changes before a push 🤦
@@ -54,7 +57,12 @@ ENV PACKAGE=$PACKAGE | |||
COPY --from=build "/root/$PACKAGE" "/usr/bin/$PACKAGE" | |||
COPY docker/entrypoint.sh /usr/bin/entrypoint.sh | |||
|
|||
EXPOSE 8080 8082 | |||
EXPOSE 8181 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be port 8086?
Hey, @jdstrand @mgattozzi I created a PR into this branch that uses a new version of |
@bnpfeife I've merged it here and if we need any more changes as follow up I can do them here, but I think this will get us over the edge. |
Okay tomorrow I'm going to do some rebasing again, put the correct filters on, and then merge! |
For releases we need to have Docker images and binary images available for the user to actually run influxdb3. These CI changes will build the binaries on a release tag and the Docker image as well, test, sign, and publish them and make them available for download. Co-Authored-By: Brandon Pfeifer <bpfeifer@influxdata.com>
* fix: use new packager to correct versions * chore: delete foo
f31ac6e
to
5484819
Compare
feat: build binaries and Docker images in CI
For releases we need to have Docker images and binary images available for the
user to actually run influxdb3. These CI changes will build the binaries on a
release tag and the Docker image as well, test, sign, and publish them and make
them available for download.
Co-Authored-By: Brandon Pfeifer bpfeifer@influxdata.com
Closes #24773