LINUX.ORG.RU

Покритикуйте плагин

 , ,


0

1

Коллеги, покритикуйте плиз плагин ( Он для сбора инфы с LXC контейнеров )

#!/usr/bin/perl
use strict;
use warnings;
use utf8;

#=============================================
# Icinga plugin For monitoring LXC
# Copyright 2018 Naim Shafiev
# Released under the GPLv3 license
# v0.95
#=============================================

use Getopt::Long qw(GetOptions);
Getopt::Long::Configure qw(gnu_getopt);
use FindBin qw($Bin);
use Data::Dumper;
use Sys::Syslog qw(:DEFAULT setlogsock);
use Fcntl qw(:DEFAULT :flock);


my $linux_host_cpu_usage = '/sys/fs/cgroup/cpu/cpuacct.usage';
my $linux_host_mem_usage = '/proc/meminfo';
my $linux_host_blkio_usage = '/sys/fs/cgroup/blkio/blkio.throttle.io_serviced';

my $warning;
my $critical;
my $perf;
my $type;
my $sleep_time;
my $out_to_plugin;

GetOptions(
    'warning|w=i' => \$warning,
    'critical|c=i' => \$critical,
    'type|t=s' => \$type,
    'perf|p' => \$perf,
    'sleep_time|s=i' => \$sleep_time,
) or die( "usage: $0 --perf -w warning -c critical -type (memory , io, cpu , network) " );


### lock
my ( $short_name ) = $0 =~ m|([^/]+)\.pl$|;
setlogsock('unix');
openlog( $short_name, 'pid', 'local0');
my $lockfile = "/tmp/".$short_name."_.lock";
open(my $LOCK, ">",$lockfile ) or die( "can't open lock file: $!" );
unless ( flock($LOCK, LOCK_EX|LOCK_NB)  ) { die( "$0 is already running - Aborting") } 
### /lock

unless ( $perf ){ $perf = 0 }
#print "$warning $critical $perf $url\n";

my @cmd_path = ( '/usr/bin', '/usr/local/bin', '.' );


my $lxc_stat;

my $cons_sum_memory;
my $cons_sum_cpu;
my $num_of_cpu;

my @out = `/usr/bin/sudo /usr/bin/lxc-ls --running -F NAME -f  `;
chomp(@out);
shift @out; # Remove COLUMN Name

for (@out) { s/ //g } 

$out_to_plugin =  "OK VM | "; 

open(my $status_file , "<", '/proc/cpuinfo');
while (<$status_file>) {
    if (m/processor/) {
        $num_of_cpu++;
    }
}

open($status_file , "<", $linux_host_cpu_usage );
while (<$status_file>) {
        chomp;
        $lxc_stat->{'main_HOST'}->{cputicks} = $_ / $num_of_cpu ;
        $out_to_plugin .= "main_HOST_whole_cputicks=" . $_ / $num_of_cpu .'c ';
    }


open($status_file , "<", $linux_host_mem_usage );
while (<$status_file>) {
    chomp;
    if ( m/MemTotal:\s+(\d+)/){
        $lxc_stat->{'main_HOST'}->{memory} = $1*1024;        
    }

}

open($status_file , "<", $linux_host_blkio_usage );
while (<$status_file>) {
       chomp;
       if ($_ =~ /Total (\d+)/ and $_!~/:/) {
           s/Total //; 
           $lxc_stat->{'main_HOST'}->{blkio} = $_;
           $out_to_plugin .= "main_HOST_whole_blkio=$_".'c ';
       }
}
#get info from each running container
foreach my $name (@out) {
        my @lxc_out = `/usr/bin/sudo  /usr/bin/lxc-info -S -H -n $name `;
        chomp @lxc_out;
        #my $link_name_br;
        for (@lxc_out) {
            if ($_ =~ /Link:/) {
                my $link_name = (split /:/)[1];
                $link_name =~ s/ //g;
                my @ip_show_out = `/sbin/ip link show`;

                for (@ip_show_out) {
                    if (m /.+$link_name.+(br-\S+)/) {
                        $lxc_stat->{$name}->{'link_name'} = $1;
                    }
                }                         
            }
            
            if ($_ =~ /TX bytes:/) {
                my ($cut,$tx) = split /:/;
                $tx =~ s/ //g;
                $lxc_stat->{$name}->{tx} = $tx;
                $out_to_plugin .= $name.'_'.$lxc_stat->{$name}->{'link_name'}."_tx=$tx".'c '; 
            }
            
            if ($_ =~ /RX bytes:/) {
                my ($cut,$rx) = split /:/;
                $rx =~ s/ //g;
                $lxc_stat->{$name}->{'rx'} = $rx;
                $out_to_plugin .= $name.'_'.$lxc_stat->{$name}->{'link_name'}."_rx=$rx".'c '; 

            }
            
            if ( $_ =~ /CPU.+?(\d+)/) {
                $lxc_stat->{$name}->{cputicks} = $1 / $num_of_cpu;
                $out_to_plugin .= $name."_cputicks=".$1 / $num_of_cpu .'c ';
                $cons_sum_cpu += $1; 
            }

            if ( $_ =~ /Memory.+?(\d+)/) {
                $lxc_stat->{$name}->{memory} = $1;
                $out_to_plugin .= $name."_memory=$1".'b ';
                $cons_sum_memory += $1;
            }
            
        }
        

        open($status_file , "<", "/sys/fs/cgroup/blkio/lxc/$name/blkio.throttle.io_serviced") or die "$@ cannot open CGROUP file for IOP/S";
        while (<$status_file>) {
            chomp;
       
            if ($_ =~ /Total/ and $_!~/:/) {
                s/Total //;
                $lxc_stat->{$name}->{blkio} = $_;
                $out_to_plugin .= $name."_blkio=$_".'c ';
            }
        }    
    
}
close ($status_file);    

$out_to_plugin .= "main_HOST_memory_left=". ( $lxc_stat->{'main_HOST'}->{memory} - $cons_sum_memory ) .'b ';
#$out_to_plugin .= "main_HOST_cpuTotal_available_time_per_second=". (  $num_of_cpu * 1000000000  ).' ';

print "$out_to_plugin \n" ;


exit 0;

★★★★★

Последнее исправление: beastie (всего исправлений: 1)

Ответ на: комментарий от hotpil

Icinga 2 он собирует инфа по LXC контейнерам . Я все в самом заголовек указал

pinachet ★★★★★
() автор топика

Что именно критиковать? Архитектуру/code style/следование PBP/модель предметной области?

Вот я предметную область не знаю, но вижу что exit вызывается с параметром 0. Ноль это дефолтное значение для exit, следовательно можно его не указывать в данном случае. Сам exit вызывается без скобок, видимо автор знает что так можно и как это работает. Еще ноль это магическая числовая константа, будет-ли ругаться Perl::Critic?

Чуть выше close закрывает файловый дескриптор в переменной $status_file. Почему-то без обработки ошибок, но со скобками (которые не обязательны) - отсутствие стилистической консистентности.

И т.д и т.п., могу долго расписывать в чем тут беда.

outtaspace ★★★
()

строки типа

if ( $_ =~ /Memory.+?(\d+)/) {

заменить на

if (/Memory.+?(\d+)/) {

if -> elsif там где требуется

anonymous
()
Ответ на: комментарий от pinachet

Проблем в этом скрипте много, я бы начал с длинных циклов с неявной переменной итерирования - плохой стиль.

close $fh ставить там где это уместно. Создали $fh, прочитали из $fh, закрыли $fh. Как вариант - не использовать явный close, пусть сам закрывается.

outtaspace ★★★
()
Ответ на: комментарий от anonymous

да это тоже делаю, это есть в perlcritic

pinachet ★★★★★
() автор топика
Ответ на: комментарий от outtaspace

Проблем в этом скрипте много, я бы начал с длинных циклов с неявной переменной итерирования - плохой стиль.

Ты про $_ ? Это для скорости

pinachet ★★★★★
() автор топика
Ответ на: комментарий от outtaspace

Т.е. по-твоему лучше писать:

exit EXIT_CODE_SUCCESS

Только потому что 0 - магическая константа? А число пи - магическая константа? А почему бит может принимать значения 1 или 0, это же тоже какие-то подозрительные константы, надо бы их в use constant вписать: BIT_IS_SET => 1, BIT_IS_UNSET => 0. Ну зачем маразмом-то уж совсем заниматься, дело что ли больше нечего?

DRVTiny ★★★★★
()
Ответ на: комментарий от outtaspace

Кому-то $_ - высосанное из собственного пальца словосочентание, а кому-то контекст. Например, огромное количество машинных инструкций работают с rax по умолчанию - и что, это «плохой стиль»?

Контекст плох ровно тем, что о нём можно забыть, если код, его использующий, великоват. А ещё его можно ненароком переустановить, что гораздо хуже, поскольку контекст устанавливают/используют в perl многие команды - и легко попасть с этим впросак.

DRVTiny ★★★★★
()
Ответ на: комментарий от DRVTiny

Ну зачем маразмом-то уж совсем заниматься, дело что ли больше нечего?

Если есть установка использовать брутальный режим перлкритика, то вполне уместно в этом контексте городить EXIT_CODE_SUCCESS. Насколько брутальность является маразмом - отдельная тема. Я бы вообще без нуля вызывал exit, о чем написал в коменте.

outtaspace ★★★
()
Ответ на: комментарий от outtaspace

Помнится, уж не в книжке ли с верблюдом было про то, что даже в open канонически можно не обрабатывать ошибки. А уж закрытие файлового дескриптора... О, да, непременно нужно звать die, если файловый дескриптор не закрылся. А то чего-то он, в самом деле, не закрывается, зараза?! А плакал... Тупой религиозный фанатизм в среде разработчиков - явление неистребимое.

Кстати, если переменная $fh имеет локальный scope, то она будет уничтожена и файл будет закрыт по выходе из scope'а, а если нет - то при завершении программы.

DRVTiny ★★★★★
()
Последнее исправление: DRVTiny (всего исправлений: 1)
Ответ на: комментарий от outtaspace

Ну так без нуля - это значит «с дефолтным значением». А вдруг дефолт - это EXIT_CODE_42_BECAUSE_IT_IS_FUNNY? Надо бы контролировать ситуацию, как бы чего не вышло!

DRVTiny ★★★★★
()
Ответ на: комментарий от anonymous

Как я написал в телеге, я бы использовал when. И плевать на perl 5.27, который в дистрибутивах будет чуть раньше, чем никогда. when - отличная штука, и given тоже. Возможно, когда-то они делают что-то не так, но я с таким не сталкивался.

DRVTiny ★★★★★
()
Ответ на: комментарий от DRVTiny

Контекст плох ровно тем, что о нём можно забыть, если код, его использующий, великоват. А ещё его можно ненароком переустановить, что гораздо хуже,

Ты все правильно написал. Вот и я стараюсь уменьшить скоуп в котором есть неявная итерация или вводить явную (именованную) итерацию. Мутабилити - зло.

Мой поинт вот в чем - сделать контекст более очевидным, избежать потенциальных проблем. Но автор утверждает что это для перформанса, т.е. его осознанный выбор (он сделал замеры, понял насколько интенсивные циклы) и у меня нет больше вопросов по его аргументации.

outtaspace ★★★
()
Ответ на: комментарий от outtaspace

Согласен, что у автора $_ используется слишком глобально и совершенно не к месту.

DRVTiny ★★★★★
()

Вставьте, пожалуйста в открывающий тег code LOR-разметки уточнение для подcветки синтаксиса языка: code=perl

DRVTiny ★★★★★
()
Ответ на: комментарий от DRVTiny

О, да, непременно нужно звать die, если файловый дескриптор не закрылся.

Я таким не занимаюсь. У меня есть autodie, который все эти заботы от меня скрывает, не люблю писать лишний код.

будет уничтожена и файл будет закрыт по выходе из scope'а, а если нет - то при завершении программы

Я это прекрасно знаю, последние 13 лет или около того. Мой поинт в том, что надо быть консистентным - закрывать после того как меняется семантика использования хендлера (сделали новый open) или совсем не использовать close т.к. это забота интерпретатора. В коде ТСа как раз смешиваются подходы, поэтому и обратил на это его внимание.

outtaspace ★★★
()
Последнее исправление: outtaspace (всего исправлений: 1)
Ответ на: комментарий от outtaspace

В коде ТС можно было просто разные файловые дескрипторы открывать, от этого бы точно не случилось перерасхода памяти или исчерпания лимита на количество дескрипторов.

DRVTiny ★★★★★
()
  • #!/usr/bin/env perl
  • Закрывай файловые дескрипторы (close)
  • Выдели парсинг отдельных (форматов) файлов в отдельные функции, тогда интерфейс станет проще
  • Запуск команд через backticks — сильно подвержен иньекциям. Посмотри в сторону IPC::Open3 или аналогов (IPC::run?)
  • Лучше не использовать sudo в скрипте, а требовать чтоб скрипт запускали с нужным EUID ($>). Иначе предоставлять конфиг для sudo вместе со скриптом.
  • Предпочитай не читать все сразу, а по линии:
    my $stdout = ...; # open3?
    while (my $line = <$stdout>) {
      chomp $line; # если надо
      ...
    }
    
  • Напиши людский help:
    my $help = <<"__HELP__";
    Usage:
      $0 [options]
    
    Options:
      -h, --help          Show this help page
      -w, --warning=NUM  ... описание ...
      ...
    __HELP__
    
    my %options = (
      # default values (if needed)
      help    => 0,
      warning => 0,
      ...
    );
    GetOptions(\%options,
      'help|'
      'warning|w=i',
      'critical|c=i',
      ...
    ) or die('invalid options, see --help');
    
    if ($options{help}) {
      print $help;
      exit 0;
    }
    
KennyMinigun ★★★★★
()
Последнее исправление: KennyMinigun (всего исправлений: 1)
Ответ на: комментарий от KennyMinigun

Иньекции не будет там , насчет sudo пока думаем как оставить

немного не понятно , я не хотел много ф-ций , ведь там и так парситься вывод lxc* кроме blkio

Выдели парсинг отдельных (форматов) файлов в отдельные функции, тогда интерфейс станет проще

pinachet ★★★★★
() автор топика
Ответ на: комментарий от KennyMinigun

Запуск команд через backticks — сильно подвержен иньекциям

Там переменные не используются внутри бэктиков. А если нужна серьезная секьюрность, то все равно используется taint mode, который такие вещи отловит

annulen ★★★★★
()

Гогно (да, это неконструктивная критика).

Что будет, если что-то случится между строками open($status_file) и close($status_file)? Программа упала, файл остался открытым?

Virtuos86 ★★★★★
()
Последнее исправление: Virtuos86 (всего исправлений: 1)
Ответ на: комментарий от Virtuos86

Если там не сегфолт то думаю должно быть не так страшно.ИМХО

pinachet ★★★★★
() автор топика
Ответ на: комментарий от Virtuos86

Программа упала, файл остался открытым

Просто вдумайся в то что пишешь

anonymous
()
Ответ на: комментарий от annulen

если так заморачиваться, то уж лучше Getopt::Long::Descriptive

anonymous
()

Если уж пишешь гигантский цикл, хотя бы комментируй блоки. Вчитываться в перловку чтобы понять что там творится никому неохота. Перл он такой НЕ самодокументируемый ни разу.

anonymous
()
Ответ на: комментарий от Virtuos86

Что будет, если что-то случится между

А если между астероид прилетит, мы тут ваще все умрем.

anonymous
()

Нет цитат из Библии...

anonymous
()
for (@out) { s/ //g }
y/ //d for @out;
anonymous
()

Начал отслеживать треды с тегом «perl» на ЛОРе некоторое время назад и понял, что мало кто знает perl. Под «знает» я подразумеваю базовый уровень которого можно достичь прочитав пару книжек от корки до корки и попрактиковавшись пол годика. В основном на perl пишут бывшие или действующие админы по примерам со stackoverflow, получается у них НЕ ОЧЕНЬ, что мимокрокодилы относят на счёт языка.

Без обид. Всем добра!

anonymous
()
use utf8;

Нужна только чтоб сказать интерпретатору, что в тексте скрипта есть utf8 символы.

my $foo;
my $bar;

my ($foo, $bar);
anonymous
()
Ответ на: комментарий от anonymous

++ Даже книгу с верблюдом, кажись, не читали. А она классным языком написана, и сразу в такие детали погружает...

То же с python: подписался, чтобы что-то узнать о нём. Ага щас. Хелло ворды. Тысячи их.

По сабжу: use perlcritic, читай модули разрабов. У тебя в целом, терпимый скрипт. Если же начать разбор по фэншую, то полностью переделать надо.

Deleted
()
Последнее исправление: Deleted (всего исправлений: 1)
Ответ на: комментарий от Deleted

То же с python: подписался, чтобы что-то узнать о нём. Ага щас. Хелло ворды. Тысячи их.

Это форум, куда приходят с проблемами. Что ты можешь узнать из чьих-то проблем? Что мир несовершенен?)

Virtuos86 ★★★★★
()

Какой-то свиной отблев. Среднестатистический код на пыхе и то код получается красивее.

lovesan ★★
()
Ответ на: комментарий от lovesan

Какой-то свиной отблев.

Это перл, детка. И чувак еше старался писать внятно, а не играть в перл-гольф как поехавшие гуры.

anonymous
()
Ответ на: комментарий от lovesan

скорее таков твой комментарий

anonymous
()

Вечер в хату! Тема ещё актуальна?

anonymous
()
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.