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

generate unit file for st2api, st2auth and st2stream #721

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

nzlosh
Copy link
Contributor

@nzlosh nzlosh commented Jul 15, 2022

Move api, auth and stream service unit files to generator so st2.conf variables are used consistently for .socket and .service files.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Looks like a bit of copy pasta. I left suggestions to fix the ports.

Also, I wonder if we could combine these so one generator handles all 3 services. But it's probably not worth the effort.

packages/st2/debian/st2auth-generator Outdated Show resolved Hide resolved
packages/st2/debian/st2stream-generator Outdated Show resolved Hide resolved
User=st2
Group=st2
Environment="DAEMON_ARGS=-k eventlet -b 127.0.0.1:9101 --workers 1 --threads 1 --graceful-timeout 10 --timeout 30 --log-config /etc/st2/logging.api.gunicorn.conf --error-logfile /var/log/st2/st2api.log"
EnvironmentFile=-/etc/default/st2api
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that the generator does not include EnvironmentFile any more.

One good thing, is that makes the generator truly agnostic between debian vs EL.

One potential gotcha is that if anyone is relying on this, then that file will no longer be sourced. They can, however, add EnvironmentFile to a systemd service override file /etc/systemd/system/st2api.conf.d/some-file-name-here.conf. That can also be used to override anything else if desired. (The same applies to st2auth and st2stream)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, the /etc/default/st2api configuration file is how Debian based distributions allowed overriding sysv and upstart service scripts. As you've rightly pointed out, people should use the built-in systemd override mechanism and all currently supported OS' are systemd based. We can highlight this in the v3.8 upgrade notes.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

I'm pretty sure tests are failing because this line is having an error that has been silenced:

%service_post st2actionrunner st2api st2stream st2auth st2notifier st2workflowengine

That line lists the 6 services that aren't getting enabled correctly.

Comment on lines 6 to 8
# Enable services created by systemd generator
systemctl enable st2api st2auth st2stream >/dev/null 2>&1 || true
systemctl start st2api st2auth st2stream >/dev/null 2>&1 || true
Copy link
Member

@cognifloyd cognifloyd Oct 10, 2022

Choose a reason for hiding this comment

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

Looking at the test error for EL distros:

I think this is already handled by the %post macro which uses the %service_post helper. That runs:

systemctl --no-reload enable %{?*} >/dev/null 2>&1 || : \

What if you remove the output redirection in the %service_post macro helper? Then maybe we'll see if there's an error. And maybe remove it here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There appears to be a worker timeout for gunicorn on EL7, but I don't think that's the cause of the services failing to start for the RPM based distros. I still don't see any errors in the logs that indicate why the service is not enabled and started.

@cognifloyd
Copy link
Member

aargh. There's still no helpful output during yum install. I don't know which verbosity flag(s) to use (--verbose or --rpmverbosity or both?), but what if we add some verbose settings on this line:

sudo yum -y install $(lookup_fullnames $@);

scripts/install_os_packages.sh gets called in the package tests here:

execute :bash, "$BASEDIR/scripts/install_os_packages.sh #{opts[:package_list]}"

We need to figure out why the %post bits aren't running like we expect. ☹️

@nzlosh
Copy link
Contributor Author

nzlosh commented Oct 11, 2022

I'm going to debug this on a VM, pushing and waiting for the CI to fail is a hindrance and unreasonably slow.

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.

2 participants