-
Notifications
You must be signed in to change notification settings - Fork 279
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
Add support for log rotate on Windows & Version bump to 1.0.2 #581
Conversation
remove verison bump
The log changes look reasonable to me. If we're updating versions I'd suggest going to the latest release, 1.1.1. @cwjohnston @amdprophet @majormoses can you take a look too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! I think it would be best to minimize the changes to just the requirements to use logrotate. We can do a separate release to update the default versions.
default["sensu"]["use_ssl"] = true | ||
default["sensu"]["use_embedded_ruby"] = true | ||
default["sensu"]["service_max_wait"] = 10 | ||
default["sensu"]["directory_mode"] = "0750" | ||
default["sensu"]["log_directory_mode"] = "0750" | ||
default["sensu"]["client_deregister_on_stop"] = false | ||
default["sensu"]["client_deregister_handler"] = nil | ||
|
||
default["sensu"]["msi_repo_url"] = "http://repositories.sensuapp.org/msi/2016" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change and needs to be called out in the changelog. Again I would prefer we limit the changes strictly to what is required to use logrotate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The url wihtout the 2016 has no MSI above 0.26.5 :( so I had to change it. I expect but have not tested it that the msi's in the 2016 folder are either backwards compatible or the same as in the 2012R2 folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the repository server the 2016 and 2012r2 directories are symlinks to the 2012 directory. There's no difference in the content being served.
else | ||
default["sensu"]["admin_user"] = "root" | ||
default["sensu"]["directory"] = "/etc/sensu" | ||
default["sensu"]["log_directory"] = "/var/log/sensu" | ||
default["sensu"]["version"] = "1.0.2-1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required to use the new logrotate? I'd prefer to keep the changes as isolated as possible. I am not sure that we should default to 1.x yet as there are a handful of community plugins that have conflicts and need to be worked out first. You can always overwrite the version in your wrapper cookbook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linux installation version does not come with a 32/64 bit version.
Previous it was all the same for both Windows and Linux.
@@ -10,30 +10,33 @@ | |||
default["sensu"]["windows"]["package_options"] = nil | |||
default["sensu"]["windows"]["install_dotnet"] = true | |||
default["sensu"]["windows"]["dotnet_major_version"] = 4 | |||
default["sensu"]["version"] = "1.0.2-1-x64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required to use the new logrotate? I'd prefer to keep the changes as isolated as possible. I am not sure that we should default to 1.x yet as there are a handful of community plugins that have conflicts and need to be worked out first. You can always overwrite the version in your wrapper cookbook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logrotate is supported as of 0.29.5 (#580). I checked the repo url and it has been changed. If I look at http://repositories.sensuapp.org/msi/. I see 1 type of msi for both 32/64 bit, now there are 2 msi's (http://repositories.sensuapp.org/msi/2016/). I had to test this with 1.0.2 as we have had issues with pre 1.x versio and already installed 1.0.2, which was latest at that moment of time. Reverting back to 0.28.4-1 would mean that logrotate won't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see 1 type of msi for both 32/64 bit
I believe these are 32bit builds which work on 64bit systems. We now build both 32 and 64 bit installers.
@@ -10,30 +10,33 @@ | |||
default["sensu"]["windows"]["package_options"] = nil | |||
default["sensu"]["windows"]["install_dotnet"] = true | |||
default["sensu"]["windows"]["dotnet_major_version"] = 4 | |||
default["sensu"]["version"] = "1.0.2-1-x64" | |||
default['sensu']['windows_package_version'] = '1.0.2.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required to use the new logrotate? I'd prefer to keep the changes as isolated as possible. I am not sure that we should default to 1.x yet as there are a handful of community plugins that have conflicts and need to be worked out first. You can always overwrite the version in your wrapper cookbook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is tricky.
Installing "1.0.2-1-x64" results in '1.0.2.1' in the registry.
As it does not has the 1.0.2-1-x64 display version it installs the client every chef run. This should imo be fixed in the MSI itself to keep the MSI version the same as the display version in the Windows registry. If that happens the attribute is absolete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @cwjohnston whos the best person to discuss the msi fix for version numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This -x64
suffix may be an artifact introduced by the toolchain changes starting in Sensu 0.27. Opening an issue against https://github.com/sensu/sensu-omnibus would help us track and fix this.
@majormoses Awaiting your comments based on my comments. If I need to change the version to 0.29.5 (the minimum release to support logrotate) I will do so. |
uggh this is messy. I am ok with these changes mostly. I'd like to hear from @cwjohnston first on some of the comments before merging and releasing. |
ping @cwjohnston |
Sorry for my tardiness in replying here. I've added to individual code comments above. Unfortunately our lack of familiarity with building Windows installers has lead to a confusing situation around Windows MSIs. I think the way to address this going forward is to figure out how we can use the installer to detect the available version of .NET instead of building different installers for each combination of CPU architecture and .NET Framework version. We're tracking this need in sensu/sensu-omnibus#234 |
closing per #607 (comment) |
Description
Windows requires package_version because of the x86/x64 msi's.
Note also that in the url there is a - character and in the package this - is a .
Windows:
default["sensu"]["msi_repo_url"] = "http://repositories.sensuapp.org/msi/2016"
default["sensu"]["version"] = "1.0.2-1-x64"
default['sensu']['windows_package_version'] = '1.0.2.1'
Linux:
default["sensu"]["version"] = "1.0.2-1"
["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
Checklist: