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 #21565 - Orchestration can now optionally be enabled during unattended operations #4976

Merged
merged 1 commit into from Nov 29, 2017

Conversation

@fabianvf
Copy link
Contributor

@fabianvf fabianvf commented Nov 2, 2017

http://projects.theforeman.org/issues/21565

I wasn't really sure what tests would look like for a change like this, would appreciate some guidance on that front.

@theforeman-bot
Copy link
Member

@theforeman-bot theforeman-bot commented Nov 2, 2017

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

@theforeman-bot theforeman-bot commented Nov 2, 2017

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

@theforeman-bot
Copy link
Member

@theforeman-bot theforeman-bot commented Nov 2, 2017

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

@theforeman-bot
Copy link
Member

@theforeman-bot theforeman-bot commented Nov 2, 2017

Issues: #21565

@theforeman-bot
Copy link
Member

@theforeman-bot theforeman-bot commented Nov 2, 2017

There were the following issues with the commit message:

  • length of the first commit message line for deb154f exceeds 65 characters
  • commit message for deb154f is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@iNecas
Copy link
Member

@iNecas iNecas commented Nov 3, 2017

I guess

host.expects(:enable_orchestration!).never
can be used for inspiration

@@ -294,7 +294,7 @@ def handle_ca
def import_facts(facts, source_proxy = nil)
# Facts come from 'existing' attributes/infrastructure. We skip triggering
# the orchestration of this infrastructure when we create a host this way.
skip_orchestration! if SETTINGS[:unattended]
skip_orchestration! if SETTINGS[:unattended] and not SETTINGS[:unattended_orchestration]

This comment has been minimized.

@iNecas

iNecas Nov 3, 2017
Member

Prefer && over and (there is slight difference in operator preferrence). Also ! is generally preferred in Foreman codebase.

@iNecas
Copy link
Member

@iNecas iNecas commented Nov 3, 2017

ok to test

@fabianvf fabianvf force-pushed the fabianvf:21565-unattended_orchestration branch from 8808bb2 to a66ed2f Nov 3, 2017
@theforeman-bot
Copy link
Member

@theforeman-bot theforeman-bot commented Nov 3, 2017

There were the following issues with the commit message:

  • 501999d must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@@ -161,6 +161,42 @@ def teardown
host.expects(:enable_orchestration!).never
assert host.import_facts(raw['facts'])
end

This comment has been minimized.

@houndci-bot

houndci-bot Nov 3, 2017

Extra empty line detected at block body end.

@fabianvf fabianvf force-pushed the fabianvf:21565-unattended_orchestration branch from 501999d to 3d37656 Nov 3, 2017
@theforeman-bot
Copy link
Member

@theforeman-bot theforeman-bot commented Nov 3, 2017

There were the following issues with the commit message:

  • 3d37656 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@fabianvf
Copy link
Contributor Author

@fabianvf fabianvf commented Nov 3, 2017

@iNecas thanks for the testing tip, very helpful

@@ -163,6 +163,41 @@ def teardown
end
end

test "should trigger orchestration when importing facts if unattended_orchestration is true" do

This comment has been minimized.

@iNecas

iNecas Nov 6, 2017
Member

Please use context block, and setup and teardown for setting up the settings: the reason is, that if something fails during the the test method, the setting to the original value would not happen

Copy link
Member

@lzap lzap left a comment

This is more about fact import than anything unattended. Please figure out a different name and description which will be more clear. Also performance impact of this setting are huge - fact importing is probably the most important API in terms of performance.

I think this flag will be dangerous and can make troubleshooting more difficult. This needs to have clear name, description and documentation in our guide.

@@ -46,6 +46,7 @@ def self.default_settings
self.set('access_unattended_without_build', N_("Allow access to unattended URLs without build mode being used"), false, N_('Access unattended without build')),
self.set('manage_puppetca', N_("Foreman will automate certificate signing upon provision of new host"), true, N_('Manage PuppetCA')),
self.set('ignore_puppet_facts_for_provisioning', N_("Stop updating IP address and MAC values from Puppet facts (affects all interfaces)"), false, N_('Ignore Puppet facts for provisioning')),
self.set('unattended_orchestration', N_("Enable host orchestration during unattended operations"), false, N_('Enable unattended orchestration')),

This comment has been minimized.

@lzap

lzap Nov 7, 2017
Member

This is not good description, this flag for example will break discovery plugin.

This comment has been minimized.

@fabianvf

fabianvf Nov 7, 2017
Author Contributor

Interesting, I definitely don't want to break discovery... The motivation for this is that I just want my hosts (which are all provisioned from discovered hosts) to keep DNS in sync with the host, which is updated by the puppet fact uploads. I knew there were potential performance issues but I wasn't aware that it would break any existing behaviors. Is there a better way I can go about this so that it won't affect the discovery plugin?

This comment has been minimized.

@lzap

lzap Nov 8, 2017
Member

Ok I take it back, it does not break discovery. Go head but I think the name and description is a little bit confusing, this has something to do with fact importing.

This comment has been minimized.

@fabianvf

fabianvf Nov 8, 2017
Author Contributor

do you think something like enable_orchestration_on_fact_import would be more fitting?

This comment has been minimized.

@fabianvf

fabianvf Nov 8, 2017
Author Contributor

With a big old performance disclaimer in the description

This comment has been minimized.

@lzap

lzap Nov 13, 2017
Member

That sounds reasonable to me, don't need to be big, just ordinary description about what is going on :-)

@@ -294,7 +294,7 @@ def handle_ca
def import_facts(facts, source_proxy = nil)
# Facts come from 'existing' attributes/infrastructure. We skip triggering
# the orchestration of this infrastructure when we create a host this way.
skip_orchestration! if SETTINGS[:unattended]
skip_orchestration! if SETTINGS[:unattended] && !SETTINGS[:unattended_orchestration]
super
ensure
enable_orchestration! if SETTINGS[:unattended]

This comment has been minimized.

@lzap

lzap Nov 8, 2017
Member

The same change must be on this line as well.

@lzap
lzap approved these changes Nov 8, 2017
Copy link
Member

@lzap lzap left a comment

  • The same change must be on this line as well
  • Figure out better name somehing like: Orchestrate during fact import
This adds the enable_orchestration_on_fact_import setting, which allows
a user to make Foreman perform host orchestration operations when a
host is updated from puppet fact upload
@fabianvf fabianvf force-pushed the fabianvf:21565-unattended_orchestration branch from e58bfa8 to d254e22 Nov 13, 2017
@fabianvf
Copy link
Contributor Author

@fabianvf fabianvf commented Nov 17, 2017

@lzap pushed up changes

@lzap
lzap approved these changes Nov 29, 2017
@lzap lzap merged commit bd33211 into theforeman:develop Nov 29, 2017
7 checks passed
7 checks passed
@theforeman-bot
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@theforeman-bot
foreman Build finished.
Details
hound No violations found. Woof!
@theforeman-bot
katello Build finished.
Details
@theforeman-bot
prprocessor Commit message style is correct
Details
@theforeman-bot
upgrade Build finished.
Details
@lzap
Copy link
Member

@lzap lzap commented Nov 29, 2017

Thanks.

@ezr-ondrej
Copy link
Member

@ezr-ondrej ezr-ondrej commented Jun 16, 2020

@fabianvf in #7749 (comment) has been raised an concern this actually doesn't work anymore. And original issue seems to be resolved by other means. Could you please share if this is still an interesting feature for you, or we can drop the setting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants