I have noticed that either 127.0.0.1 is deleted from server_localhost.alias.drushrc.php or server IP from server_master.alias.drushrc.php on upgrade, or even when added again manually in the table, on every server verify task.

It is because function hosting_ip_save() calls hosting_ip_delete_revision(), and since there is no longer revision there, and vid has been replaced with nid in the function hosting_ip_delete_revision, it effectively deletes the record from the hosting_ip_addresses table, which causes missing IPs and all the mess which follows, when there are IPs used/expected in vhosts and not just wildcards.

Comments

Note also, that not only hosting_ip_delete_revision() is now the same as function hosting_ip_delete() , but removed revisions will cause confusion in other places, like: function hosting_server_update() and function hosting_server_insert() etc.

Title:IPs deleted on update from hosting_ip_addressesIPs deleted from hosting_ip_addresses table on server verify

Changing the title to precise where exactly this happens.

i marked hosting_ip_delete_revision() as deprecated, it now just calls hosting_ip_delete().

It's okay that this function is called: we need to remove the existing IPs to replace them. What we need to figure out is why they are not added back correctly!

Hmm.. check also hosting_nodeapi_server_delete_revision - deprecated and modified hosting_ip_delete_revision will break things there, so I don't think it should be deprecated or modified, rather just not used in the hosting_ip_save probably?

Well, sure, we need to remove calls to hosting_ip_delete_revision() elsewhere, but I don't see the problem with deprecating it, as revisions are gone from the IP table.

And this doesn't bring us closer to the source of the problem anyways... I'll try to reproduce this... :)

Hmm, but revisions are still used in the hosting_server table, so it may still break things if you deprecate this completely - it is still needed in the hosting_nodeapi_server_delete_revision.

So back to the issue - I have tested this also with Aegir 2.x vanilla option via BOA installer to make sure it is not some side result of other little BOA mods, and it doesn't even write 127.0.0.1 in that table, so also in the server_localhost.alias.drushrc.php file on initial install. But it worked before I have merged in that refactored SSL stuff, so it must be related.

*servers* are revisions, IP addresses are not. Also, IP addresses are not associated *only* to servers now, but also to SSL certificates, which means that it doesn't make much sense to have them versionned. That's why I removed the vid field from that table, it was not really being used - all revisions were flushed whenever there was an update, so in a way, even where server/IP coupling was stronger, there was no revisions of the IP addresses anyways.

So basically, hosting_nodeapi_server_delete_revision should just not mess around with IPs. In fact, I wonder if we shouldn't be generally more careful around the IP address table now that they are associated with certificates - we are probably missing some validation in the frontend: if a user removes an IP address associated with a certificate, it will silently do so, but then the site may be broken!

But you are right, I have no doubt that this is related to the IP refactoring. I just don't think the revisions bit is the culprit here.

I understand that, I just noticed that hosting_nodeapi_server_delete_revision which is(?) used for servers and not IPs, does have that hosting_ip_delete_revision used and expected to run on revisions, but expected to delete them in the hosting_server table (which has them still), not in the IPs table. But that is just a side note while we are messing with this stuff here, not related to the main issue - I just wanted to avoid breaking other stuff during this rewrite :)

StatusFileSize
new35.49 KB
new37.42 KB

I have tested it again, starting with Aegir 2.x based install (pre-ssl-rewritten-version), so the hosting_ip_addresses table had just three rows, like:

3

Then, after the first upgrade to Aegir 2.x head it didn't remove IP from drush aliases yet, but removed one record from the table:

2

After another upgrade, it removed server IP also from server_master.alias.drushrc.php but didn't remove IP from server_localhost.alias.drushrc.php, of course because 127.0.0.1 was still listed in the table and attached to the correct nid, while the server IP with server nid=2 no longer existed there, and only the third, related to the hostmaster site record was left, but of course it has its own nid, so it doesn't help.

Status:Active» Needs review

My guess is that it may be related to indexes, which prevent using INSERT INTO, so we may need to use REPLACE INTO instead. At least it worked with this patch: http://drupalcode.org/sandbox/omega8cc/1074912.git/commit/bcf6f2a, which fixed the problem.

We could simplify it by always using REPLACE INTO there, but I have left it for readability, for now.

StatusFileSize
new37.01 KB

It results with properly updated rows, now in a different order, but none is missing.

all

Status:Needs review» Needs work

REPLACE INTO is not portable (doesn't exist in PostgreSQL) and while we do not fully support postgres yet (see #548882: PostgreSQL frontend support and #585796: postgresql backend support), we are pretty close, and I wouldn't want to go backwards, especially in the frontend, which is almost completely portable.

Really, the deletion on that table should work and insert back again. If one of the IP is missing, it's probably that it's not being inserted back properly, I am not sure how using a REPLACE fixes the problem; indeed what I rather think is that this patch works because it doesn't remove the old entries anymore...

Hum. It just occurred to me that I forgot to push a commit that may unbreak some things here - there was a problem with the id column that was marked as "id" and not "serial" as it should have been. I just pushed 89ae01b, maybe it will fix things here... See also #1930670: Duplicate entry 0 for key PRIMARY in hosting_ip_addresses when installing / upgrading.

I have tested this as you have recommended and now it completely breaks on upgrade from hostmaster installed without those SSL related changes:

Executing hosting_server_update_6200 [32.24 sec, 12.33 MB]                                                                            [success]
Incorrect table definition; there can be only one auto column and it must be defined as a key                                         [warning]
query: ALTER TABLE hosting_ip_addresses ADD `id` INT unsigned auto_increment DEFAULT NULL database.mysqli.inc:169 [32.24 sec, 12.33
MB]
Unknown column 'id' in 'hosting_ip_addresses'                                                                     [warning]
query: ALTER TABLE hosting_ip_addresses CHANGE `id` `id` INT unsigned NOT NULL auto_increment database.mysqli.inc:169 [32.24 sec,
12.33 MB]
Unknown column 'id' in 'field list'                                                                               [warning]
query: UPDATE hosting_ip_addresses SET id=0 WHERE nid=4 AND ip_address='127.0.0.1' database.mysqli.inc:169 [32.24 sec,
12.33 MB]
Unknown column 'id' in 'field list'                                                                               [warning]
query: UPDATE hosting_ip_addresses SET id=1 WHERE nid=2 AND ip_address='198.211.104.130' database.mysqli.inc:169 [32.24
sec, 12.33 MB]
Unknown column 'id' in 'field list'                                                                               [warning]
query: UPDATE hosting_ip_addresses SET id=2 WHERE nid=10 AND ip_address='198.211.104.130' database.mysqli.inc:169 [32.24
sec, 12.33 MB]
Key column 'id' doesn't exist in table                                                                                 [warning]
query: ALTER TABLE hosting_ip_addresses ADD PRIMARY KEY (id) database.mysqli.inc:169 [32.24 sec, 12.34 MB]
ALTER TABLE {hosting_ip_addresses} ADD `id` INT unsigned auto_increment DEFAULT NULL [32.24 sec, 12.34 MB]                              [error]
ALTER TABLE {hosting_ip_addresses} CHANGE `id` `id` INT unsigned NOT NULL auto_increment [32.24 sec, 12.34 MB]                          [error]
ALTER TABLE {hosting_ip_addresses} DROP vid [32.24 sec, 12.34 MB]                                                                     [success]
UPDATE {hosting_ip_addresses} SET id=0 WHERE nid=4 AND ip_address='127.0.0.1' [32.24 sec, 12.34 MB]                           [error]
UPDATE {hosting_ip_addresses} SET id=1 WHERE nid=2 AND ip_address='198.211.104.130' [32.24 sec, 12.34 MB]                     [error]
UPDATE {hosting_ip_addresses} SET id=2 WHERE nid=10 AND ip_address='198.211.104.130' [32.24 sec, 12.34 MB]                    [error]
ALTER TABLE {hosting_ip_addresses} ADD PRIMARY KEY (id) [32.24 sec, 12.34 MB]                                                           [error]
ALTER TABLE {hosting_ip_addresses} ADD INDEX nid (nid) [32.24 sec, 12.35 MB]                                                          [success]
Executing hosting_server_update_6201 [32.24 sec, 12.35 MB]                                                                            [success]
Undefined index: query drush.inc:2729 [32.24 sec, 12.35 MB]                                                                            [notice]
[32.24 sec, 12.35 MB]                                                                                                                [success]
Executing hosting_server_update_6202 [32.24 sec, 12.35 MB]                                                                            [success]
Undefined index: query drush.inc:2729 [32.24 sec, 12.35 MB]                                                                            [notice]
[32.24 sec, 12.35 MB]                                                                                                                [success]
Executing hosting_site_update_6202 [32.24 sec, 12.35 MB]                                                                              [success]
ALTER TABLE {hosting_site} ADD `db_name` VARCHAR(64) NOT NULL DEFAULT '' [32.24 sec, 12.35 MB]                              [success]
Command dispatch complete [32.24 sec, 12.35 MB]                                                                                        [notice]
Peak memory usage was 24.57 MB [32.24 sec, 12.36 MB]                                                                                   [memory]
Including /var/aegir/drush/commands/core/clear.cache.inc [32.24 sec, 12.36 MB]                                                      [bootstrap]
'all' cache was cleared [32.24 sec, 12.36 MB]                                                                                         [success]
Finished performing updates. [32.24 sec, 12.36 MB]                                                                                         [ok]
Command dispatch complete [32.24 sec, 12.36 MB]                                                                                        [notice]
Peak memory usage was 27 MB [32.25 sec, 12.36 MB]                                                                                      [memory]
Drush was not able to start (bootstrap) the Drupal database.                                                                            [error]

After reverting these changes:

http://drupalcode.org/sandbox/omega8cc/1074912.git/commit/c79028e
http://drupalcode.org/sandbox/omega8cc/1074912.git/commit/00b4c7b

it works again - or at least, upgrade works and no IPs get wiped out from the table.

Status:Needs work» Needs review

This commit fixes the problem with broken upgrades: http://drupalcode.org/sandbox/omega8cc/1074912.git/commit/a968689

The result:

Executing hosting_server_update_6200 [37.85 sec, 14.31 MB]                                                                          [success]
ALTER TABLE {hosting_ip_addresses} DROP INDEX vid [37.85 sec, 14.31 MB]                                                             [success]
ALTER TABLE {hosting_ip_addresses} DROP vid [37.85 sec, 14.31 MB]                                                                   [success]
ALTER TABLE {hosting_ip_addresses} ADD `id` INT DEFAULT NULL [37.85 sec, 14.32 MB]                                                  [success]
ALTER TABLE {hosting_ip_addresses} CHANGE `id` `id` INT NOT NULL [37.85 sec, 14.32 MB]                                              [success]
ALTER TABLE {hosting_ip_addresses} CHANGE `id` `id` INT unsigned NOT NULL auto_increment, ADD PRIMARY KEY (id) [37.85 sec, 14.32 MB][success]
UPDATE {hosting_ip_addresses} SET id=0 WHERE nid=2 AND ip_address='198.211.104.130' [37.85 sec, 14.32 MB]                 [success]
UPDATE {hosting_ip_addresses} SET id=1 WHERE nid=4 AND ip_address='127.0.0.1' [37.85 sec, 14.32 MB]                       [success]
UPDATE {hosting_ip_addresses} SET id=2 WHERE nid=10 AND ip_address='198.211.104.130' [37.85 sec, 14.32 MB]                [success]
ALTER TABLE {hosting_ip_addresses} ADD INDEX nid (nid) [37.85 sec, 14.32 MB]                                                        [success]

Credit: http://api.drupal.org/comment/16549#comment-16549

StatusFileSize
new40.79 KB

The only problem that it doesn't really SET id=0, 1 and 2 as it shows in the task log. Instead, it looks like below:

qw

Assigned:Unassigned» anarcat
Status:Needs review» Needs work

re #17 - this does look odd. After a fresh install of 1.9 and an upgrade to 2.x git head (after the push of d517efa for #1930670: Duplicate entry 0 for key PRIMARY in hosting_ip_addresses when installing / upgrading), I get this:

+-----+-------------+----+
| nid | ip_address  | id |
+-----+-------------+----+
|   4 | 127.0.0.1   |  0 |
|   2 | 192.168.0.3 |  1 |
|  10 | 192.168.0.3 |  2 |
+-----+-------------+----+

Also, with the current git and the above install, a verify doesn't delete IPs from the hosting_ip_addresses table, but it does change the identifier of the IP, which is odd, here's what I get after the verify:

+-----+-------------+----+
| nid | ip_address  | id |
+-----+-------------+----+
|   2 | 192.168.0.3 |  1 |
|  10 | 192.168.0.3 |  2 |
|   4 | 127.0.0.1   |  4 |
+-----+-------------+----+

... which is what you are getting, so I guess there's a verify in between, and that's where the IP addresses get deleted and then re-created.

Note that the IPs don't seem to get lost anymore, which is an improvement, but I feel we still have a problem here: if an SSL certificate is associated with one of the IPs in the list, the connexions will be lost, which is one of the problems I wanted to avoid in the first place.

I also notice an IP is associated with node #10, which is the hostmaster site - that shouldn't be. An association should be made with the SSL certificate, if necessary...

I'll dig more into this now.

Yeah, this id's-dance doesn't look good, and yes, it will just cause a total mess when used to associate certs.

Maybe it is a side effect of having 'primary key' => array('id'), plus InnoDB way of handle indexes? See my other comment for reference.

I guess it must be something on the InnoDB side, because in the task log it was reported properly:

UPDATE {hosting_ip_addresses} SET id=0 WHERE nid=2 AND ip_address='198.211.104.130'
UPDATE {hosting_ip_addresses} SET id=1 WHERE nid=4 AND ip_address='127.0.0.1'
UPDATE {hosting_ip_addresses} SET id=2 WHERE nid=10 AND ip_address='198.211.104.130'

Yet, after checking the truth in the db directly, it was messed up.

Status:Needs work» Fixed

Ok, so I think I have an idea: we should manage IPs individually instead of with a huge textfield like this - it's too error prone and hard to validate.

If each IP is its own entity, the edit form should treat it as such and we should be able to add/remove IPs directly. To workaround the problem of adding a bunch of IPs, we could parse CIDR notations, but that would be just a nice addition on top.

Otherwise I can't see how we can fix this cleanly and still have the IP make sense.

I will therefore close this issue, as IPs are not being totally obliterated on server verify anymore - but it is a problem that the mapping gets lost, so I have opened #1968226: manage each IP individually on the server level to implement a cleaner solution for this.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.