Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

add_copy_spec_limit() is limited #281

Open
bmr-cymru opened this Issue Apr 29, 2014 · 1 comment

Comments

Projects
None yet
3 participants
Owner

bmr-cymru commented Apr 29, 2014

The add_copy_spec_limit() interface has always been a bit of a hack. Size limits are applied during setup() to the result of applying glob expansion to the copyspec argument. Paths that pass the test are subsequently passed to add_copy_spec() with no limit. Since the glob results can include directories this is obviously broken for any glob that would include a directory in its expansion.

This can definitely be improved:

  • Stat files during setup() and get an estimate of the report size
    • can't easily estimate command output size
  • Apply limits during collect()
  • Separate global, plugin and file limits

It's not really clear how the limit should be applied to paths that include directories; if we're going to allow the limit interface to be used with directories we should define how it's applied. E.g. collect files in the order returned by sorting the result of os.walk() for each item that matches a copyspec.

Having a global limit (and a default plugin limit) would fix the general concerns raised in Issue #150.

Contributor

mbaldessari commented Apr 29, 2014

yes this is definitely a needed interface. I was finishing up the PCP plugin (https://github.com/mbaldessari/sosreport/blob/pcp-plugin/sos/plugins/pcp.py) and resorted to recursive directory size calculation as pcp log files can have a flexible structure and are potentially big. For my specific PCP use case a very specific add_directory API like add_copy_directory_limited() would be great as it is very well defined in its scope. (Might clash a bit with the existing _spec API family though).

@bmr-cymru bmr-cymru added the medium label Jun 8, 2014

@bmr-cymru bmr-cymru added this to the 3.2 milestone Jun 8, 2014

@bmr-cymru bmr-cymru added the bugfix label Jun 8, 2014

@bmr-cymru bmr-cymru self-assigned this Jun 16, 2014

@battlemidget battlemidget modified the milestones: 3.2, 3.3 Aug 19, 2014

@bmr-cymru bmr-cymru modified the milestones: 3.3, 3.2 Oct 8, 2014

bmr-cymru referenced this issue in BryanQuigley/sos Jul 7, 2015

Update juju logging to have limits
Juju can name files differently depending on the deployment
so we need iterate over all the files and capture at least some
of them all. At the same time the files can get quite big so we
need to be able to limit their size.

Now captures upstart/juju-db.log which is usually quite small.
Juju local is usually used for testing so we just capture
all-machines.log for that (format /var/log/juju-name-name/)

Lastly we capture the ls of all of the key directories just in
case we missed something.

Signed-off-by: Bryan Quigley <bryan.quigley@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment