Detect conflicts during merge conflict

[atanasdinov]

During an upgrade, /etc (and any snapshotted RW volume) is reconciled with a 3-way merge: the customizations delta (old stock -> user-modified) is applied on top of the new image's defaults. When a file is changed by both the user and the new image, the user's version wins, but until now that happened silently.

This change detects those overlaps and warns the user, e.g.:

Merge conflicts detected for /etc (2 file(s)), user version kept:
    /modifiedFile — user: modified, OS: modified
    /deletedFile — user: deleted, OS: modified

Previously users had no visibility into cases where an upgrade wanted to change a file they had also customised. The merge result is correct (customisations are intentionally preferred), but silently discarding the new image's version of a config file is surprising. This surfaces it so the user can reconcile manually if needed.

The merge outcome is unchanged. This is detection and reporting only; no automated resolution, no change to what ends up on disk:

• The customisations delta is still applied exactly as before (user version always wins).
• The process of volume configurations and stock-snapshot handling, xattr filtering, and rsync invocations are all unchanged.
• Conflict detection is a read-only per-file comparison (old stock vs new image) done in the same single pass that already parses the snapper status file and applies changes; no extra tree walk, no second parse as per David's initial review.

One implementation nuance in applyCustomChanges: user deletions are now buffered and applied after the status scan instead of inline. This is required so the per-file OS-delta check sees a pristine new tree. It is equivalent to the previous behaviour for the on-disk result; deleted paths and synced paths are disjoint, and both still complete before rsync runs.

For each file the user changed (from snapper status), we report whether the new image also changed that path (added/modified/deleted, by type → size → content, with a size cap for large blobs). If both sides touched it, it's recorded as a conflict and reported via a warning after the volume is merged.

[davidcassany]

I appreciate this is in draft review, hence this is probably not ready for review. If you don't mind I have a couple of comments regarding the implementation.

I am wondering if instead of gathering a full diff for both cases wouldn't it be simpler to diff one and then from a first list of modified files then use it to diff them on the other side of the 3 way merge. After all we want to detect conflicts so any file not included in the first diff is known not to be a conflict candidate.

[davidcassany]

Wouldn't it be possible to check for conflict on file per file basis here in this loop instead of walking the whole tree? So here we just check the user modified tree if it is also modified by the new image. So we only diff the files which are potentially conflicting and we do not need to parse again snapper diff status, load it in memory, iterate over it again and walk the whole tree of the snapshot.
So we basically create the list of files to merge and the list of conflicting files together at the same time.

[davidcassany]

LGTM, just a couple of minor comments

[davidcassany]

This buffer is initialized but not used right?. IIUC the diff of this file is most likely a leftover, this does not apply any effective change.

[atanasdinov]

It is used in the assertions in snapper_upgrade_helper_test.go (line 270+).

[davidcassany]

Oh yes, sorry, I forgot this is shared code with several tests. I just looked in was not used within the same file 😅

[davidcassany]

Does it mean any file bigger than 16MiB which is modified in both branches (upgraded content and modified content) is always considered a conflict? Just to make sure I am understanding it.

I wonder if 16MiB is low value. In any case I'd not expect many big and potentially conflicting files under /etc, as it would mean we ship them at first place. Have you checked if we ship and big file under /etc?

[atanasdinov]

Yes, your understanding is correct, with one caveat.

FileChange is only evaluated for files snapper already flagged as user-modified, and it compares the old stock vs the new image to decide the OS side. For a file that exists on both sides and is larger than 16 MB, it reports "modified" without reading the content:

• if the sizes differ -> the file genuinely changed.
• if the sizes are equal -> the file is assumed modified rather than reading the whole blob.

So for such a file, "user modified it" + "assumed OS-modified" = conflict, even in the (rare) case where the new image's copy is byte-identical. That's the trade-off we introduce here: bounded I/O vs. a possible false-positive warning (resolution is unaffected as user's version is kept either way).

On whether 16 MB is low value, this is what we are observing for the largest files (ignoring /etc/.snapshots):

  ┌──────┬─────────────────────────────────────────────────────────────────┐
  │ Size │                              File                               │
  ├──────┼─────────────────────────────────────────────────────────────────┤
  │ 13M  │ /etc/udev/hwdb.bin                                              │
  ├──────┼─────────────────────────────────────────────────────────────────┤
  │ 5.3M │ /etc/selinux/targeted/contexts/files/file_contexts.bin          │
  ├──────┼─────────────────────────────────────────────────────────────────┤
  │ 3.8M │ /etc/selinux/targeted/policy/policy.34                          │
  ├──────┼─────────────────────────────────────────────────────────────────┤
  │ 436K │ /etc/selinux/targeted/contexts/files/file_contexts              │
  ├──────┼─────────────────────────────────────────────────────────────────┤
  │ 296K │ /etc/selinux/targeted/contexts/files/file_contexts.homedirs.bin │
  ├──────┼─────────────────────────────────────────────────────────────────┤
  │ 116K │ /etc/mime.types                                                 │
  ├──────┼─────────────────────────────────────────────────────────────────┤
  │ 88K  │ /etc/vmware-tools/vgauth/schemas/XMLSchema.xsd                  │
  ├──────┼─────────────────────────────────────────────────────────────────┤
  │ 32K  │ /etc/nftables/osf/pf.os                                         │
  ├──────┼─────────────────────────────────────────────────────────────────┤
  │ 20K  │ /etc/vmware-tools/tools.conf.example, /etc/grub.d/20_linux_xen  │
  └──────┴─────────────────────────────────────────────────────────────────┘

In general, I'd expect files over 16MB to be binaries and deeming them as "large" enough not to consume memory is probably a safer bet. Thoughts?

[ipetrov117]

Overall approach looks good to me, approved 🚀 . I think we should consider how this will be handled in the LCM use-case as well (not in this PR obviously).

Right now LCM runs elemental3 upgrade .. through a SUC workload. If a conflict is detected, it will be logged, but no-one will be able to act upon it, because merge conflicts will not break the upgrade. As the upgrade will be marked as successful, the SUC workload will reboot the OS and only after this reboot will the user be aware that something is wrong due to a potentially breaking conflict.

I wonder whether it would make sense to make failing on merge conflicts configurable. That way LCM could block an upgrade on merge conflicts, allowing for the user to inspect the conflicts and only after assessing their severity, instruct LCM to continue with the upgrade. This would ensure that we catch user configuration conflict errors before we actually apply the new version, which will reduce the chance for needlessly recovering an OS after an upgrade.

I recognise that this will have some caveats, but I think it is worth considering, as blindly accepting merge conflicts on an automated upgrade process could prove to be problematic. That said I can be missing, misunderstanding, or overcomplicating things. Let me know what you think.