Running gravity resets group management client assignments

Ronnick · January 26, 2020, 7:14pm

Every gravity run resets the group management client assignments to default. Need to have the selection persist a gravity run.

**Pi-hole Version**  vDev (release/v5.0, v4.3.2-391-ge0b3405) 
**Web Interface Version**  vDev (release/v5.0, v4.3.2-360-g88da85f5)  
**FTL Version**  vDev (release/v5.0, vDev-9889501)

Tntdruid · January 26, 2020, 10:22pm

Seen that too now.

jpgpi250 · January 27, 2020, 9:39am

This is probably caused by this PR, the result of a three day test session
pihole -g now uses a temporary database, to build the new gravity table. The content of the existing database is than copied over, the temporary database than becomes the live one.
I assume @DL6ER did NOT copy over all the tables, or the copy process has a bug.

I also wonder what happens if users are using the web interface, while pihole -g is running. should the database not be locked (+ warning) during pihole -g?

DL6ER · January 27, 2020, 9:47am

We do copy the respective database:

github.com

pi-hole/pi-hole/blob/e0b3405a4df337caf276d18428f2c02cbc50fb64/advanced/Templates/gravity_copy.sql#L19


      
          
          INSERT OR REPLACE INTO domainlist SELECT * FROM OLD.domainlist;
          INSERT OR REPLACE INTO domainlist_by_group SELECT * FROM OLD.domainlist_by_group;
          
          INSERT OR REPLACE INTO adlist SELECT * FROM OLD.adlist;
          INSERT OR REPLACE INTO adlist_by_group SELECT * FROM OLD.adlist_by_group;
          
          INSERT OR REPLACE INTO info SELECT * FROM OLD.info;
          
          INSERT OR REPLACE INTO client SELECT * FROM OLD.client;
          INSERT OR REPLACE INTO client_by_group SELECT * FROM OLD.client_by_group;
          
          COMMIT;

I will try to reproduce this myself now.

Hmm, yes, but it seems better to just move copy process itself further down, immediately before the swapping. Hereby, users will be able to use the database all the time and no locking happens. If they would try to add something in the exact milliseconds the swapping happens, we already show an error as the database will indeed be locked during this short time interval.

edit Reproduced. Group assignments are correctly copied and kept.

However, one of the triggers fires and adds the default group for all added clients on top of their already configured groups. This is clearly a bug and needs to be addresses. The only way to do this is to delete the triggers before copying the stuff and re-add them later again.

DL6ER · January 27, 2020, 9:56am

This is already happening so no need to change anything here.

DL6ER · January 27, 2020, 10:24am

Bugfix is on its way:

github.com/pi-hole/pi-hole

DROP and reCREATE TRIGGERs during gravity swapping.

pi-hole:release/v5.0 ← pi-hole:fix/group_assignments

opened 10:23AM - 27 Jan 20 UTC

DL6ER

+21 -0

**By submitting this pull request, I confirm the following:** - [X] I have r…ead and understood the [contributors guide](https://github.com/pi-hole/pi-hole/blob/master/CONTRIBUTING.md), as well as this entire template. - [X] I have made only one major change in my proposed changes. - [X] I have commented my proposed changes within the code. - [X] I have tested my proposed changes, and have included unit tests where possible. - [X] I am willing to help maintain this change if there are issues with it later. - [X] I give this submission freely and claim no ownership. - [X] It is compatible with the [EUPL 1.2 license](https://opensource.org/licenses/EUPL-1.1) - [X] I have squashed any insignificant commits. ([`git rebase`](http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html)) --- **What does this PR aim to accomplish?:** Fix v5.0 bug reported on [Discourse](https://discourse.pi-hole.net/t/running-gravity-resets-group-management-client-assignments/27490/5?u=dl6er). **How does this PR accomplish the above?:** `DROP` triggers before copying data from the old to the new gravity database that create an association to all newly `INSERT`es clients/domains/adlists. Recreate them afterwards. If we do not drop these triggers, they will force the Unassociated group onto all entries even if the user deleted them before. This is a bug. **What documentation changes (if any) are needed to support this PR?:** None

Thanks @Ronnick for reporting this and helping us make Pi-hole a better software for all of us!

DL6ER · February 7, 2020, 4:43pm

The bugfix has been reviewed and merged.

Ronnick · February 7, 2020, 5:10pm

Awesome!

Ronnick · February 7, 2020, 8:57pm

It’s working as expected now. Thanks again.

Tntdruid · February 7, 2020, 10:37pm

Works for me too!

DL6ER · February 10, 2020, 8:27am

Thanks for your confirmations and for helping us make Pi-hole a better software for us all!