Serving: Future of /var/log

[dprotaso]

@JRBANCEL From the WG meeting discussion a suggested approach could be:

1. Make /var/log optional in the runtime contract
2. Instead of dropping support in the upstream we change the behaviour to:
   2.1 When /var/log collection is enabled by the operator we keep the same behaviour as is
   2.2 When /var/log collection is disabled by the operator we _do not_ mount the read/writable /var/log volume
   2.2 We can still drop the queue proxy logic with the subpath expression but it'll require changes to the monitoring bundle (ie. the regex)

For 2.1 we could also move the logic to add the volumes to a webhook

_Originally posted by @dprotaso in https://github.com/knative/serving/pull/7751#issuecomment-621920685_

[julz]

does 2.1 mean that an operator changing this setting would cause images that work on one knative installation to fail on another?

[dprotaso]

Yeah JR pointed this out here: https://github.com/knative/serving/pull/7751#issuecomment-622011842

This is up for discussion so hence why I made the issue.

For context the motivation for this is we’re dropping support/removing the monitoring bundle

[julz]

Right just checking my understanding. I understand making the monitoring bundle optional, I guess I’m missing some context on why it’s not possible to disable it without changing which images work. Obviously if you don’t deploy the bundle you won’t see logs etc, but couldn’t we keep the image contract the same and just not collect the logs?

[dprotaso]

To support the log collection we currently always mount our own empty volume at /var/log - that overrides the image content at that mount point. So some images don’t work regardless if you collect the logs or not. ie. nginx

I don't know what’s preferred by the community at this point. So I’ve been raising awareness.

[julz]

Right, I guess it just seems like two orthogonal things:

1. Monitoring bundle disabling - doesn’t require changing image contract when that changes, we can just not collect the logs
2. Supporting images with /var/log already in them - could be an orthogonal per-revision parameter which would work _whether or not_ the monitoring bundle were deployed (so images with that set/not set work consistently regardless monitoring bundle)

[julz]

(I could also see us changing the global default to be not collecting/mounting and then letting people explicitly say if they want us to collect from /var/log, I just think it seems like if the contract changes depending on an operator rather than per-revision setting that makes it not really much of a contract :))

[JRBANCEL]

From our internal data, I can tell you this is feature is barely enabled.

It would make sense to not mount the emptyDir volume when collection is not enabled, but as mentioned earlier it is very risky because enabling it could potentially break existing Knative services (it is a global setting, it doesn't follow any orchestration or deployment strategy).

[dprotaso]

From our internal data, I can tell you this is feature is barely enabled.

It would make sense to not mount the emptyDir volume when collection is not enabled, but as mentioned earlier it is very risky because enabling it could potentially break existing Knative services (it is a global setting, it doesn't follow any orchestration or deployment strategy).

Funny enough the test image I added breaks when removing the mount since /var/log isn't a path in the image.

To @julz's point maybe the end state here is a per-revision annotation that adds the volume mount.

[evankanderson]

One other option would be to perform a mkdir -p 1777 /var/log inside the container filesystem. I'm not sure if this is something that queue-proxy should do, or it if would be better to be a separate e.g. DaemonSet.

[JRBANCEL]

One other option would be to perform a mkdir -p 1777 /var/log inside the container filesystem.

Are you suggesting to always create (and keep if it already exists) /var/log?
This solves the issue mentioned by @dprotaso above, but doesn't address the fact that enabling/disabling /var/log collection could still break thing since mounting an emptyDir erases the content of the folder where it is mounted and some images (e.g. nginx) break because they expect files to be present in that folder.

I am in favor of completely removing this feature from Knative and let users/platforms handle it.

[mattmoor]

Let's follow what we're doing for service links?

1. Create a flag to disable things, and socialize it (release notes, mailing list, tweeterz)
2. Wait; periodically reminding folks this is coming.
3. After a suitable period, flip the default on the flag.

If the plan is to wholly remove the flag, then we should also encourage folks to reach out to us starting at 1. if they have a problem with that.

[dprotaso]

From: https://github.com/knative/serving/pull/9580#discussion_r496148991

We're going to repurpose - EnableVarLogCollection as the flag and do the following

1) Only add the /var/log volume when it’s set to true
2) So people who expect it should switch it from the default (false) to on (true)
3) Update the runtime contract to say MAY

[dprotaso]

Updated the 0.18 release notes with:

Breaking EnableVarLogCollection behaviour

We always mount a emptyDir volume at /var/log in our user-containers. This impacts some popular containers images (ie. nginx) preventing them from starting.

In the next release (v0.19) we plan on changing this default behaviour and only mount a volume when EnableVarLogCollection is set to true.

Please reach out in #7881 if you have issues/comments about the approach & timeline.

[dprotaso]

Email to knative-users went out:

Hey Knative Devs/Users,

Currently, we always mount an empty volume at /var/log which gives vendors optionality to collect logs from that location. This volume mount has a side-effect of breaking some off-the-shelf images (knative/serving#3809).

After some discussion (knative/serving#7881) we settled on only adding the volume mount iff the operator has set logging.enable-var-log-collection to true in config-observability.

For operators:

1. If you're collecting logs make sure you have logging.enable-var-log-collection set to true
2. If you're not collecting logs make sure you have logging.enable-var-log-collection set to false.
3. Inform your app developers whether or not you are collecting these logs.

For application developers:

1. If your installation doesn't collect logs from /var/log please ensure your app/container does not stat/write anything to the old mount location. It will fail since it won't be present.
2. If your installation does collect logs from /var/log then no changes are required on your part.

Unless we hear otherwise on (knative/serving#7881) we hope to make this change in the next release (v0.20 - November 10, 2020).

thanks,
dave protasowski

[dprotaso]

Reminder email sent:

Just following up - I haven't heard any objection so we're continuing as planned.

thanks,
dave

[dprotaso]

Going to close this out since we haven't heard anything and the changes are in 0.19