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.

Sign up for GitHub

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

Jump to bottom

[Build] Add and Use distroless image in e2e tests #7371

Merged
merged 15 commits into from
Feb 27, 2024
Merged

[Build] Add and Use distroless image in e2e tests #7371

merged 15 commits into from
Feb 27, 2024

Conversation

ying-jeanne
Copy link
Contributor

@ying-jeanne ying-jeanne commented Feb 13, 2024 •
edited

What this PR does

  • Introduces a distroless image in Mimir.
  • Executes e2e tests against mimir-distroless.
  • During release, conducts tests on both Alpine and distroless.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@ying-jeanne ying-jeanne force-pushed the builddistroless branch 9 times, most recently from 723ad30 to a308d12 Compare February 15, 2024 15:55
@ying-jeanne ying-jeanne marked this pull request as ready for review February 15, 2024 16:39
jhesketh
jhesketh requested changes Feb 16, 2024
View reviewed changes
.github/workflows/test-build-deploy.yml Outdated Show resolved Hide resolved
.github/workflows/test-build-deploy.yml Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
integration/api_endpoints_test.go Outdated Show resolved Hide resolved
@ying-jeanne ying-jeanne force-pushed the builddistroless branch 7 times, most recently from 0e374d9 to d30470c Compare February 16, 2024 14:57
pstibrany
pstibrany reviewed Feb 16, 2024
View reviewed changes
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you, this looks pretty good. I've left some suggestions.

.github/workflows/test-build-deploy.yml Show resolved Hide resolved
.github/workflows/test-build-deploy.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
cmd/mimir/Dockerfile.distroless Outdated Show resolved Hide resolved
@pstibrany
Copy link
Member

I find it ironic that "distroless" image using gcr.io/distroless/base-nossl-debian12 as base is larger than our alpine-based image :)

us.gcr.io/grafanalabs-dev/mimir                             builddistroless-7e26dced8                                                    370fcc836755   19 minutes ago   77.2MB
us.gcr.io/grafanalabs-dev/mimir                             distroless-builddistroless-7e26dced8                                         c24df8f8be35   19 minutes ago   93.1MB

@ying-jeanne ying-jeanne force-pushed the builddistroless branch 2 times, most recently from 6bef566 to 6dc3fde Compare February 19, 2024 11:49
ying-jeanne and others added 2 commits February 21, 2024 10:37
@ying-jeanne @jhesketh
Update .github/workflows/test-build-deploy.yml
8bb89da 
Co-authored-by: Joshua Hesketh <joshua.hesketh@grafana.com>
@ying-jeanne @jhesketh
Update .github/workflows/scripts/run-integration-tests-group.sh
22ec9fb 
Co-authored-by: Joshua Hesketh <joshua.hesketh@grafana.com>
jhesketh
jhesketh approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

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

lgtm, thank you !

jhesketh
jhesketh previously requested changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

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

We have missed a part of how the images are pushed that will currently not work with this approach. I am going to change to using a different image as opposed to tag for distroless

pstibrany
pstibrany reviewed Feb 21, 2024
View reviewed changes
.github/workflows/scripts/run-integration-tests-group.sh Outdated Show resolved Hide resolved
.github/workflows/scripts/run-integration-tests-group.sh Outdated Show resolved Hide resolved
.github/workflows/test-build-deploy.yml Outdated Show resolved Hide resolved
.github/workflows/test-build-deploy.yml Outdated Show resolved Hide resolved
.github/workflows/test-build-deploy.yml Outdated Show resolved Hide resolved
.github/workflows/test-build-deploy.yml Outdated Show resolved Hide resolved
.github/workflows/test-build-deploy.yml Outdated Show resolved Hide resolved
@jhesketh
Return to using a separate image name rather than tag
1eca65c 
This simplifies working with the existing build-images and push-images
scripts.

This is temporary until we switch over to distroless as default.
@jhesketh
Split matrix into two separate steps
6cdcede 
It's clearer even if duplicated. This is temporary anyway.
@jhesketh
Remove unused variable
4981815
@jhesketh
Fix job dependencies
0b64c18
@jhesketh
Copy link
Contributor

For this to work we'll either need to keep the name integration (instead of integration-distroless), or update the repo settings of which checks are "required".

@pstibrany
Copy link
Member

For this to work we'll either need to keep the name integration (instead of integration-distroless), or update the repo settings of which checks are "required".

Oh, good point. With matrix approach, when both integration sets (for distroless and alpine images) are running in a single job, this is not a problem, right?

@pstibrany
Copy link
Member

Oh, good point. With matrix approach, when both integration sets (for distroless and alpine images) are running in a single job, this is not a problem, right?

Oh, of course it is. We specify job names that must succeed. I'd say we just update the repo settings, and ask all PR owners to rebase their branches.

@jhesketh
Copy link
Contributor

Oh, good point. With matrix approach, when both integration sets (for distroless and alpine images) are running in a single job, this is not a problem, right?

Oh, of course it is. We specify job names that must succeed. I'd say we just update the repo settings, and ask all PR owners to rebase their branches.

Yep, or we can just keep the name integration to avoid rebasing (as I hadn't considered that). I'm open to either.

@jhesketh
Copy link
Contributor

As I've now authored a significant part of this, I don't think I should review it. @pstibrany , if you don't mind having another look at this approach that would be appreciated.

@ying-jeanne ying-jeanne dismissed jhesketh’s stale review February 26, 2024 08:46

joshua is also the author of the PR

@pstibrany
Copy link
Member

Yep, or we can just keep the name integration to avoid rebasing (as I hadn't considered that). I'm open to either.

That would be easiest solution. integration for distroless images, and integration-alpine until we get rid of it.

pstibrany
pstibrany reviewed Feb 26, 2024
View reviewed changes
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

We will need to ask for mimir-distroless repository before we can merge this. (Asked @jhesketh privately)

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
cmd/mimir/Dockerfile.distroless Show resolved Hide resolved
@ying-jeanne
Copy link
Contributor Author

We will need to ask for mimir-distroless repository before we can merge this. (Asked @jhesketh privately)

i don't have editor permission, opened it help ticket for this.

@ying-jeanne
Copy link
Contributor Author

mimir-distroless created https://hub.docker.com/repository/docker/grafana/mimir-distroless/general

@ying-jeanne
Copy link
Contributor Author

@pstibrany could you take another look, thanks

pstibrany
pstibrany approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Great work, thank you!

Makefile Outdated Show resolved Hide resolved
@ying-jeanne ying-jeanne merged commit eb09921 into main Feb 27, 2024
29 checks passed
@ying-jeanne ying-jeanne deleted the builddistroless branch February 27, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstibrany pstibrany pstibrany approved these changes

@wilfriedroset wilfriedroset Awaiting requested review from wilfriedroset wilfriedroset is a code owner

@vaxvms vaxvms Awaiting requested review from vaxvms vaxvms is a code owner

@bubu11e bubu11e Awaiting requested review from bubu11e bubu11e is a code owner

@jhesketh jhesketh Awaiting requested review from jhesketh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ying-jeanne @pstibrany @jhesketh