Merge ~oddbloke/cloud-init/+git/cloud-init:lp1843276 into cloud-init:master

[Dan Watkins]

atomic_helper: add DEBUG logging to write_file

LP: #1843276

[Scott Moser (smoser)]

Your successful log path would not even include the final location for the file, which is ultimately the useful thing (except in the write-failed case, which is probably rare).

I think I'd prefer logging with a message that included the target file (filename), not just the temporary file name. And hopefully a way to differentiate in the log message between atomic_helpers.write_file and write_file.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:cea88e21a22c752bfd268e1ca49fada2858ab858
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1109/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1109//rebuild

review: Needs Fixing (continuous-integration)

[Dan Watkins (oddbloke)]

Oh yeah, great points. Will fix.

[Dan Watkins (oddbloke)]

OK, I've updated the log message to indicate we're writing to a temporary file, and added a log message for the rename (because I think failures could happen in either of those steps, and this allows us to distinguish between those). (We will also have the different logger name to distinguish between the two log messages, which is why I didn't push even more context into the log message itself.)

Thanks for the review, Scott!

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:ae70b6bcef3002b3ed7318faba23df4dda2d188b
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1110/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1110//rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

In the interest of making "cloud-init debug is way to loud" even worse, Could one log message be enough?

"Atomic writing to %s via temporary %s", filename, tf.name

If you see such a log and its not followed by a stack trace, then its good.

[Dan Watkins (oddbloke)]

Updated.

(CI failures are due to an infra problem, which we've hopefully just fixed.)

[Scott Moser (smoser)]

sorry for round trips.

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:a32846af474f078df0f6f56196a754d5b79f079f
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1111/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    FAILED: Ubuntu LTS: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1111//rebuild

review: Needs Fixing (continuous-integration)

[Dan Watkins (oddbloke)]

On Mon, Sep 09, 2019 at 04:30:16PM -0000, Scott Moser wrote:
> sorry for round trips.

No worries, I'd like to get this right!

> Diff comments:
>
> > diff --git a/cloudinit/atomic_helper.py b/cloudinit/atomic_helper.py
> > index 587b994..8770193 100644
> > --- a/cloudinit/atomic_helper.py
> > +++ b/cloudinit/atomic_helper.py
> > @@ -23,6 +25,10 @@ def write_file(filename, content, mode=_DEF_PERMS,
> > try:
> > tf = tempfile.NamedTemporaryFile(dir=os.path.dirname(filename),
> > delete=False, mode=omode)
> > + LOG.debug(
> > + "Atomically writing to file %s (via temporary file %s) - %s: [%s]"
> > + " %s bytes/chars",
>
> %s formats terribly here for 'mode' :-(.
> >>> "%s %d" % (0o644, 9)
>
> You want '%o' for that, otherwise you'll get some confusing non-sense of '420' as the mode.

TIL about %o. Thanks! Changed.

> Then, since you have to change that anyway... can we use '%d' for the bytes/chars ?
>
> I realize i'm probably the worst offender here, but len(content) is guaranteed to be an int.

Also changed.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:a32846af474f078df0f6f56196a754d5b79f079f
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1112/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1112//rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I adjusted your commit message in the MP to drop 'Fixes: ' from the last line.

review: Approve

[Dan Watkins (oddbloke)]

Thanks! I knew something about it looked weird but I could not figure it out. :)

[Server Team CI bot (server-team-bot)]

Autolanding: FAILED
More details in the following jenkins job:
https://jenkins.ubuntu.com/server/job/cloud-init-autoland-test/316/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:46e6ee541bf4cc7943d76db95f79fb145c7a2226
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1114/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1114//rebuild

review: Approve (continuous-integration)

diff --git a/cloudinit/atomic_helper.py b/cloudinit/atomic_helper.py
index 587b994..1f61faa 100644
--- a/cloudinit/atomic_helper.py
+++ b/cloudinit/atomic_helper.py
@@ -1,11 +1,13 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 import json
+import logging
 import os
 import stat
 import tempfile
 
 _DEF_PERMS = 0o644
+LOG = logging.getLogger(__name__)
 
 
 def write_file(filename, content, mode=_DEF_PERMS,
@@ -23,6 +25,10 @@ def write_file(filename, content, mode=_DEF_PERMS,
     try:
         tf = tempfile.NamedTemporaryFile(dir=os.path.dirname(filename),
                                          delete=False, mode=omode)
+        LOG.debug(
+            "Atomically writing to file %s (via temporary file %s) - %s: [%o]"
+            " %d bytes/chars",
+            filename, tf.name, omode, mode, len(content))
         tf.write(content)
         tf.close()
         os.chmod(tf.name, mode)