Merge ~raharper/cloud-init:systemd_export_tz into cloud-init:master

[Ryan Harper]

Export TZ environment variable to systemd units

When TZ is unset in the execution environment for cloud-init, glibc
behavior of strftime will check if /etc/localtime is available via
a stat syscall. During a normal cloud-init boot, we execute roughly
400+ stat calls to /etc/localtime. Exporting TZ into the environment
prevents these stats syscalls.

[Scott Moser (smoser)]

an alternative implementation of this would be for cloud-init to read TZ from os.environ, and if not set, then it could set it in os.environ to :/etc/localtime .

Not sure which i prefer.

[Ryan Harper (raharper)]

That won;t fix python interpreter from fstat'ing though

On Tue, Oct 18, 2016 at 2:51 PM, Scott Moser <email address hidden> wrote:

> an alternative implementation of this would be for cloud-init to read TZ
> from os.environ, and if not set, then it could set it in os.environ to
> :/etc/localtime .
>
> Not sure which i prefer.
>
> --
> https://code.launchpad.net/~raharper/cloud-init/+git/
> cloud-init/+merge/307722
> You are the owner of ~raharper/cloud-init:systemd_export_tz.
>

[Scott Moser (smoser)]

Are hyou sure it wouldn't fix it?
changing os.environ should change the environment of the process that is running, which (I'd think) would have the same result. Unless python is doing all those stats before we could set it (it may well do some).

I couldn't reproduce any difference with set or unset with a simple program like this:
import os, time
if 'TZ' not in os.environ:
    os.environ['TZ'] = ":/etc/localtime"
print(time.strftime("%a, %d %b %Y %T %z"))

And then running with:
  env -i TZ strace python program.py

I'm not opposed to this, and dont want to discount the performance improvement, but if we can do it in a single place, i'd rather that then in multiple systemd files which would only benefit systemd.

[Ryan Harper (raharper)]

On Fri, Nov 4, 2016 at 4:08 AM, Scott Moser <email address hidden> wrote:

> Are hyou sure it wouldn't fix it?
> changing os.environ should change the environment of the process that is
> running, which (I'd think) would have the same result. Unless python is
> doing all those stats before we could set it (it may well do some).
>

updating os.environ will help in that cloud-init drives lots of python
logging with timestamps. However, there are other modules which may run as
forked processes under our unit which will NOT have it; and it's literally
any program making the glibc call.

>
> I couldn't reproduce any difference with set or unset with a simple
> program like this:
> import os, time
> if 'TZ' not in os.environ:
> os.environ['TZ'] = ":/etc/localtime"
> print(time.strftime("%a, %d %b %Y %T %z"))
>
> And then running with:
> env -i TZ strace python program.py
>

 (foudres) ~ % env -i strace /usr/bin/python3 -c 'import os; from datetime
import datetime; print(datetime.now())' 2>&1 | egrep
"(stat.*etc/localtime|zoneinfo)"
stat("/etc/localtime", {st_mode=S_IFREG|0644, st_size=2221, ...}) = 0
stat("/etc/localtime", {st_mode=S_IFREG|0644, st_size=2221, ...}) = 0

(foudres) ~ % strace /usr/bin/python3 -c 'import os; from datetime import
datetime; print(datetime.now())' 2>&1 | egrep
"(stat.*etc/localtime|zoneinfo)"
open("/usr/share/zoneinfo/America/Chicago", O_RDONLY|O_CLOEXEC) = 3

>
> I'm not opposed to this, and dont want to discount the performance
> improvement, but if we can do it in a single place, i'd rather that then in
> multiple systemd files which would only benefit systemd.
>

cloud-init can write a single EnvironmentFile= and we can specify that in
each Unit file
showed this here and Steve and Martin were interested in having that maybe
set as systemd default.

however, for cloud-init which may run in *other* distro environments with
systemd, we really should bring our own.

>
>
> --
> https://code.launchpad.net/~raharper/cloud-init/+git/
> cloud-init/+merge/307722
> You are the owner of ~raharper/cloud-init:systemd_export_tz.
>

[Scott Moser (smoser)]

$ ./go
lrwxrwxrwx 1 root root 36 Nov 7 08:43 /etc/localtime -> /usr/share/zoneinfo/America/New_York
== -u TZ ==
open("/etc/localtime", O_RDONLY|O_CLOEXEC) = 3
stat("/etc/localtime", {st_mode=S_IFREG|0644, st_size=3545, ...}) = 0
stat("/etc/localtime", {st_mode=S_IFREG|0644, st_size=3545, ...}) = 0
2016-11-21 11:12:25.758464

== TZ=US/Eastern ==
open("/usr/share/zoneinfo/US/Eastern", O_RDONLY|O_CLOEXEC) = 3
2016-11-21 11:12:25.855751

== TZ=Invalid ==
open("/usr/share/zoneinfo/Invalid", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
2016-11-21 16:12:25.943840

== TZ= ==
open("/usr/share/zoneinfo/Universal", O_RDONLY|O_CLOEXEC) = 3
2016-11-21 16:12:26.024612

== TZ=:/etc/localtime ==
open("/etc/localtime", O_RDONLY|O_CLOEXEC) = 3
2016-11-21 11:12:26.108717

== TZ=:/etc/localtime.bogus ==
open("/etc/localtime.bogus", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
2016-11-21 16:12:26.191718

$ cat go
#!/bin/sh
runit() { "$@" python3 -c 'from datetime import datetime; print(datetime.now())'; }
ls -l /etc/localtime
for p in "-u TZ" "TZ=US/Eastern" "TZ=Invalid" TZ= "TZ=:/etc/localtime" "TZ=:/etc/localtime.bogus"; do
   echo == $p ==
   runit env $p strace 2>&1 | egrep "((open|stat).*etc/localtime|zoneinfo)"
   runit env $p
   echo
done

[Scott Moser (smoser)]

So what I think the above shows is that having TZ not set at all is the worst possible result.
Setting it to either a bogus value or :/etc/localtime or and empty value all result in a single open attempt.

Interestinglym, though, i could not see any difference based on TZ using 'ls -l /etc/passwd' as the program executed by 'runit'. For all values of TZ above, ls -l /etc/passwd will do a single 'open'.

Thus, I think this is python that is doing the subsequent stat and not something glibc is doing.

[Scott Moser (smoser)]

based on the findings above/my understanding, this should be equivalent to your suggested change:

--- a/cloudinit/cmd/main.py
+++ b/cloudinit/cmd/main.py
@@ -694,4 +694,6 @@ def main(sysv_args=None):

 if __name__ == '__main__':
+ if 'TZ' not in os.environ:
+ os.environ[TZ] = ":/etc/localtime"
     main(sys.argv)

python is doing the additional stats' I suspect with the intent of being more dynamic / allowing an update to /etc/TZ to affect subsequent calls to datetime() .

[Ryan Harper (raharper)]

On Mon, Nov 21, 2016 at 10:32 AM, Scott Moser <email address hidden> wrote:

> based on the findings above/my understanding, this should be equivalent to
> your suggested change:
>
> --- a/cloudinit/cmd/main.py
> +++ b/cloudinit/cmd/main.py
> @@ -694,4 +694,6 @@ def main(sysv_args=None):
>
>
> if __name__ == '__main__':
> + if 'TZ' not in os.environ:
> + os.environ[TZ] = ":/etc/localtime"
> main(sys.argv)
>
>
>
Yes, I think that's reasonable, and makes it portable across init systems.

>
> python is doing the additional stats' I suspect with the intent of being
> more dynamic / allowing an update to /etc/TZ to affect subsequent calls to
> datetime() .
>
>
> --
> https://code.launchpad.net/~raharper/cloud-init/+git/
> cloud-init/+merge/307722
> You are the owner of ~raharper/cloud-init:systemd_export_tz.
>

[Scott Moser (smoser)]

merged in the suggestion above
--- a/cloudinit/cmd/main.py
+++ b/cloudinit/cmd/main.py
@@ -694,4 +694,6 @@ def main(sysv_args=None):

 if __name__ == '__main__':
+ if 'TZ' not in os.environ:
+ os.environ['TZ'] = ":/etc/localtime"
     main(sys.argv)

[Scott Moser (smoser)]

For reference
https://blog.packagecloud.io/eng/2017/02/21/set-environment-variable-save-thousands-of-system-calls/

not that I'll ever find this MP again, but if i ever did...

diff --git a/systemd/cloud-config.service b/systemd/cloud-config.service
index 3309e08..411b2b0 100644
--- a/systemd/cloud-config.service
+++ b/systemd/cloud-config.service
@@ -5,6 +5,7 @@ Wants=network-online.target cloud-config.target
 
 [Service]
 Type=oneshot
+Environment=TZ=:/etc/localtime
 ExecStart=/usr/bin/cloud-init modules --mode=config
 RemainAfterExit=yes
 TimeoutSec=0
diff --git a/systemd/cloud-final.service b/systemd/cloud-final.service
index b8f69b7..b740dac 100644
--- a/systemd/cloud-final.service
+++ b/systemd/cloud-final.service
@@ -5,6 +5,7 @@ Wants=network-online.target cloud-config.service
 
 [Service]
 Type=oneshot
+Environment=TZ=:/etc/localtime
 ExecStart=/usr/bin/cloud-init modules --mode=final
 RemainAfterExit=yes
 TimeoutSec=0
diff --git a/systemd/cloud-init-local.service b/systemd/cloud-init-local.service
index 55834ba..1459703 100644
--- a/systemd/cloud-init-local.service
+++ b/systemd/cloud-init-local.service
@@ -12,6 +12,7 @@ Conflicts=shutdown.target
 
 [Service]
 Type=oneshot
+Environment=TZ=:/etc/localtime
 ExecStart=/usr/bin/cloud-init init --local
 ExecStart=/bin/touch /run/cloud-init/network-config-ready
 RemainAfterExit=yes
diff --git a/systemd/cloud-init.service b/systemd/cloud-init.service
index bbddb2d..1b12b04 100644
--- a/systemd/cloud-init.service
+++ b/systemd/cloud-init.service
@@ -18,6 +18,7 @@ Conflicts=shutdown.target
 
 [Service]
 Type=oneshot
+Environment=TZ=:/etc/localtime
 ExecStart=/usr/bin/cloud-init init
 RemainAfterExit=yes
 TimeoutSec=0