salt: [BUG] RuntimeWarning: line buffering (buffering=1) isn't supported in binary mode

Why do you even want to configure such a low-level detail in the first place?

Answering my own question: I’ve checked, and bufsize was included from the very commit that brought us file.replace in the first place, 89ebfc02d6682e39cdcc79db974773f5657a9fce. Its intention seems to be to make sure that multi-line search/replace works: The documentation points out that you need to use bufsize: file for multi-line searches because line buffering.

Okay, that’s a good reason, I guess. Or it was back then.

However, nowadays the file is mmapped anyway. Does the buffer size make any difference (in functionality, not in speed) at all anymore? Multi-line searches should work regardless of what bufsize is set to, or am I missing something here? Could we simply get rid of bufsize altogether and use Python’s defaults?

@scy could you please add some more details

Okay. Let’s recap. The issue, as pointed out by @OrangeDog, is that file.replace uses a default bufsize of 1, apparently assuming that this means “line-buffered”. And it indeed would mean line-buffered if the file was opened in text (not binary) mode – but it isn’t.

The workaround suggested by @boltronics changes Salt’s file opening code to completely disable buffering when the file is opened in binary mode. All of the read/write operations on that file then will issue real filesystem requests (I guess) which may lead to a bit of degraded performance, depending on how many operations you’re actually doing. (NB: This is just guesswork, I haven’t measured any of this and I assume it will have no noticeable real-world impact in most scenarios.)

Instead of disabling buffering, you might as well set it to any value greater than 1 to mean “use a buffer that’s X bytes in size”. Any value other than 1 will make the warning go away, because 1 is reserved for line-based buffering, but you can’t use line-based buffering on a binary file. (See Python’s open() documentation.)

As stated in the documentation for file.replace, file.replace also understands the special value "file" that means “make the buffer as large as the file”. This will read the whole file into RAM, but all read operations will then not need to talk to the disk at all.

Also, and this is why I chose bufsize: file as my suggestion: It works without modifying Salt’s source code.

If you’d like to have an example for where to apply this workaround: Simply modify the file.replace use in your SLS to include bufsize: file, e.g.

add_login_group_to_winbind_ssh_access_list:
  file.replace:
    - name: '/etc/security/pam_winbind.conf'
    - pattern: '^(require_membership_of = )(.*)$'
    - repl: '\1\2,append-new-group-to-line'
    - bufsize: file

You could, as a workaround, just as well basically use any integer > 1. Python’s default buffer size depends on the block size and might be something like 4096 or 8192.

The best solution, in my opinion, would be to not make the buffer size configurable at all and rely on Python’s defaults. Deprecate and/or ignore the bufsize parameter for file.replace (and other file functions that might be doing something similar). Why do you even want to configure such a low-level detail in the first place?

I chose bufsize: file as my suggestion

Just to note that if the file is really big then that’s a terrible idea.

You’re totally right 😃 Which is why I originally wrote

Note that this might not be feasible if you are working with a large file, but it should be fine for most config files etc.

1 is a magic number that means “line buffering”, on the assumption that nobody wants a 1-byte buffer.

So the problem is here (default bufsize is 1). https://github.com/saltstack/salt/blob/v3001rc1/salt/modules/file.py#L2552