Switch from /tmp to /run to DP_CONF default path#719
Conversation
d6e1b60 to
1545698
Compare
PlagueCZ
left a comment
There was a problem hiding this comment.
I am unable to currently test this in OSC, but it seems that I can just define DP_CONF and make it compatible with current production setup, so it looks fine.
You are missing changes to config/default/dp-service.yaml where /tmp is still mounted but /run/dpservice is not. This will also remove the need for doing a mkdir inside the script, because the directoyr will be craeted by kubernetes.
hack/prepare.sh
Outdated
| conf_vf_pattern="$(get_pattern "${devs[0]}")" | ||
| conf_ipv6="$(get_ipv6)" | ||
|
|
||
| mkdir -p "$(dirname "$CONFIG")" |
There was a problem hiding this comment.
While I totally understand the need for creating the directory somewhere, and the fact this is the simplest way to do it, I am wary of situations where DP_CONF is used. Then this script would call mkdir -p $(dirname ) on an arbitrary quoted string and do it as root.
Not saying it's a big problem, but I would like to mention it, because it make me feel uneasy at first glance, may be just paranoia.
There was a problem hiding this comment.
That's definitely a valid point. The alternative I see is something like:
if ! test -d "$(dirname "$CONFIG")"; then
echo "error: config directory does not exist"
exit 1
fi
But for automated deployments you would then need some custom script to setup the directory first since /run is wiped on reboots.
Edit: just saw the other comment, that sounds like we would be safe to assume that the directory exists? Sorry, I'm not yet that familiar with the deploy process of dpservice.
There was a problem hiding this comment.
The change(s) in config/default/dp-service.yaml is missing. If you specify the hostPath in deployment yaml of the dpservice with type DirectoryOrCreate then the kubelet would create the directory, if it doesnt exist and you can eliminate the "mkdir" in the script which tries to create the directory with arbitrary given string.
dpservice/config/default/dp-service.yaml
Line 103 in 786ca5e
There was a problem hiding this comment.
@maxmoehl in case you missed the last comment.
There was a problem hiding this comment.
I've fixed the kubernetes manifest, please check if got all the occurrences.
The script now assumes that the directory exists, the echo forwarding will fail if the directory does not exist as the script is not explicitly checking that.
On modern debian-based systems files may be cleared from /tmp after some time. The recommended location for files which should be kept as long as the system is running is the /run directory. This commit changes the relevant references to load the config file from /run/dpservice/dpservice.conf by default instead of /tmp/dp_service.conf. The old behavior can be restored by setting DP_CONF to /tmp/dp_service.conf. The hack/prepare.sh was updated to respect the DP_CONF environemnt variable as well. Resolves: #717
1545698 to
221566f
Compare
PlagueCZ
left a comment
There was a problem hiding this comment.
Looks great now.
I should test in OSC, but I would need to prepare three overlay YAMLs we have (we have three separate configs) and the environment is somewhat flaky at this point, and I am short on time, so let's trust how it looks.
On modern debian-based systems files may be cleared from /tmp after some
time. The recommended location for files which should be kept as long as
the system is running is the /run directory.
This commit changes the relevant references to load the config file from
/run/dpservice/dpservice.conf by default instead of /tmp/dp_service.conf.
The old behavior can be restored by setting DP_CONF to
/tmp/dp_service.conf.
The hack/prepare.sh was updated to respect the DP_CONF environemnt
variable as well.
Resolves: #717