search

uptane / aktualizr

C++ Uptane Client
Mozilla Public License 2.0
15 stars 15 forks source link

Validate OSTree objects before uploading them #4

Closed cajun-rat closed 3 years ago

cajun-rat commented 3 years ago

The point of this patch series is to add a --enable-integrity-checks option to garage-push.

There are quite a few tidy-ups in the lead up to the 'Rework OSTreeObject' and 'Add option to validate' commits that include the meat of feature. If you'd like me to split the series into some smaller PRs, then please shout (or merge the commits that you're happy with out of the box). I tried to put the less controversial ones first.

There are a couple of tests for the new functionality: unit tests against OSTreeObject, and tests/sota_tools/test-upload-corrupt-object that exercises the garage-push command line interface.

pattivacek commented 3 years ago

Also, how much extra time does all this fsck'ing add to a typical garage-push run? Is this something you want to enable by default? Either way, meta-updater will need to be updated to adjust the dependencies.

cajun-rat commented 3 years ago

I doubt the execution time is significant, although I haven't tested it. Only objects that need to be uploaded are checked, so the fsck time will scale with the bytes uploaded and not the overall repo size. I believe @sergioprado was keen on having it off by default for compatibility reasons rather than performance though.

sergioprado commented 3 years ago

@cajun-rat exactly. The additional --enable-integrity-checks parameter was just a suggestion to avoid any side effects (like performance hits) on already deployed systems. But I don't have hard feelings about it. I am OK if we decide to drop it, or maybe revert the flag (aka --disable-integrity-checks).

cajun-rat commented 3 years ago

So it looks like we have 3 options:

  1. Default off with --enable-integrity-checks - The most compatible with existing operation
  2. Default on --disable-integrity-checks - Makes the default mode our recommended mode of operation
  3. Always on - Slightly less code

I don't have a strong preference either way.

pattivacek commented 3 years ago

I doubt the execution time is significant, although I haven't tested it. Only objects that need to be uploaded are checked, so the fsck time will scale with the bytes uploaded and not the overall repo size. I believe @sergioprado was keen on having it off by default for compatibility reasons rather than performance though.

I'd be curious to measure the difference, but I'm also not too worried about it. Assuming it really is fairly small, I don't see why you wouldn't want it on by default. What backwards compatibility concern would there be? It would only cause a problem if you have a corrupt object, and if you do, you probably want to know that and fix it. If you can't fix it and don't care, or the runtime cost is too high, then overriding with a --disable-integrity-checks flag seems reasonable. But generally, we're talking about running these checks on build machines that have to be powerful enough to run bitbake, so efficiency isn't exactly the biggest concern. Even in the case of using garage-deploy on a less-powerful laptop, it still can't be that bad. That's how I see it, but I'm open to debate.

And thanks for fixing all the small things!

tkfu commented 3 years ago

It would only cause a problem if you have a corrupt object, and if you do, you probably want to know that and fix it. If you can't fix it and don't care, or the runtime cost is too high, then overriding with a --disable-integrity-checks flag seems reasonable.

I'd agree with this sentiment in general--I can't really imagine wanting to skip it.

Just to give an idea of the order of magnitude of the runtime cost, I decided to time fscking a whole repo with about 30,000 content objects and a total size of just under 3 GB on my thinkpad from 2015. It was CPU-bound, and completed in 31 seconds. At an average of one millisecond per object on an old laptop, I don't think the fscking overhead will be anything anyone's build machines will worry about.

cajun-rat commented 3 years ago

@tkfu, @sergioprado and I had a chat about this just now, and we're in favour of the --disable-integrity-checks option too. I'll update the PR shortly (it should only be a change to the final commit).

pattivacek commented 3 years ago

Great, thanks for doing the research @tkfu !

cajun-rat commented 3 years ago

Thank you @pattivacek What's the process for merging to master? I don't have write access to the repo

pattivacek commented 3 years ago

I'd say if tests pass and no one objects, we merge it!

cajun-rat commented 3 years ago

:+1: Thank you!