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

re-enable proxy service fails on execstart integration test #627

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

engelmi
Copy link
Member

@engelmi engelmi commented Nov 1, 2023

Fixes: #320

This PR re-enables the - currently - skipped integration test for the proxy service where the ExecStart returns exit code 1.
Running it locally a few times it works fine, so lets enable it again and see if the flakiness is gone.

@engelmi engelmi requested a review from mwperina November 1, 2023 06:59
@engelmi engelmi requested a review from rhatdan as a code owner November 1, 2023 06:59
@engelmi engelmi force-pushed the reenable-proxy-service-fails-on-execstart-test branch 2 times, most recently from 28c9d13 to 00fa431 Compare November 1, 2023 07:56
@engelmi engelmi force-pushed the reenable-proxy-service-fails-on-execstart-test branch 3 times, most recently from 8aee893 to 6849584 Compare November 2, 2023 10:25
@alexlarsson
Copy link
Contributor

Actually, I wonder if this is the right approach.
Do we even need to run /bin/true?
The docs for ExacStart say:

When Type=oneshot is used, zero or more commands may be specified
If no ExecStart= is specified, then the service must have RemainAfterExit=yes and at least one ExecStop= line set. (Services lacking both ExecStart= and ExecStop= are not valid.)
Maybe just having an ExecStop is better? It will start quicker, and we don't risk this race.

@alexlarsson
Copy link
Contributor

Another option is to add the kill signal to SuccessExitStatus=

@engelmi engelmi force-pushed the reenable-proxy-service-fails-on-execstart-test branch 2 times, most recently from 34effde to c8e960c Compare November 6, 2023 09:44
@engelmi engelmi force-pushed the reenable-proxy-service-fails-on-execstart-test branch from c8e960c to a3dc382 Compare November 7, 2023 07:58
By using ExecStop= instead of ExecStart= a race condition is
avoided that leads systemd to send a SIGTERM signal to the
dep service, resulting in it being in a failed state.

Signed-off-by: Michael Engel <[email protected]>
@engelmi engelmi force-pushed the reenable-proxy-service-fails-on-execstart-test branch from a3dc382 to 987d0bd Compare November 7, 2023 08:00
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

LGTM

@mwperina mwperina merged commit f65493e into main Nov 7, 2023
@mwperina mwperina deleted the reenable-proxy-service-fails-on-execstart-test branch November 7, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration test for ExecStart failure is flaky
3 participants