Skip to content

core: fix logic of merging units #25387

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.

Sign up for GitHub

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

Jump to bottom
Merged
bluca merged 7 commits into from
Dec 17, 2022
Merged

core: fix logic of merging units #25387

bluca merged 7 commits into from
Dec 17, 2022

Conversation

@yuwata
Copy link
Member

@yuwata yuwata commented Nov 15, 2022 •
edited
Loading

Fixes several bugs introduced by 15ed3c3 (v249).

Fixes #24990. Also, hopefully fixes #24577.

@yuwata yuwata linked an issue Nov 15, 2022 that may be closed by this pull request
init: Assertion 'hashmap_isempty(m->jobs)' failed at src/core/manager.c:1474, function manager_clear_jobs_and_units(). Aborting. #24577
Closed
@yuwata yuwata mentioned this pull request Nov 15, 2022
Closed
@yuwata yuwata added this to the v253 milestone Nov 15, 2022
@yuwata yuwata requested a review from poettering November 15, 2022 14:19
@yuwata yuwata added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Nov 15, 2022
@yuwata yuwata removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Nov 15, 2022
@yuwata yuwata changed the title core: fix unit GC logic core: fix unit garbage collection and merging units logic Nov 15, 2022
@yuwata
Copy link
Member Author

yuwata commented Nov 15, 2022

(Hm, still GC logic needs to be updated.)

@yuwata yuwata changed the title core: fix unit garbage collection and merging units logic core: fix logic of merging units Nov 15, 2022
@yuwata
Copy link
Member Author

yuwata commented Nov 15, 2022 •
edited
Loading

The commit for unit GC is dropped. PTAL.

@yuwata yuwata mentioned this pull request Nov 17, 2022
Open
@evverx evverx mentioned this pull request Nov 27, 2022
Closed
keszybz
keszybz requested changes Dec 2, 2022
View reviewed changes
@keszybz keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 2, 2022
@yuwata yuwata force-pushed the core-fix-gc-logic branch from f90143e to 2e30309 Compare December 5, 2022 05:31
@yuwata yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 5, 2022
@yuwata
Copy link
Member Author

yuwata commented Dec 5, 2022

@keszybz Thank you for the review. All points, except for the bashism in the test, are addressed. PTAL!

keszybz
keszybz reviewed Dec 5, 2022
View reviewed changes
yuwata added 2 commits December 6, 2022 17:49
@yuwata
test: add test case for sysv-generator and invalid dependency
5f882cc
@yuwata
core/unit: merge unit names after merging deps
1d0c81a 
Before:
systemd[1]: issue-24990.service: Dependency Before=n/a dropped, merged into issue-24990.service
After:
systemd[1]: issue-24990.service: Dependency Before=test1.service dropped, merged into issue-24990.service
@yuwata yuwata force-pushed the core-fix-gc-logic branch from 2e30309 to 4f41ccd Compare December 6, 2022 08:59
@keszybz keszybz added the please-review PR is ready for (re-)review by a maintainer label Dec 8, 2022
keszybz
keszybz reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

LaGTM. (almost good)

@keszybz keszybz added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels Dec 15, 2022
@yuwata
core/unit: fix log message
ed99116 
As you can see in the below, the dropped dependency Before=issue-24990.service
is not logged, but the dependency Before=test1.service which is not owned by
the units generated by the TEST-26 is logged.

Before:
systemd[1]: issue-24990.service: Dependency After=test1.service dropped, merged into issue-24990.service
systemd[1]: issue-24990.service: Dependency Before=test1.service dropped, merged into issue-24990.service

After:
systemd[1]: issue-24990.service: Dependency After=test1.service is dropped, as test1.service is merged into issue-24990.service.
systemd[1]: issue-24990.service: Dependency Before=issue-24990.service in test1.service is dropped, as test1.service is merged into issue-24990.service.
@yuwata
Copy link
Member Author

yuwata commented Dec 15, 2022

@keszybz Thank you for your review. All comments are addressed. Upgrading the green label.

LaGTM. (almost good)

I like the abbreviation!

@yuwata yuwata 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 Dec 15, 2022
poettering
poettering reviewed Dec 16, 2022
View reviewed changes
src/core/unit.c
/* This is a dependency pointing back to the unit we want to merge with?
* Suppress it (but warn) */
unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt));
hashmap_remove(other_deps, back);
Copy link
Member

@poettering poettering Dec 16, 2022

Choose a reason for hiding this comment

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

hmm, now you are changing the hashmap you are iterating through. It's something I so far avoided

@poettering
Copy link
Member

poettering commented Dec 16, 2022

looks good, but iterating through a hashmap that we modify is problematic, iirc our hashmap implementation is not safe against arbitrary changes during iteration

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks 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 Dec 16, 2022
@yuwata
Copy link
Member Author

yuwata commented Dec 17, 2022

looks good, but iterating through a hashmap that we modify is problematic, iirc our hashmap implementation is not safe against arbitrary changes during iteration

It is safe. Please see the comment in hashmap_iterate_in_internal_order().

@yuwata yuwata 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 reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Dec 17, 2022
@bluca bluca merged commit 896785a into systemd:main Dec 17, 2022
@yuwata yuwata deleted the core-fix-gc-logic branch December 17, 2022 14:05
@evverx evverx mentioned this pull request Jan 1, 2023
Closed
@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 Feb 10, 2023
@keszybz
Copy link
Member

keszybz commented Feb 10, 2023

This was already included in v252.4.

@evverx evverx mentioned this pull request Mar 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@keszybz keszybz keszybz requested changes

@poettering poettering poettering left review comments

@bluca bluca bluca approved these changes

Assignees

No one assigned

Labels

pid1

Milestone

v253

4 participants

@yuwata @poettering @keszybz @bluca