-
Notifications
You must be signed in to change notification settings - Fork 2
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
Can't integrate rocks due to readOnlyRootFilesystem: true
#291
Comments
Thank you for reporting your feedback to us! The internal ticket has been created: https://warthogs.atlassian.net/browse/KF-6899.
|
After syncing with @deusebio and @jnsgruk we discussed about the following as a plan:
Also note that this is a problem because Knative Serving manifests (and Deployments) are deployed via the Knative Operator charm. Thus we can't control what the Knative Operator applies for the serving manifests, as seen in #243 (comment) Another approach would be to create a sidecar charm for each Knative Serving workload (activator, autoscaler etc) and not rely on the Knative Operator for applying the manifests for serving. But this will require a significant refactor and creating a lot of new charms. |
Thankfully, all the manifests that the Knative Operator applies for Knative Serving and Eventing are stored as yaml files, and included in the final container image: So this should be quite a straight forward change to update the source code. |
Initially I tried to do the following:
The script for generating the patch would look like this. It's goal is to load the target git tag from the #!/bin/bash
# scripts/create-readonlyfs-patch.sh
set -xe
SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
KNATIVE_OP=$SCRIPT_DIR/../knative-operator
# cleanup patches
rm $KNATIVE_OP/patches/*
# Copy the git tag from `knative-operator` rock
TARGET_TAG=$(cat $KNATIVE_OP/rockcraft.yaml | yq ".parts.operator.source-tag")
cd /tmp
git clone https://github.com/knative/operator knative-operator
cd knative-operator
git checkout $TARGET_TAG
# create the patch
find . -type f \
-exec sed -i \
"s#readOnlyRootFilesystem: true#readOnlyRootFilesystem: false#g" \
{} +
git add . && git commit -m "readOnlyRootFilesystem false"
git format-patch -1 HEAD
# 0001-readOnlyRootFilesystem-false.patch
# Copy back the patch
cd $KNATIVE_OP
cp /tmp/knative-operator/0001-readOnlyRootFilesystem-false.patch patches
# cleanup cloned repo
rm -rf /tmp/knative-operator Then the ...
copy-patch:
plugin: dump
source-type: local
source: ./patches
# exclude the patch from the final container
prime:
- -0001-readOnlyRootFilesystem-false.patch
operator:
after: [copy-patch]
plugin: go
...
override-build: |
# apply the readOnlyRootFilesystem patch
cp $CRAFT_STAGE/0001-readOnlyRootFilesystem-false.patch .
git apply 0001-readOnlyRootFilesystem-false.patch
# regular build
go mod download
... |
Yeh, and I'd just carry these as lightweight patches against the operators for now (patch at the point of build in the rock build) - rather than a full fork. I'll investigate the possibility of some "retrograde" behaviour for Pebble on read-only filesystems, too. |
But the above felt a bit of an overkill, since we can update the source code with a simple sed command. So in the end after discussing with @deusebio we'll just be updating the ...
operator:
plugin: go
...
override-build: |
# apply the readOnlyRootFilesystem patch
find . -type f \
-exec sed -i \
"s#readOnlyRootFilesystem: true#readOnlyRootFilesystem: false#g" \
{} +
# regular build
go mod download
... And with the above I confirm that we see the final Deployments have |
Yeh, that looks much better |
Bug Description
This is a follow-up of #243
We can't integrate the Knative rocks because:
readOnlyRootFilesystem: true
https://github.com/knative/operator/blob/knative-v1.12.4/cmd/operator/kodata/knative-serving/1.12.4/2-serving-core.yaml#L6254readOnlyRootFilesystem
pebble#462To Reproduce
knative-serving
and their logsEnvironment
Knative 1.12
Relevant Log Output
The same as described in canonical/pebble#462
Additional Context
This was also captured in #243 (comment), but to keep the 2 problems separated I've created this issue.
The text was updated successfully, but these errors were encountered: