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

hostnamed: stop caching so much #15624

Merged
merged 2 commits into from Jun 25, 2020
Merged

Conversation

poettering
Copy link
Member

Let's always report current settings instead of what we read while initializing. It's less confusing to users, as it makes hostnamed a much thinner layer around the backing logic.

Prompted by this thread:

https://lists.freedesktop.org/archives/systemd-devel/2020-April/044395.html

@poettering poettering force-pushed the hostnamed-instant branch 4 times, most recently from 58527ba to 25d7777 Compare April 30, 2020 07:28
man/sd_bus_add_object.xml Outdated Show resolved Hide resolved
src/hostname/hostnamed.c Outdated Show resolved Hide resolved
src/hostname/hostnamed.c Outdated Show resolved Hide resolved
@poettering
Copy link
Member Author

force pushed with the suggested changes. no other changes.

(i added a pretty long and nice comment to that property handling code, i hope this is very clear now...)

@poettering
Copy link
Member Author

(eventually we should add the same logic to localed and timedated, btw)

@keszybz keszybz added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Apr 30, 2020
@keszybz
Copy link
Member

keszybz commented May 1, 2020

======================================================================
FAIL: test_transient_hostname_with_static (__main__.DnsmasqClientTest)
transient hostname is not applied if static hostname exists
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/autopkgtest.bc9Q5g/build.YAG/systemd/test/networkd-test.py", line 794, in test_transient_hostname_with_static
    self.assertEqual(socket.gethostname(), "foobarqux")
AssertionError: 'testgreen' != 'foobarqux'
- testgreen
+ foobarqux


----------------------------------------------------------------------
Ran 36 tests in 217.007s

FAILED (failures=1, skipped=2)
autopkgtest [21:27:51]: test networkd-test.py: -----------------------]
networkd-test.py     FAIL non-zero exit status 1

Looks related ;(

@keszybz keszybz added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels May 1, 2020
@keszybz
Copy link
Member

keszybz commented May 1, 2020

... it is quite possible that the error is in the test, but it needs to be reconciled anyway.

@poettering
Copy link
Member Author

Split the first four commits into #15804. Hopefully they pas and we can merge them before the others.

keszybz added a commit that referenced this pull request May 18, 2020
keszybz added a commit that referenced this pull request May 19, 2020
Copy link

@hixio-mh hixio-mh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pul requested

Querying the current hostname is cheap, hence let's not cache it. That
way it is much less likely we'll return out-of-date data.
…c/machine-info

Instead of reading these files at startup and never again, let's read
them when we need them. As an optimization (in particular as some of
these files contain the data for many fields at once) let's cache the
results as long as the stat data (i.e. mtime) remains stable.

Also, while we are at it, if we can't read any of these props, let's not
fail everything, but continue without the data.
@poettering poettering removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Jun 25, 2020
@poettering poettering added this to the v246 milestone Jun 25, 2020
@poettering
Copy link
Member Author

So seams I finally tracked this one down. The fix was trivial ultimately: not only when clients query the hostname and other data from hostnamed we need to possibly refresh the data from what#s on disk, but also when we make changes to it, since we actually take the status quo into consideration too when figuring out how the updated data is to be propagated to the kernel's setting, and similar.

So I added a few calls to re-read data on disk in the setters in addition to the getters and everything worked.

@keszybz already liked this once, given the trivial fix I figure it should be OK to set the green label again?

@poettering poettering added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jun 25, 2020
@keszybz
Copy link
Member

keszybz commented Jun 25, 2020

LGTM.

@keszybz
Copy link
Member

keszybz commented Jun 25, 2020

bionic-amd64 is still outstanding, but it's been slow lately, and I don't think this is in any way arch-specific, so let's not wait for it.

@keszybz keszybz merged commit 66ed28d into systemd:master Jun 25, 2020
@keszybz keszybz removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants