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

Fixes #32235,#19494 - Run Dynflow within smart-proxy on EL* #655

Merged
merged 2 commits into from
Mar 31, 2021

Conversation

adamruzicka
Copy link
Contributor

@adamruzicka adamruzicka commented Mar 23, 2021

@ehelms
Copy link
Member

ehelms commented Mar 26, 2021

You could pull https://github.com/theforeman/puppet-foreman_proxy/pull/655/files/7103a1a6e7aad3559198a760479f7a7a1a7f9419..b6c140d71a7e4b467d6adf82469c8da528b5632f#diff-30599268ecf267469b31876ad92802766e3596423d310db9aa425b7db9cc2beeR79 out of the if-else and set the properties on it based on external_core.

    service { 'smart_proxy_dynflow_core':
      ensure => $external_core,
      enable => $external_core,
    }

@adamruzicka
Copy link
Contributor Author

Is it worth it? I mean the entire true branch will get dropped once we get to stage 3 of the proposal

@ehelms
Copy link
Member

ehelms commented Mar 26, 2021

Is it worth it? I mean the entire true branch will get dropped once we get to stage 3 of the proposal

Will that happen this Foreman release or afterwards? (stage 3)

@adamruzicka
Copy link
Contributor Author

Most likely in the next one

@ehelms
Copy link
Member

ehelms commented Mar 29, 2021

Should I see the dynflow plugin present and working inside the smart-proxy on a Katello install?

@adamruzicka
Copy link
Contributor Author

If it was a fresh install then yes. If an upgrade (or if installer was run before applying patches from this PR) then no. We'll most likely need an installer migration

@ehelms
Copy link
Member

ehelms commented Mar 29, 2021

If it was a fresh install then yes

I think we are missing this in the answers file, so likely need updates all around after I grepped through -- https://github.com/theforeman/foreman-installer/blob/develop/config/katello-answers.yaml

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.

Overall this looks good.

manifests/plugin/dynflow.pp Outdated Show resolved Hide resolved
manifests/plugin/dynflow.pp Show resolved Hide resolved
manifests/plugin/dynflow/params.pp Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
spec/classes/foreman_proxy__plugin__dynflow_spec.rb Outdated Show resolved Hide resolved
manifests/plugin/dynflow.pp Show resolved Hide resolved
@ekohl
Copy link
Member

ekohl commented Mar 30, 2021

Also, can we open an issue in the installer Redmine project?

@ekohl ekohl changed the title Do not enable external dynflow core on EL* Fixes #32235 - Run Dynflow within smart-proxy on EL* Mar 31, 2021
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.

This looks good to me. I edited the PR title to what I think is a bit better description of what it's achieving. Do you want to squash the commits, should I or is it intention that it's multiple commits?

@adamruzicka
Copy link
Contributor Author

Squashed the commits into two, one for running dynflow within smart proxy, the other for setting open file limits for smart proxy.

@ekohl
Copy link
Member

ekohl commented Mar 31, 2021

Should I move https://projects.theforeman.org/issues/19494 to the Installer project?

We did this for standalone smart_proxy_dynflow_core, we need to do it for
all-in-one deployments as well.
@ekohl ekohl changed the title Fixes #32235 - Run Dynflow within smart-proxy on EL* Fixes #32235,#19494 - Run Dynflow within smart-proxy on EL* Mar 31, 2021
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.

Thanks! Merging this and we'll see how it goes in nightly.

@ekohl
Copy link
Member

ekohl commented Mar 31, 2021

Since @ehelms also reviewed, I'll give him a chance for a last look over.

@ehelms ehelms merged commit 941e934 into theforeman:master Mar 31, 2021
@adamruzicka adamruzicka deleted the no-spdc branch March 31, 2021 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants