remove-from-packages: deleting files it shouldn't

[dustymabe]

I believe remove-from-packages: in the manifest is being overzealous when removing files.

For example in RHCOS I was trying to remove all files from the dhcp-client rpm and it ended up removing everything under and including /etc/NetworkManager as well.

Here is the excerpt from the manifest:

remove-from-packages:                                                  
  # In newer dracut-network dhclient isn't required if you have NM 1.20
  # https://github.com/dracutdevs/dracut/commit/e863807                
  # Let's go ahead and get rid of it now.                              
  - - dhcp-client                                                      
    - "/"                                                              

Here are the files owned by the dhcp-client rpm:

$ rpm -ql dhcp-client-4.3.6-34.el8.x86_64 | grep NetworkManager
/etc/NetworkManager
/etc/NetworkManager/dispatcher.d
/etc/NetworkManager/dispatcher.d/11-dhclient

Here is the log snippet from the compose:

Deleting: etc/NetworkManager
Deleting: etc/NetworkManager/dispatcher.d
Deleting: etc/NetworkManager/dispatcher.d/11-dhclient

And we don't see any NetworkManager files in /etc/ for the resulting OSTree:

$ ostree --repo=tmp/repo/ ls fe1f2a034338d60ccae9bacc6caaf412b9e68638c5ed35a16bdcbfd6b6f1ca81 /usr/etc/NetworkManager
error: No such file or directory: /usr/etc/NetworkManager

I believe expected behavior in this scenario would be that only the /etc/NetworkManager/dispatcher.d/11-dhclient would be deleted. Arguably the dhcp-client rpm should not claim to own the /etc/NetworkManager or /etc/NetworkManager/dispatcher.d directories, but I don't think we should rely on packagers to get this right. Maybe remove-from-packages: can do something like cross check if any other rpms own this file then don't delete it.

[dustymabe]

The version of rpm-ostree in COSA I'm using: rpm-ostree-2020.1.80.g3ec5e287-1.fc31.x86_64

[jlebon]

Yeah, I think we need to make remove-from-packages use the same code in the core that checks which files/dirs are shared (see #1227, specifically a6926e0).

[dustymabe]

One other small thing I noticed. I updated the manifest entry to be more surgical (i.e. just remove regular files):

- - dhcp-client                                        
  # we want to delete all files ("/") but can't because
  # `remove-from-packages` is being overzealous:       
  # https://github.com/coreos/rpm-ostree/issues/2068   
  - "/etc/dhcp/dhclient.conf"                          
  - "/etc/NetworkManager/dispatcher.d/11-dhclient"     
  - "/usr/lib64/pm-utils/sleep.d/56dhclient"           
  - "/usr/sbin/dhclient"                               
  - "/usr/sbin/dhclient-script"                        

But I notice the logs saying:

Deleting: etc/dhcp/dhclient.conf
Deleting: etc/NetworkManager/dispatcher.d/11-dhclient
Deleting: usr/lib64/pm-utils/sleep.d/56dhclient
Deleting: usr/sbin/dhclient
Deleting: usr/sbin/dhclient-script
Deleting: usr/sbin/dhclient-script

It all looks good up until that duplicated Deleting: usr/sbin/dhclient-script which would lead me to think it's inappropriately globbing the - "/usr/sbin/dhclient" entry. What if I didn't want to delete dhclient-script?

Just to confirm I re-ran with:

- - dhcp-client                                        
  # we want to delete all files ("/") but can't because
  # `remove-from-packages` is being overzealous:       
  # https://github.com/coreos/rpm-ostree/issues/2068   
  - "/etc/dhcp/dhclient.conf"                          
  - "/etc/NetworkManager/dispatcher.d/11-dhclient"     
  - "/usr/lib64/pm-utils/sleep.d/56dhclient"           
  - "/usr/sbin/dhclient"                               

and see:

Deleting: etc/dhcp/dhclient.conf                     
Deleting: etc/NetworkManager/dispatcher.d/11-dhclient
Deleting: usr/lib64/pm-utils/sleep.d/56dhclient      
Deleting: usr/sbin/dhclient                          
Deleting: usr/sbin/dhclient-script                   

[cgwalters]

One thing that would fix this in a nice way is to delete the files during import. Then it'd work similarly to e.g. documentation: false. However, this would exacerbate issues we have where we're not accounting for "changes" like this during import. So what would happen is if e.g. one changes the manifest to remove the remove-from-files, we'd silently reuse the previous cached version with the removed files.

[kelvinfan001]

I haven't looked at how import works yet, but I think handle_file_dispositions() makes sense to me for now. Is there a preference for one way over the other?

[cgwalters]

I'm OK with refactoring handle_file_dispositions() to try to handle this (as jlebon suggested). But if we do it at import time, then: the file was never written in the first place, so conflicts are automatically handled correctly. It'd also be way simpler to implement I'm sure, no dealing with the librpm APIs. Just pass the excludes for a given package down from the core into the importer and change compose_filter_cb() in a similar way to how it's handling documentation: false.

[kelvinfan001]

Does this mean to not use the current handle_remove_files_from_package(), and, instead, never have the files that we don't need be installed in the first place?

[kelvinfan001]

In order to call handle_file_dispositions() from handle_remove_files_from_package() without changing its parameters, it looks like I would need to pass in an RpmOstreeContext; this would require me to pass it all the way down from impl_install_tree() (handle_remove_files_from_package()'s caller's caller). Does this sound reasonable? @jlebon

[jlebon]

Handling it at import time has an appeal, but I'd want to handle the cache invalidation issue. The dev UX is not great though; it's not at all obvious that changing remove-from-files means you'll probably have to rerun cosa fetch.

Hmm actually, an approach better than either of those is to just do it in the checkout_filter in the core. E.g. in checkout_package_into_root, check if the package in question is in remove-from-files (you have access to the treefile via treefile_rs; you can add a Rust helper), then add all the patterns to a ptrarray and make that part of the filter_user_data. Then in checkout_filter, you'd just go through each pattern and g_regex_match against it.

Does that make sense?

[kelvinfan001]

I see, that makes sense. On a high level, that's also along the lines of "not installing" instead of "installing and then removing" right?

[jlebon]

I see, that makes sense. On a high level, that's also along the lines of "not installing" instead of "installing and then removing" right?

Oh definitely. (Even in the handle_file_dispositions approach, it would've been about avoiding what files to checkout via the existing files_skip_add). rpm-ostree server composes always start from scratch, so there is no RPM "uninstall" in that sense. The code in core.c is used both on the server and the client though, which is why it does have a concept of "uninstalling" packages.