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

feature/go-pfmon #5613

Merged
merged 126 commits into from
Sep 23, 2020
Merged

feature/go-pfmon #5613

merged 126 commits into from
Sep 23, 2020

Conversation

jrouzierinverse
Copy link
Member

@jrouzierinverse jrouzierinverse commented Jun 18, 2020

Description

A go based pfmon

NEWS file entries

New Features

  • go based pfmon

Delete branch after merge

NO

Checklist

  • Document the feature
  • Add unit tests
  • Add acceptance tests (TestLink)

Upgrade Instructions

Rename PFMON -> PFCRON

/usr/local/pf/bin/addons/upgrade/to-10.2-adminroles-conf.pl

@julsemaan
Copy link
Collaborator

One general comment I have about this is that the new binary shouldn't be called pfmaint since it can easily be confused with pf-maint.pl

We should keep the name pfmon

@fdurand
Copy link
Member

fdurand commented Jul 17, 2020

Does the new pfmon binary is aware of the tasks it can run on all cluster members versus some of them who needs to run on only one server ?

@jrouzierinverse
Copy link
Member Author

Does the new pfmon binary is aware of the tasks it can run on all cluster members versus some of them who needs to run on only one server ?

No, I will add support for running on only the master server since that is the current behavior of pfmon.

@jrouzierinverse
Copy link
Member Author

It now only runs on the master in the cluster.

@nqb
Copy link
Contributor

nqb commented Aug 20, 2020

After this one is merged, will pfmon continues to exist or should we need to replace pfmon by pfcron in all places (for instance in documentation) ?

addons/dev-helpers/pfmon_to_maintenance.pl Outdated Show resolved Hide resolved
go/cron/window_batch_task.go Show resolved Hide resolved
go/cron/pfcronjob.go Outdated Show resolved Hide resolved
go/cron/config.go Outdated Show resolved Hide resolved
go/cron/cluster.go Outdated Show resolved Hide resolved
go/cron/cluster.go Outdated Show resolved Hide resolved
go/cron/cluster.go Outdated Show resolved Hide resolved
go/cron/batch.go Show resolved Hide resolved
@@ -67,71 +67,8 @@ pf::cmd::pf::pfmon

use strict;
use warnings;
use pf::config::pfmon qw(%ConfigPfmon);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just remove the file ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it for backward compatibility of cron jobs.

lib/pf/file_paths.pm Outdated Show resolved Hide resolved
@jrouzierinverse jrouzierinverse force-pushed the feature/go-pfmon branch 3 times, most recently from 30c4e89 to 33f61c6 Compare September 18, 2020 15:17
@jrouzierinverse
Copy link
Member Author

Please rereview

import (
"testing"
)

Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to test an expired certificate ?

@fdurand
Copy link
Member

fdurand commented Sep 21, 2020

There is still references to pfmon in the code.

addons/backup-and-maintenance.sh
addons/monit/monit_checks_configurations/00_packetfence.tt
conf/log.conf.d/pfmon.conf.example
conf/monitoring/statsd.d/packetfence.conf.example
docs/PacketFence_Network_Devices_Configuration_Guide.asciidoc
docs/PacketFence_Clustering_Guide.asciidoc
docs/api/spec/openapi.yaml
lib/pf/constants/admin_roles.pm
....

What do we do with that ?

@extrafu
Copy link
Member

extrafu commented Sep 22, 2020

@jrouzierinverse Advised on Fabrice's last comment and let's wrap this up once for all.

@jrouzierinverse
Copy link
Member Author

Please re-review

addons/upgrade/to-10.2-adminroles-conf.pl Outdated Show resolved Hide resolved
addons/upgrade/to-10.2-adminroles-conf.pl Outdated Show resolved Hide resolved
@fdurand
Copy link
Member

fdurand commented Sep 22, 2020

On my side it looks ok, i let keep @julsemaan doing a last shot.

@julsemaan
Copy link
Collaborator

My comments were marked as resolved so I'm fine with this being merged

@fdurand fdurand merged commit 55baa6a into devel Sep 23, 2020
fdurand added a commit that referenced this pull request Sep 23, 2020
satkunas pushed a commit that referenced this pull request Dec 11, 2020
@satkunas satkunas deleted the feature/go-pfmon branch May 15, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants