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.

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

Add support for dynflow_sidekiq #292

Merged
merged 6 commits into from
Jan 22, 2020
Merged

Add support for dynflow_sidekiq #292

merged 6 commits into from
Jan 22, 2020

Conversation

adamruzicka
Copy link
Contributor

Still WIP, opening in current state for visibility

@adamruzicka adamruzicka marked this pull request as ready for review January 20, 2020 14:18
ehelms
ehelms reviewed Jan 20, 2020
View reviewed changes
definitions/features/redis.rb Outdated
@@ -0,0 +1,22 @@
class Features::Redis < ForemanMaintain::Feature
SCL_PREFIX = 'rh-redis5'.freeze
SERVICE_NAME = "#{SCL_PREFIX}-redis".freeze
Copy link
Member

Choose a reason for hiding this comment

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

This could be fine for now, but will be an issue for Debian if ever used there. This will also break on EL8 when we land there. Unfortunately other SCLs have syspaths packages to allow using the classic service names. Redis does not appear to. @evgeni have you seen anything? would it be worth our shipping our own?

Copy link
Member

Choose a reason for hiding this comment

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

We can ask the SCL team to provide one? :)

@ehelms
Copy link
Member

ehelms commented Jan 20, 2020

This seems generally good to go. Can it be dropped from WIP state? We will need this both merged and released to prevent nightly breakages when the corresponding installer changes are merged.

@ekohl ekohl mentioned this pull request Jan 20, 2020
Merged
1 task
ekohl
ekohl reviewed Jan 20, 2020
View reviewed changes
definitions/features/redis.rb Outdated

def config_files
["/etc/opt/rh/#{SCL_PREFIX}/redis",
"/etc/opt/rh/#{SCL_PREFIX}/redis.conf"]
Copy link
Member

Choose a reason for hiding this comment

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

These paths will also be different when there is no SCL. On non-SCL (EL8), it's /etc/redis.conf. Debian(-based) uses /etc/redis/redis.conf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handled it similarly as with the scl prefix. When support for el8/deb lands, the etc_prefix method will need to contain a condition checking current platform

definitions/features/redis.rb Outdated
end

def config_files
["/etc/opt/rh/#{SCL_PREFIX}/redis",
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a directory puppet-redis adds to be ready for multiple instances but we don't intend to use those. AFAIK the RPM doesn't create this directory. Debian(-based) does have such a directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be safe, if a file does not exist, f-m will just skip it. I'd say it is better to try to collect everything there is

definitions/features/dynflow_sidekiq.rb Outdated Show resolved Hide resolved
@adamruzicka
Fixes #28329 - Turn constants into methods
1f24eb6 
This should allow us to modify them more easily when running on different
platforms.
@adamruzicka adamruzicka changed the title [WIP] Add support for dynflow_sidekiq Add support for dynflow_sidekiq Jan 21, 2020
@adamruzicka
Fixes #28329 - Handle redis config files paths better
194351c 
This should allow us to modify them more easily when running on different
platforms.
ekohl
ekohl approved these changes Jan 21, 2020
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

From the platform point of view this looks like a good first version. Can't speak for foreman-maintain itself.

@ehelms
Copy link
Member

ehelms commented Jan 21, 2020

@kgaikwad Would a merge and release of this be possible within the next day? This would allow us to merge and get into nightly the new dynflow.

kgaikwad
kgaikwad suggested changes Jan 21, 2020
View reviewed changes
definitions/features/redis.rb

confine do
# Luckily, the service name is the same as the package providing it
server? && find_package(service_name)
Copy link
Member

Choose a reason for hiding this comment

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

Given block passed as argument to metadata and its execution happens outside of this feature scope so it will raise an error for service_name.

Either you can declare one more constant or use "#{SCL_NAME}-redis" directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, forgot about that. Fixed in a slightly different way

definitions/features/foreman_tasks.rb
@@ -108,7 +108,7 @@ def fetch_tasks_status(state, spinner)
end

def services
[system_service(service_name, 30)]
feature(:dynflow_sidekiq) ? [] : [system_service(service_name, 30)]
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean rest of things from feature foreman_tasks remain as it is and only services get changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK foreman-tasks hasn't had a service ever since dynflowd was introduced. dynflowd is part of Foreman itself while foreman-tasks is an optional plugin. dynflowd.service always had the alias foreman-tasks so systemctl status foreman-tasks continued to work.

kgaikwad
kgaikwad reviewed Jan 21, 2020
View reviewed changes
Copy link
Member

@kgaikwad kgaikwad left a comment

Choose a reason for hiding this comment

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

Thank you @adamruzicka.
Rubocop is not happy :-) I will merge this PR once tests become green.

@adamruzicka
Copy link
Contributor Author

We're 🍏

@kgaikwad kgaikwad merged commit b3a0cc4 into theforeman:master Jan 22, 2020
@kgaikwad
Copy link
Member

Thanks @adamruzicka!

@adamruzicka adamruzicka deleted the dynflow-backup branch January 22, 2020 10:06
@kgaikwad
Copy link
Member

@adamruzicka, @ehelms,
Released a new version - foreman-maintain 0.6.0.

My Apologies, I didn't noticed all commit messages before merging this PR so except first inital commit I have modified the remaining commit messages from Fixes #xxxx to Refs #xxxx.
Also, it would be good to have some test coverage for new changes later in a separate PR.

@ehelms
Copy link
Member

ehelms commented Jan 22, 2020

@kgaikwad thanks so much for quick reviews and the release!

@adamruzicka
Copy link
Contributor Author

Thank you @kgaikwad , I'll add the tests to my todo.

@kgaikwad kgaikwad mentioned this pull request Apr 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evgeni evgeni evgeni left review comments

@ekohl ekohl ekohl approved these changes

@ehelms ehelms ehelms approved these changes

@kgaikwad kgaikwad kgaikwad approved these changes

Assignees
No one assigned
Labels
Needs testing Waiting on contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@adamruzicka @ehelms @kgaikwad @evgeni @ekohl @theforeman-bot