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

Make the onbootsec parameter to attribute #707

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vineethp088
Copy link

@vineethp088 vineethp088 commented Aug 30, 2020

This is to make the OnBootSec parameter to control via attribute when the chef-client runs as a timer service,

Description

This is to make the OnBootSec parameter to control via attribute when the chef-client runs as a timer service.
There are cases chef-client run after the boot is elapsed, becuase of the default 1min and there should be a mechanism to control this parameter.
Changing this parameter to hgher value seems to be fixing this and should be control it via attribute, keeping 1 min as default.

Issues Resolved

There are cases chef-client run after the boot is elapsed, becuase of the default 1min and there should be a mechanism to control this parameter.
Changing this parameter to higher value seems to be fixing this and should be control it via attribute, keeping 1 min as default.

Check List

@@ -27,7 +27,7 @@

property :user, String, default: 'root'

property :delay_after_boot, String, default: '1min'
Copy link
Contributor

Choose a reason for hiding this comment

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

This resource is actually a backport from Chef Infra Client 16 so we need to keep the default value the same as it is in client at 1min.

Copy link
Author

Choose a reason for hiding this comment

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

We wanted to make the OnBootsec parameter as attribute, so that it can be controlled. We are seeing a problem with the chef client timer its getting elapased ( when runinng the chef client as a timer). We do not see that problem all the time/servers but we could reproduce the elapsed issue after reducing/increasing the onbootsec parameter. So if it can be controlled by node attribute it will help us fixing this issue. We can keep the default as ( 1 min/60sec) , May be I didnt get the point here about the chef16 dependecny.

Choose a reason for hiding this comment

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

This cookbook has a systemd resource copied from Chef Infra 16, so that users of the cookbook can use older versions of Chef Infra. In order to ease transitioning to use the resource in Chef Infra 16, this needs to be defined the same as the one there: https://github.com/chef/chef/blob/master/lib/chef/resource/chef_client_systemd_timer.rb#L65-L67

The other changes in this PR are fine, but changing the default from '1min' to '60sec' needs to be reverted.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, Reverted let me know if this is fine. We just need that attribute to be controlled.

Copy link
Author

Choose a reason for hiding this comment

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

Please let me know if this can be merged or need any additional modification ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dheerajd-msys
Copy link
Contributor

Issue #627 probably be fixed by this PR

@phiggins
Copy link

phiggins commented Sep 17, 2020

I think this change is a good idea, but I don't think this is the right way to do it and this is complicated by what looks like a bug from confusing behavior.

  • The properties delay_after_boot, interval, and splay are eventually used as systemd time spans.
  • delay_after_boot and interval accept arbitrary strings, so they can be a number of seconds eg '60' or it can include time units eg 5h30m.
  • splay is coerced to an integer so it is assumed to be a number of seconds.

Appending sec onto the value specified for delay_after_boot in recipes/systemd_service.rb could end up with incorrect values like '5h30msec. That also means the current behavior of interpolating interval is incorrect. Since bare values are interpreted as seconds, the code in the recipe should probably use the values directly:

      'OnBootSec' => node['chef_client']['delay_after_boot'],
      'OnUnitInactiveSec' => node['chef_client']['interval'],
      'RandomizedDelaySec' => node['chef_client']['splay'],

@dheerajd-msys
Copy link
Contributor

@vineethp088 Could you please make changes per suggestion here ? then I guess we would be good to merge the PR.

Thanks

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

Successfully merging this pull request may close these issues.

5 participants