Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Use default localhost finder for MacOs - fix #2098 #2134

Merged
merged 8 commits into from Oct 11, 2022

Conversation

grzi
Copy link
Contributor

@grzi grzi commented Oct 1, 2021

This will fix #2098

The NetUtil retrieves a 'wrong' localhost addr on local, that is impossible to map under 30 seconds, using the standard way makes it instant.


Dev Handoff Notes (Internal Use)

Links

Testing

  • Steps to Reproduce: NetUtils.getLocalHostName() takes 30s  #2098
  • Guidance:
    • Impact: Changes hostname lookup used in a variety of places. Easiest to see may be in the databasechangeloglock table
    • Change unifies logic across linux/mac/windows, but the underlying java calls may act differently in different OS's
    • To ensure that the hostname is working correctly:
      • run liquibase update
      • ctrl-c to kill liquibase after it has locked the database
      • liquibase list-locks to ensure that the hostname is not localhost
    • To test speed, run a liquibase update with an empty changelog so that there is little to do. On mac, it should be about as fast as windows/linux

Dev Verification

Ran updated NetUtilTest on linux and windows. Don't have a mac to run it on

Test Requirements (Liquibase Internal QA)

Performance testing is out of scope for this issue. QA will validate that the correct hostname is used and that update is successful, but not how long it takes an update to run.

  • Setup
  • You will need a Mac and a Windows machine to test this fix.
  • The database platform does not matter but example snippets are for MySQL.
  • On the database, ensure AUTOCOMMIT is disabled.
  • Create a table, "accounts":
CREATE TABLE accounts (
    name VARCHAR(100) NOT NULL,
    balance DEC(15,2) NOT NULL
);
  • In a database IDE, start an insert transaction to the accounts table:
BEGIN;

INSERT INTO accounts(name,balance)
VALUES('Abernathy',50000);
  • Do not commit the insert!!
  • Create a formatted SQL file named addColumn-changelog.sql with the following content:
--liquibase formatted sql

--changeset liquibase:lb2180
ALTER TABLE accounts
ADD COLUMN phone VARCHAR(15);`

Manual Tests

TEST 1: Verify the DATABASECHANGELOGLOCK.LOCKEDBY has the test machine's name and not localhost.
Start a Liquibase update to add a column to table accounts:

  • liquibase update --changelog-file addColumn-changelog.sql

In another command prompt, run:

  • liquibase list-locks
    The list-locks command outputs the lock information with the machine name, for example:
    - LENOVO (172.21.224.1) at Dec 2, 2021 3:54:40 PM , where LENOVO is the machine name.

TEST 2: Verify an update operation is successful.
Execute Liquibase update:

  • liquibase update --changelog-file addColumn-changelog.sql

  • Automated Tests

    • No new functional or harness tests required for this fix.

┆Issue is synchronized with this Jira Bug by Unito

@grzi grzi force-pushed the fix/slow_get_localhost_on_mac branch 5 times, most recently from d809e2d to 8a1c943 Compare October 1, 2021 12:11
@grzi grzi marked this pull request as ready for review October 1, 2021 12:15
@grzi grzi force-pushed the fix/slow_get_localhost_on_mac branch from 8a1c943 to fada7b4 Compare October 1, 2021 12:28
@grzi grzi changed the title fix: Use default localhost finder for MacOs fix: Use default localhost finder for MacOs - fix #2098 Oct 4, 2021
@molivasdat molivasdat added this to To Do in Conditioning++ via automation Nov 23, 2021
…generic check for whether a loopback or local address got returned before falling back to iterating over interfaces

Also cached the found interface to avoid future lookups
@nvoxland nvoxland changed the base branch from master to 4.4.x December 8, 2021 17:59
@nvoxland nvoxland changed the base branch from 4.4.x to master December 8, 2021 17:59
@nvoxland
Copy link
Contributor

nvoxland commented Dec 8, 2021

I merged the current master branch into the fork.

I also changed the implementation a bit in the fork and added more tests.

Since it seems that the InetAddress.getLocalHost() is so much faster, we are now always calling that regardless of the OS and then falling back to the "loop through the interfaces" setup only if the initial fast call returned a localhost address.

Testing on windows, the code didn't fall back into the loop through.
Testing on linux, the code DID fall through to the interface looping.
Both those behaviors were as they were before.

Thanks for the details on how mac behaves, @grzi

@nvoxland nvoxland moved this from To Do to Code Review in Conditioning++ Dec 8, 2021
@suryaaki2 suryaaki2 moved this from Code Review to Ready for Handoff (In JIRA) in Conditioning++ Dec 8, 2021
@XDelphiGrl
Copy link
Contributor

This PR removes OS-specific code to get the hostaddress and uses InetAddress foundHost = InetAddress.getLocalHost(); for Mac, Windows and Linux. If the foundHost is a loopback address or a link-local address, Liquibase iterates through the network interfaces to locate a more "descriptive" hostaddress.


Validate update-sql INSERT for DATABASECHANGELOGLOCK.LOCKEDBY includes hostname and hostaddress.

  • Mac FAIL (see issues logged during test)
UPDATE PUBLIC.DATABASECHANGELOGLOCK SET LOCKED = TRUE, LOCKEDBY = '10.0.0.8 (10.0.0.8)', LOCKGRANTED = '2022-02-14 14:47:04.238' WHERE ID = 1 AND LOCKED = FALSE;
  • Windows PASS
UPDATE testdb1.DATABASECHANGELOGLOCK SET `LOCKED` = 1, LOCKEDBY = 'LENOVO (10.0.0.11)', LOCKGRANTED = NOW() WHERE ID = 1 AND `LOCKED` = 0;

Verify update using an empty changelog on a Mac takes less than 30 seconds. PASS

$ time liquibase update --changelog-file blank-changelog.sql

Trial 1 Trial 2 Trial 3
real 0m2.377s real 0m2.376s real 0m2.471s
user 0m5.976s user 0m5.831s user 0m5.522s
sys 0m0.373s sys 0m0.356s sys 0m0.343s
  • See Limitations in Test Methodology for more information.

Limitations in Test Methodology

  • The reproduction steps to see localhost in the DATABASECHANGELOGLOCK.LOCKEDBY column did not work for me. On Windows, the Liquibase CLI correctly gets the hostname + hostaddress (IP) and uses those to populate the LOCKEDBY value. On Mac, however, even with the fix, Liquibase only ever gets hostaddress and the LOCKEDBY value ends up being hostaddress(hostaddress). This is entered as Issue 2539.

  • There are no performance benchmarks available for empirical analysis of the speed of NetUtils.getLocalHost(). Anecdotally, my Mac performs Liquibase operations slower than on my Windows machine, but this is likely a moot point, as the two machines do not have equivalent system resources.

  • The tests in liquibase-core/src/test/groovy/liquibase/util/NetUtilTest.groovy ran significantly faster on master than on the fix branch; ~80ms on master and ~428ms on the fix branch. I am sending this back to @nvoxland to take a look.


Issues Logged During Test (loosely related to this PR)
Issue 2539
Issue 2537


Test Environment
Liquibase Core fix/slow_get_localhost_on_mac/1081/aa2358, Pro: master/343/d9abdc
Mac Big Sur (11.4)
Windows 10
Passing Functional Tests

@XDelphiGrl XDelphiGrl assigned nvoxland and unassigned XDelphiGrl Feb 15, 2022
@nvoxland
Copy link
Contributor

I don't see anything new in master that would make this be so much faster in master vs. here besides changes to this logic.

If you're seeing this code faster on you rmac vs the old version, @grzi , we'll have to do more research into why and what the best logic to use is. I don't want to just accept this PR and end up making it slower for a majority of people.

@kataggart kataggart removed this from Ready for Handoff (In JIRA) in Conditioning++ Sep 2, 2022
@nvoxland nvoxland assigned MalloD12 and unassigned nvoxland Sep 27, 2022
…external address does not have a useful host name attached to it.
@MalloD12 MalloD12 changed the base branch from master to 1_9 October 4, 2022 21:06
@MalloD12 MalloD12 changed the base branch from 1_9 to master October 4, 2022 21:08
@nvoxland nvoxland added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label Oct 4, 2022
@github-actions
Copy link

github-actions bot commented Oct 4, 2022

Unit Test Results

  4 668 files    4 668 suites   31m 16s ⏱️
  4 627 tests   4 401 ✔️    226 💤 0
54 528 runs  49 465 ✔️ 5 063 💤 0

Results for commit a2caa67.

♻️ This comment has been updated with latest results.

@nvoxland nvoxland added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Oct 6, 2022
@nvoxland
Copy link
Contributor

nvoxland commented Oct 6, 2022

I guess I see that there is some additional caching in NetUtil to avoid so many hostname/IP lookups. That must be the main change.

@MalloD12 fixed up the hostname handling so it will check localhost for a hostname when the external interface's hostname is not set. That is the code used by the databasechangeloglock description that you were seeing off, @XDelphiGrl.

It's ready to test again

@FBurguer
Copy link

Tested this PR against MacOS and it looks like its showing the hostname instead of localhost now.

@nvoxland nvoxland merged commit e245093 into liquibase:master Oct 11, 2022
@nvoxland nvoxland added this to the 1NEXT milestone Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DBAll ImpactLow IntegrationAnt performance SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions Severity4 sprint2022-35 SyncTicket TypeEnhancement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

NetUtils.getLocalHostName() takes 30s
8 participants