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

Use debian-base image from kubernetes repository as base for NPD. #160

Merged
merged 1 commit into from
Feb 26, 2018
Merged

Use debian-base image from kubernetes repository as base for NPD. #160

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,18 +50,17 @@ IMAGE:=$(REGISTRY)/node-problem-detector:$(TAG)
ENABLE_JOURNALD?=1

# TODO(random-liu): Support different architectures.
BASEIMAGE:=alpine:3.4
# The debian-base:0.3 image built from kubernetes repository is based on Debian Stretch.
# It includes systemd 232 with support for both +XZ and +LZ4 compression.
# +LZ4 is needed on some os distros such as COS.
BASEIMAGE:=gcr.io/google-containers/debian-base-amd64:0.3

# Disable cgo by default to make the binary statically linked.
CGO_ENABLED:=0

# NOTE that enable journald will increase the image size.
ifeq ($(ENABLE_JOURNALD), 1)
# Enable journald build tag.
BUILD_TAGS:=-tags journald
# Use fedora because it has newer systemd version (229) and support +LZ4. +LZ4 is needed
# on some os distros such as GCI.
BASEIMAGE:=fedora
Copy link
Member

Choose a reason for hiding this comment

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

Can we only replace this one?
alpine:3.4 is ~4mb.
gcr.io/google-containers/debian-base-amd64:0.3 is ~40mb.

I feel like we should use smaller image if we don't need journald support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like we should use smaller image if we don't need journald support.

A few points I have against that:

  1. Every relevant Linux distribution uses systemd nowadays, so the point of supporting no journal is kind of moot to me...

  2. We already use debian-base-amd:0.3 as base layer for other images (such as fluentd-gcp, etc.) so reusing it might actually reduce overall disk usage since they're all sharing from the same base.

  3. Supporting two builds means this needs more testing, it's likely we might end up breaking one but not the other, which might go unnoticed as one of them is seldom or never used... Let's try to reduce overall complexity.

Having said that, if you really insist, I think it's OK to keep using Alpine for no journald... Let me know if you really want to keep Alpine for the non-journal case.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with all @filbranden 's points.

Copy link
Member

@Random-Liu Random-Liu Feb 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Every relevant Linux distribution uses systemd nowadays, so the point of supporting no journal is kind of moot to me...

We have the no-journal support because we were still using containerVM at that time.

I still think we should support no journal, because journal support:

  • need lib-systemd-dev to build;
  • will make the binary non-static.

I still prefer making it possible to have a simple statically linked binary for users who don't use journal, e.g. people may use this in LinuxKit.

As for the base image, as long as BASEIMAGE is over-writable, I'm fine with always using gcr.io/google-containers/debian-base-amd64:0.3 as the default base image to reduce the maintenance overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this PR still does what you suggested... It still keeps the ENABLE_JOURNALD variable and sets build tags appropriately in those cases. So, good to merge?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah :)

# Enable cgo because sdjournal needs cgo to compile. The binary will be dynamically
# linked if CGO_ENABLED is enabled. This is fine because fedora already has necessary
# dynamic library. We can not use `-extldflags "-static"` here, because go-systemd uses
Expand Down