Add support for log rotate on Windows #607

evandervecht · 2018-04-16T11:07:07Z

I refactored the #581 PR. (that one can be closed).

Description

[Windows|Linux] Updated installation to 1.2.0 (havent tested with 1.3.x yet)
If needed I can make a PR just for installing a newer version first, then another PR to support logrotate. DEfaulting to 1.2.0 as 1.0.0 is out since mid 2017.
[Windows] Repo url same as sensu-puppet.
[Windows] In recipe created if statement to check whether you have a 32/64 bit OS.
[Windows] Updated client xml to support log rotating the in the sensu log folder.
["sensu"]["log_directory"] Log folder can be any folder as logn as the file name is sensu-client.out

Motivation and Context

Log file on Windows was not being rotated. This caused disk to fill up.
#580

How Has This Been Tested?

Tested this on Windows 2016 & CentOS 7, Chef client 12.19.36
Run Chef-client, see if it installs the newer version, check if the client.xml is being changed.
See if the log file is being rotated (tested with 10kb else I would have to wait for a much longer time)

Screenshots (if appropriate):

Please note that the rotate was set to 10kb but after running Chef it was changed to 10 MB, therefor the sensu-client.out is larger than the rotated files.

Types of changes

New feature
Newer version installation

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document, but I do not have Kitchen etc installed.

majormoses

I feel like we are almost there but there are still some things that need changes and explaining. Forgive my lack of understanding how things work on windows.

majormoses · 2018-04-18T06:43:53Z

attributes/default.rb

 default["sensu"]["version_suffix"] = nil
 default["sensu"]["apt_repo_codename"] = nil
 default["sensu"]["yum_repo_releasever"] = nil
 default["sensu"]["use_unstable_repo"] = false
 default["sensu"]["log_level"] = "info"
+default["sensu"]["log_rotate_file_size"] = '10240'


Based on your screenshot and the documentation example this should be an integer not a string but the puppet module has a string. I am leaning integer because thats what the documentation has but not sure matters 🤷‍♂️

@majormoses Thanks for the review. I changed it to int based on your comment/docs. It should not really matter for the function of it but this way it's consistent.

majormoses · 2018-04-18T06:44:23Z

attributes/default.rb

 default["sensu"]["version_suffix"] = nil
 default["sensu"]["apt_repo_codename"] = nil
 default["sensu"]["yum_repo_releasever"] = nil
 default["sensu"]["use_unstable_repo"] = false
 default["sensu"]["log_level"] = "info"
+default["sensu"]["log_rotate_file_size"] = '10240'
+default["sensu"]["log_rotate_file_keep"] = '10'


Based on your screenshot and the documentation example this should be an integer not a string but the puppet module has a string. I am leaning integer because thats what the documentation has but not sure matters 🤷‍♂️

majormoses · 2018-04-18T07:04:53Z

attributes/default.rb

@@ -34,7 +36,7 @@
 default["sensu"]["apt_repo_url"] = "http://repositories.sensuapp.org/apt"
 default["sensu"]["yum_repo_url"] = "http://repositories.sensuapp.org"
 default['sensu']['yum_flush_cache'] = nil
-default["sensu"]["msi_repo_url"] = "https://repositories.sensuapp.org/msi"
+default["sensu"]["msi_repo_url"] = "https://repositories.sensuapp.org/msi/2012r2"


why does this need to change? I am still unclear with the previous discussion wont this break all other versions?

@majormoses When you go to the old url, version go up to 0.26. Logrotate works as of 0.29.x. Puppet version also uses the new url, however still using the 0.29.x version. If uncertain of my comment please check the old url vs new url.

I see new versions in here: http://repositories.sensuapp.org/msi/2016/ which was my point the windows version plays into what is installed. I really don't know enough about windows stuff but this feel like this would break every version other than server 2012.

I think the problem is that we need to include the version not hardcore it to 2012r2

I don't have a windows machine to test with can you please give me the relevant ohai output for this: https://github.com/chef/ohai/blob/v14.1.0/lib/ohai/plugins/windows/platform.rb#L30 as I think we can use this to solve the problem.

@majormoses I can use the current Windows cookbook (1.36) using on platform_version.
I'd rather use "case ::Windows::VersionHelper.nt_version node", which is more clean than the current solution, however to use that I need to bump up the Windows coookbook version to > 4.0.0 which might cause some cookbook rewriting. To have the least impact on the dependencies I used the current platform_version.

I also removed the attribute from the attributes file and placed the logic in the recipe. Please let me know what you think of it. As with the logic it already checks which version is needed I do not think it's necessary to have it in the attributes file (it also excludes saving the attribute on Linux hosts).

majormoses · 2018-05-07T16:18:26Z

I reached out to some folks in the chef slack windows channel and it looks like node['kernel']['os_info']['name'] might be the way to go rather than the kernel version. I will review this later but just wanted to get that out to you.

majormoses · 2018-05-08T04:30:05Z

I rebased your fork to fix conflicts.

majormoses

Overall I think this makes sense one minor tweak and we should be good to go. Agree about later using the windows cookbook helper function for this.

majormoses · 2018-05-08T04:36:12Z

recipes/_windows.rb

+end
+
+if node['platform_version'].to_f == 6.1
+  node.default['sensu']['msi_repo_url'] = 'https://repositories.sensuapp.org/msi/2008r2'


I think we should specify the attribute as https://repositories.sensuapp.org/msi/ and then just have a local variable here to keep track of version/edition just like we do with kernel.

majormoses · 2018-05-08T04:37:10Z

recipes/_windows.rb

@@ -35,7 +51,7 @@
 end

 package "Sensu" do
-  source "#{node['sensu']['msi_repo_url']}/sensu-#{node['sensu']['version']}.msi"
+  source "#{node['sensu']['msi_repo_url']}/sensu-#{node['sensu']['version']}-#{kernel}.msi"


I think this should be "#{node['sensu']['msi_repo_url']}/#{version}/sensu-#{node['sensu']['version']}-#{kernel}.msi"

Can do, but it would mean that the OS version is always part of the url.

If we keep it like this, anybody can overwrite the #{node['sensu']['msi_repo_url']} attribute and point it to a different url also without the OS version bound in the url itself.
For example, I cannot overwrite the attribute anymore with www.google.com/download/ if the client is located at www.google.com/download/sensu-client-x64.msi. I then have to save the file in www.google.com/download//sensu-client-x64.msi, for each OS that I have 1 time.

A reason to save/cache the binary somewhere else can be, if the original url changes multiple times or the installed version is removed from the original url.

Fair point but that's true about the kernel version as well, they could rename the file because they feel they don't need the (x86|x|64) version. I feel like if they want to use a different (internal?) location they should mirror the repo structure and copy several versions over (for rollback) not just copy the one binary they want. By making it single attribute it is harder to override for just a particular instance. By just needing to give it the base path and version it should figure out the platform and pull from the right location. In my experience most shops do not have all servers with the exact same OS version especially in windows land as things tend to be more manual and therefore servers are often left on older versions of windows server while newer systems are put on newer versions.

True that.

We use an internal artifact system but can't remember the exact reason why :-). Currently, it's being used by ~20 customers of us.

I updated the recipe/attributes files. I already had it ready :-)

When I bump up the cookbook I will also need to update the Artifacts repo, but that's not a big issue.

…evelop

majormoses · 2018-05-08T16:24:41Z

released: https://supermarket.chef.io/cookbooks/sensu/versions/5.1.0

evandervecht added 2 commits April 16, 2018 11:30

support for windows

c82842b

.

0522692

majormoses mentioned this pull request Apr 18, 2018

Add support for log rotate on Windows & Version bump to 1.0.2 #581

Closed

4 tasks

majormoses suggested changes Apr 18, 2018

View reviewed changes

majormoses added Type: Enhancement Status: Awaiting Response Status: Needs SME Review labels Apr 18, 2018

majormoses requested a review from cwjohnston April 18, 2018 07:07

evandervecht added 2 commits April 18, 2018 09:51

string to int

edc8b63

each os version should point to their own repo url

5edd695

evandervecht added 3 commits May 7, 2018 21:27

support for windows

c480483

string to int

7a31c2e

each os version should point to their own repo url

1756c52

majormoses force-pushed the develop branch from 5edd695 to 1756c52 Compare May 8, 2018 04:29

majormoses approved these changes May 8, 2018

View reviewed changes

evandervecht added 2 commits May 8, 2018 13:27

Merge branch 'develop' of github.com:ridiculousness/sensu-chef into d…

8ee4564

…evelop

default msi url back to attributes files, only distinguish in os version

4192ead

majormoses merged commit 703983e into sensu:develop May 8, 2018

majormoses added a commit that referenced this pull request May 8, 2018

adding changelog for #607

c6fde66

majormoses added a commit that referenced this pull request May 8, 2018

adding new attributes from #607 to readme

91148a6

majormoses added Type: Bug and removed Status: Awaiting Response Status: Needs SME Review labels May 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for log rotate on Windows #607

Add support for log rotate on Windows #607

evandervecht commented Apr 16, 2018 •

edited

Loading

majormoses left a comment

majormoses Apr 18, 2018

evandervecht Apr 18, 2018

majormoses Apr 18, 2018

majormoses Apr 18, 2018

evandervecht Apr 18, 2018

majormoses May 4, 2018

majormoses May 4, 2018

evandervecht May 7, 2018 •

edited

Loading

majormoses commented May 7, 2018 •

edited

Loading

majormoses commented May 8, 2018

majormoses left a comment

majormoses May 8, 2018

majormoses May 8, 2018

evandervecht May 8, 2018

majormoses May 8, 2018

evandervecht May 8, 2018 •

edited

Loading

majormoses commented May 8, 2018

Add support for log rotate on Windows #607

Add support for log rotate on Windows #607

Conversation

evandervecht commented Apr 16, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

majormoses left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evandervecht May 7, 2018 • edited Loading

Choose a reason for hiding this comment

majormoses commented May 7, 2018 • edited Loading

majormoses commented May 8, 2018

majormoses left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evandervecht May 8, 2018 • edited Loading

Choose a reason for hiding this comment

majormoses commented May 8, 2018

evandervecht commented Apr 16, 2018 •

edited

Loading

evandervecht May 7, 2018 •

edited

Loading

majormoses commented May 7, 2018 •

edited

Loading

evandervecht May 8, 2018 •

edited

Loading