Skip to content

Rely on the service_provider fact#694

Merged
ekohl merged 2 commits intotheforeman:masterfrom
ekohl:service-fact
Feb 4, 2022
Merged

Rely on the service_provider fact#694
ekohl merged 2 commits intotheforeman:masterfrom
ekohl:service-fact

Conversation

@ekohl
Copy link
Copy Markdown
Member

@ekohl ekohl commented Apr 17, 2019

This avoids hardcoding the service provider and greatly simplifies the code.

@mmoll
Copy link
Copy Markdown
Contributor

mmoll commented Apr 17, 2019

@ekohl tests fail 💔

@mmoll
Copy link
Copy Markdown
Contributor

mmoll commented Apr 17, 2019

is this a 12.0.0 candidate?

@ekohl
Copy link
Copy Markdown
Member Author

ekohl commented Apr 17, 2019

No, it's a branch I had locally that I thought I should finish at some point. I'd rather get 12.0.0 out now and merge this later.

@mmoll
Copy link
Copy Markdown
Contributor

mmoll commented Nov 26, 2019

what's the status here?

@ekohl ekohl force-pushed the service-fact branch 2 times, most recently from 9f89e2d to 5c08590 Compare March 5, 2020 19:00
@ekohl
Copy link
Copy Markdown
Member Author

ekohl commented Mar 5, 2020

Rebased and updated the tests

@mmoll
Copy link
Copy Markdown
Contributor

mmoll commented Mar 5, 2020

test failures related?

@ekohl
Copy link
Copy Markdown
Member Author

ekohl commented Apr 13, 2020

I'm not entirely sure how to explain the test failures. It's also annoying we don't have EL7 tests now so that should be fixed first by using an older image.

@ekohl ekohl marked this pull request as draft April 13, 2020 15:24
@ekohl ekohl mentioned this pull request Apr 13, 2020
Merged
@ekohl
Copy link
Copy Markdown
Member Author

ekohl commented Mar 29, 2021

So that does appear to be the problem:

2021-03-29T15:04:07.1177819Z debian9-64.example.com 15:04:07$ facter --json "service_provider"
2021-03-29T15:04:07.2509850Z   {
2021-03-29T15:04:07.2510867Z     "service_provider": ""
2021-03-29T15:04:07.2517137Z   }

@ekohl
Copy link
Copy Markdown
Member Author

ekohl commented Mar 29, 2021

Thinking more about this, it may well be that facter doesn't include the custom Puppet facts.

@ekohl ekohl force-pushed the service-fact branch 2 times, most recently from f85d957 to e0ab4c5 Compare February 4, 2022 13:00
ekohl added 2 commits February 4, 2022 17:33
@ekohl
Rely on the service_provider fact
97713f2 
This avoids hardcoding the service provider and greatly simplifies the
code.

It changes the path to systemctl to /bin/systemctl which exists on
Debian. On Red Hat /bin is a symlink to /usr/bin so it also works.
@ekohl
Increase maxium heap size
274ad0c 
This was way too conservative and caused puppetserver to run out of heap
space. This gives it a bit more breathing room.
@ekohl
Copy link
Copy Markdown
Member Author

ekohl commented Feb 4, 2022

After such a long time I finally found out the problem and it was really stupid: I used /usr/bin/systemctl but on Debian it's /bin/systemctl. Somehow Puppet didn't raise an error about the command not being found and it fell back to a real restart instead of a reload. Let's see if CI agrees.

@ekohl ekohl marked this pull request as ready for review February 4, 2022 17:00
@ekohl
Copy link
Copy Markdown
Member Author

ekohl commented Feb 4, 2022

This finally passes. The other failures are unrelated.

@ekohl ekohl mentioned this pull request Feb 4, 2022
Merged
wbclark
wbclark reviewed Feb 4, 2022
View reviewed changes
manifests/params.pp
$agent_restart_command = "/usr/sbin/service ${service_name} reload"
$unavailable_runmodes = []
if $facts['service_provider'] == 'systemd' {
$agent_restart_command = "/bin/systemctl reload-or-restart ${service_name}"
Copy link
Copy Markdown
Contributor

@wbclark wbclark Feb 4, 2022

Choose a reason for hiding this comment

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

Nice, TIL

wbclark
wbclark reviewed Feb 4, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@wbclark wbclark left a comment

Choose a reason for hiding this comment

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

What if any is the relationship between these two commits? Is the change to server_jvm_max_heap_size better suited to a separate PR?

@ekohl
Copy link
Copy Markdown
Member Author

ekohl commented Feb 4, 2022

It's something I found while testing this manually. Technically it could be a separate PR, but I think there's little value in that. It is only for the acceptance tests so it's not like users need to know about it via the changelog. Combining them saves CI resources.

@wbclark
Copy link
Copy Markdown
Contributor

wbclark commented Feb 4, 2022

It's something I found while testing this manually. Technically it could be a separate PR, but I think there's little value in that. It is only for the acceptance tests so it's not like users need to know about it via the changelog. Combining them saves CI resources.

Ah, got it, TY

@ekohl ekohl merged commit d22d5ed into theforeman:master Feb 4, 2022
@ekohl ekohl deleted the service-fact branch February 4, 2022 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wbclark wbclark wbclark approved these changes

Assignees

No one assigned

Labels

Enhancement Needs testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ekohl @mmoll @wbclark @theforeman-bot @zjhuntin