Support Windows #33

mxzinke · 2021-08-23T12:12:23Z

Supporting Windows:

genebean · 2021-08-23T15:14:20Z

So, the notify that is in here already should actually be correct. I think the issue my lie in the way service.pp or config.pp is applied on Raspbian. Can you post the output of facter os and also share what version of puppet you are using?

mxzinke · 2021-08-24T19:29:39Z

With systemd::unit_file you don't need a notify anymore.

Facts:

{
  architecture => "armv7l",
  distro => {
    codename => "buster",
    description => "Raspbian GNU/Linux 10 (buster)",
    id => "Raspbian",
    release => {
      full => "10.10",
      major => "10",
      minor => "10"
    }
  },
  family => "Debian",
  hardware => "armv7l",
  name => "Raspbian",
  release => {
    full => "10.10",
    major => "10",
    minor => "10"
  },
  selinux => {
    enabled => false
  }
}

genebean · 2021-08-24T19:48:28Z

@mxzinke What version of Puppet?

bastelfreak · 2021-08-30T19:16:44Z

manifests/service.pp

-        enable  => $promtail::service_enable,
-        require => Systemd::Unit_file['promtail.service'],
+        enable => $promtail::service_enable,
+        active => $promtail::service_ensure == 'running'


I love the syntax <3

I am not familiar with this syntax @bastelfreak, what does it do and why do you <3 it?

Maybe because it's readable?

bastelfreak · 2021-08-30T19:17:31Z

manifests/service.pp

 class promtail::service {
  case $facts['kernel'] {
    'Linux': {
+      include systemd::systemctl::daemon_reload


this resource is gone in latest releases of the systemd module. The module now requires puppet 6.1 or newer because that version contains the automatic systemctl daemon-reload und changed/new unit files.

This module already states 6.1 as the bottom end so I think removing this would make sense.

genebean · 2021-08-30T19:23:10Z

@mxzinke I need a couple of things:

please fix the syntax error shown in the tests. You can easily check for others by running pdk validate and pdk test unit locally if you want.
please remove the daemon_reload line as noted above.

Also, still wondering what version of puppet you are running.

bastelfreak · 2021-08-30T19:25:35Z

@mxzinke which version of the systemd module do you have in your environment and which puppet version are you on? And you use systemd, right (the code really looke like it but the error message sounds so much like sysv)?

as mentioned in the inline comment, camptocamp/systemd dropped the exec resource which does a systemctl daemon-reload with the 3.0.0 release: voxpupuli/puppet-systemd#171

to make this a bit more confusing, after the release the module got migrated to Vox Pupuli. To work properly you need the latest camptocamp module and puppet 6.1 or newer. I assume you've a combination of an older systemd module and/or puppet. I'm not aware of any issues with the latest version + puppet 6.1 so the code changes shouldn't be required, but I also think it's a good cleanup and the service should be managed through systemd::unit_file (as long as the promtail isn't installed from a package).

mxzinke · 2021-08-30T19:50:56Z

@mxzinke What version of Puppet?

Puppet Version 6.23

bastelfreak · 2021-09-16T11:58:12Z

manifests/init.pp

  Hash                           $positions_config_hash,
  Hash                           $scrape_configs_hash,
-  Stdlib::Absolutepath           $bin_dir,
+  Stdlib::Absolutepath           $bin_dir               = promtail::params::bin_dir,


my personal opinion is that instead of adding a params.pp, hiera in modules should be used (but I'm not a maintainer here).

Agreed, please move this to the existing in module data. I will comment below with some pointers in case this is not something you have done before (no disrespect intended if you have, just trying to help)

Would be nice. If you could do it. Also would be nice if you could have a look at the windows service thing, it seems to not work correctly.

genebean

I absolutely love that you are adding Windows support, but that work also needs to be its own PR, not added onto this one.

Regarding this PR, @bastelfreak brought up in #33 (comment) that your issue might well be related to the version of the systemd module you are using. Does that line up at all with what you have locally?

genebean · 2021-09-16T12:45:36Z

manifests/init.pp

  Hash                           $positions_config_hash,
  Hash                           $scrape_configs_hash,
-  Stdlib::Absolutepath           $bin_dir,
+  Stdlib::Absolutepath           $bin_dir               = promtail::params::bin_dir,


Agreed, please move this to the existing in module data. I will comment below with some pointers in case this is not something you have done before (no disrespect intended if you have, just trying to help)

genebean · 2021-09-16T12:46:44Z

manifests/params.pp

+    'Linux': {
+      $bin_dir = '/usr/local/bin'
+      $data_dir = '/usr/local/promtail_data'
+      $config_dir = '/etc/promtail'


These items belong in data/kernel/Linux.yaml

genebean · 2021-09-16T12:47:15Z

manifests/params.pp

+      $bin_dir = 'C:\\Program Files\\promtail'
+      $data_dir = "${bin_dir}\\versions"
+      $config_dir = "${bin_dir}\\config"


These items belong in data/kernel/windows.yaml

mxzinke · 2021-09-16T16:14:39Z

I absolutely love that you are adding Windows support, but that work also needs to be its own PR, not added onto this one.

Sorry, I actually worked with my fork and forgot to change the branch. The original issue is actually gone. So may we just change the title and work on the Windows Support. It's actually needed soon.

Regarding this PR, @bastelfreak brought up in #33 (comment) that your issue might well be related to the version of the systemd module you are using. Does that line up at all with what you have locally?

The solution for my issue was to set the correct default provider.

mxzinke · 2021-09-16T16:19:13Z

Just to tell one more time and to not confuse:

The original problem resolved by setting a correct default service provider. (to systemd)

I changed the title of the PR to now supporting windows. Sorry for switching topics, but I am really in a hurry and sometimes you mess things up

This reverts commit 7cd029a.

This reverts commit c20d9f6.

CLAassistant · 2022-06-15T17:46:37Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Maximilian Zinke seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

bastelfreak · 2022-06-15T18:06:55Z

manifests/init.pp

 # @param [Stdlib::Absolutepath] bin_dir
 #   The directory in which to create a symlink to the promtail binary
 #
+# @param [Stdlib::Absolutepath] config_dir


since newer puppet-strings releases, you don't have to provide the datatype here, puppet-strings will read it from the actual code

bastelfreak · 2022-06-15T18:07:39Z

manifests/params.pp

+#
+# Sets default variables per OS
+#
+# @api private


if this class is private I suggest adding assert_private() to the code

mxzinke · 2022-08-24T13:50:05Z

@bastelfreak In case this topic is still in your interest, please go on. I've no time for managing bringing the change into the project. Sorry.

Maximilian Zinke added 5 commits August 23, 2021 12:23

fix: use systemd::unit_file for service

628a3dc

fix: wait until restarting service

7c3684e

fix: notify service-install when files changed

8144493

fix: restart correct service when updating config

d9353d4

fix: add Start-Limit Burst for Promtail

c2d1e06

bastelfreak reviewed Aug 30, 2021

View reviewed changes

feat: implement support for windows

ac83b6f

bastelfreak reviewed Sep 16, 2021

View reviewed changes

genebean requested changes Sep 16, 2021

View reviewed changes

Maximilian Zinke added 4 commits September 16, 2021 15:03

fix: convert to string type

ebc2114

fix: add supported architecture 'x64'

ec25d16

fix: actually use absolute-path type

a908547

fix: install windows service correctly before using service class

7b06435

mxzinke changed the title ~~Fix Service on Raspbian OS~~ Support Windows Sep 16, 2021

Maximilian Zinke added 7 commits September 17, 2021 22:15

fix: correctly register service

810743a

fix: remove New-Service definition in promtail::service class

c20d9f6

fix: remove unused require of the installed service

7cd029a

Revert "fix: remove unused require of the installed service"

02f5cb8

This reverts commit 7cd029a.

Revert "fix: remove New-Service definition in promtail::service class"

3eaf2a6

This reverts commit c20d9f6.

fix(service): install windows service with sc.exe

bc0a286

fix: install and manage service with sc.exe only

1161f52

Maximilian Zinke added 12 commits September 20, 2021 10:17

fix: hopefully install service correctly

633a89b

fix: actual install promtail windows service

b977bad

fix: install windows service as automatic startup

35f1024

fix: path of service for installation

5b73154

fix: mistyping in hiera.yaml

cf50843

feat: add support of windows to metadata

1ca3a88

fix: use nssm package to support windows service

114ac66

fix: add correct ensure state to nssm::service definition

f890bce

fix: require nssm package

79bfb86

fix: how config path is given to nssm::service definition

924d350

fix: update required nssm package

e647edb

fix: app parameter to enquote the config path

e5158a7

bastelfreak reviewed Jun 15, 2022

View reviewed changes

Support Windows #33

Are you sure you want to change the base?

Support Windows #33

Uh oh!

Conversation

mxzinke commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

genebean commented Aug 23, 2021

Uh oh!

mxzinke commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

genebean commented Aug 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genebean commented Aug 30, 2021

Uh oh!

bastelfreak commented Aug 30, 2021

Uh oh!

mxzinke commented Aug 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genebean left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxzinke commented Sep 16, 2021

Uh oh!

mxzinke commented Sep 16, 2021

Uh oh!

CLAassistant commented Jun 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxzinke commented Aug 24, 2022

Uh oh!

Uh oh!

mxzinke commented Aug 23, 2021 •

edited

Loading

mxzinke commented Aug 24, 2021 •

edited

Loading