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

Proposed by Dan Watkins on 2019-09-09
Status: Merged
Approved by: Dan Watkins on 2019-09-09
Approved revision: 46e6ee541bf4cc7943d76db95f79fb145c7a2226
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~daniel-thewatkins/cloud-init/+git/cloud-init:lp1843276
Merge into: cloud-init:master
Diff against target: 29 lines (+6/-0)
1 file modified
cloudinit/atomic_helper.py (+6/-0)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2019-09-10
Scott Moser 2019-09-09 Approve on 2019-09-09
Review via email: mp+372491@code.launchpad.net

Commit message

atomic_helper: add DEBUG logging to write_file

LP: #1843276

To post a comment you must log in.
Scott Moser (smoser) wrote :

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.

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 (daniel-thewatkins) wrote :

Oh yeah, great points. Will fix.

Dan Watkins (daniel-thewatkins) wrote :

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!

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) wrote :

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 (daniel-thewatkins) wrote :

Updated.

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

Scott Moser (smoser) wrote :

sorry for round trips.

review: Needs Fixing

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 (daniel-thewatkins) wrote :

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.

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) wrote :

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

review: Approve
Dan Watkins (daniel-thewatkins) wrote :

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

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)
review: Approve (continuous-integration)

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/atomic_helper.py b/cloudinit/atomic_helper.py
2index 587b994..1f61faa 100644
3--- a/cloudinit/atomic_helper.py
4+++ b/cloudinit/atomic_helper.py
5@@ -1,11 +1,13 @@
6 # This file is part of cloud-init. See LICENSE file for license information.
7
8 import json
9+import logging
10 import os
11 import stat
12 import tempfile
13
14 _DEF_PERMS = 0o644
15+LOG = logging.getLogger(__name__)
16
17
18 def write_file(filename, content, mode=_DEF_PERMS,
19@@ -23,6 +25,10 @@ def write_file(filename, content, mode=_DEF_PERMS,
20 try:
21 tf = tempfile.NamedTemporaryFile(dir=os.path.dirname(filename),
22 delete=False, mode=omode)
23+ LOG.debug(
24+ "Atomically writing to file %s (via temporary file %s) - %s: [%o]"
25+ " %d bytes/chars",
26+ filename, tf.name, omode, mode, len(content))
27 tf.write(content)
28 tf.close()
29 os.chmod(tf.name, mode)

Subscribers

People subscribed via source and target branches