On 2020-03-30 12:43, Paolo Bonzini wrote: > On 29/03/20 13:22, Stefan Raspl wrote: >> I wrote a respective patch and tried it out, and found this approach not to be >> workable for a number of reasons: >> - The implicit buffering that takes place when redirecting output of kvm_stat in >> logging mode to a file messes up the format: logrotate moves the files on the >> disks, but there might be still data buffered that hasn't been written out >> yet. The SIGHUP triggers a new header to be written with the patch I came up >> with, but that header would sometimes appear only after a couple of lines >> written to the file, which messes up the format. Flushing stdout in the signal >> handler won't help, either - it's already too late by then. > > I don't understand this. Unless you were using copytruncate, the > sequence should be: > > - logrotate moves file A to B > > - your file descriptor should point to B, so kvm_stat keeps writing to > file B > > - you send the signal to kvm_stat > > - kvm_stat closes file B, so all pending records are written > > - kvm_stat reopens file A and writes the header. I was using copytruncate indeed. But removing it, things still don't work (kvm_stat continues to write to B). But maybe there's a deeper misunderstanding: My assumption is you'd do something like 'kvm_stat -l > /var/log/kvm_stat.txt'. If so, how could kvm_stat ever be aware of where its output gets redirected to, nevermind open/closing any of those files? Or did you mean kvm_stat should close & open stdout?! > If you have a race of some sort, try having the signal handler do > nothing but set a flag that is examined in the main loop. > >> - When restarting kvm_stat logging (e.g. after a reboot), appending to an >> existing file should suppress a new header from being written, or it would end >> up somewhere in the middle of the file, messing up the format. kvm_stat >> doesn't know that or where its output is redirected to, so no chance of >> suppressing it within kvm_stat. We would probably require some kind of wrapper >> script (and possibly an extra cmd-line option to suppress the header on >> start). > > You could stat the output file, and suppress the header if it is a > regular non-empty file. But it would be a problem anyway if the header > has changed since the last boot, which prompts the stupid and lazy > question: how does your series deal with this? How could we stat the output file if kvm_stat is just writing to a console? My previous patch series was built on top of the RotatingFileHandler class, which was making sure that we wouldn't repeat the header in case we're appending to an existing file. I don't believe there is any way of dealing with changes in the fields selected - unless we just rotate files whenever we restart logging to be on the safe side. But with the target scenario at hand (routinely logging in the background as part of systemd or the like), the only plausible scenario would be that we introduce new fields that get printed by kvm_stat per default. > This one seems the biggest of the three problems to me. > >> - I was surprised to realize that SIGHUP is actually not part of logrotate - >> one has to write that manually into the logrotate config as a postrotate... >> and I'll openly admit that writing a respective killall-command that aims at a >> python script doesn't seem to be trivial... > > This one is easy, put "ExecReload=/bin/kill -HUP $MAINPID" in the > systemd unit and use "systemctl reload kvm_stat.service" in the > postrotate command. Ah, OK - was searching for a solution within the realms of logrotate. >> As much as I sympathize with re-use of existing components, I'd like to point >> out that my original patch was also re-using existing python code for rotating >> logs, and made things just _so_ much easier from a user perspective. > > I understand that, yes. My request was more about not requiring > kvm_stat-specific configuration than about reusing existing components, > though. Taking a step back and looking at the tightly integrated kvm_stat - logrotate - systemd approach outlined above, I'd bet most users would prefer a self-contained solution in kvm_stat that merely requires adding a single extra command line switch. And they can still add systemd on top, which wouldn't need to interact with any of the other components except to start kvm_stat initally. Ciao, Stefan
