Skip to content

Comments

Review of php-sync#6

Open
mokraemer wants to merge 43 commits intoimmobiliare:masterfrom
mokraemer:master
Open

Review of php-sync#6
mokraemer wants to merge 43 commits intoimmobiliare:masterfrom
mokraemer:master

Conversation

@mokraemer
Copy link
Contributor

So, in short what did I do:

  1. create a service that can be started at system level most distributions use systemd
  2. for 1, have parameter -u, -g in global config, can still be given at prompt
  3. move config at a common directory: /etc/sysconfig/sfs - debian uses /etc/defaults - is just an adjustment in systemd file where is searches by default
  4. added a few mkdir's - maybe too much
  5. corrected regular-expr. which didn't handle 123_name_name.fqn.tld_[...].batch
  6. Changed the install documentation for php-sync & how to use this tool via rsync over ssh, added some prequists

I think that's all for now :-) Currently I'm just expermenting a bit, with the default config it takes 60 seconds before a file is transferred to the destination.

@mokraemer
Copy link
Contributor Author

as I said, I'm unfamiliar to git - this merge request has everything in it, I did in the fork. It contains also the changes we disagreed, sorry for that.

@mokraemer
Copy link
Contributor Author

=> almost forgotten: I changed the name "sync.php" to sfs-sync.php, so it is more clear in the processlist, or as a global executeable

//batch dir, if this is the same for all nodes
$BATCHDIR = '/mnt/batches/';
//drop privileges to user if different from 0, needs a restart if changed
$UID = 0;
Copy link
Collaborator

@lucabrunox lucabrunox Apr 15, 2016

Choose a reason for hiding this comment

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

I do not want to handle this in php sync. Systemd is perfectly able to drop privileges within the service file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was your code - I only added the options to the config file,in case someone don't use systemd. And for templated services you can't add a dynamic user/group too. You had to write an extra config for systemd to read.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes didn't want to add extra stuff like this to the daemon. It's old code and we hope to transition to systemd. What's the reason for adding $UID to the config too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea behind templated services is to have one template and instantiate this template with one parameter. @xxx. So you don't have to manually copy the file & change it.
So the parameter is mostly a port or a configuration file. Since systemd can't handle php files we would need 2 config files for one service. I thought this makes it more complex.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok so the problem is that the two services are running with different users. That's fine then. Once again we have to reinvent things at app level because the service manager isn't able to template User and Group. Understandable :)

@mokraemer
Copy link
Contributor Author

I had another problem, what should be fixed by my latest commit:
proc_open was used in blocking mode. I could see some rsync processes that don't wanted to proceed. They were all stuck in "write(2,....", so I changed to non-blocking mode. I had to do the same for the input stream, because not all data was written to the process - in the output sometimes rsync error 13 appeared.

All these errors seems only to occur if you have "big transactions", e.g. >1000 files are changed which can be due to chmod/chown, or a mv operation.

@lucabrunox
Copy link
Collaborator

I'm quite opposed to add a systemd service. It's just few lines of code and you are probably going to edit it anyway. It complicates all the documentation about /etc assumptions and binary installation assumptions.
Systemd files and /etc configurations should be distro-specific. So I'd rather have a debian/ building a .deb with all the correct stuff in place, rather than writing wrong assumptions in the most generic manner.

docs/INSTALL.md Outdated
--------------------
Install php, if not already present.

Make sure you have installed php together with:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please mention this only applies to rpm-based distros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does debian have only one big php install file?
On php.net these modules are marked as optional, so they may not be installed. I can name this after the php modules that have to be present.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't have to install anything in addition to php on debian.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like the current situation:
https://packages.debian.org/en/wheezy/php5-cli
but this may change with newer releases, php states this as extensions which are optional, so its to the maintainer to bundle all, or make these extra packages:
http://php.net/manual/en/book.sem.php

I changed the documentaion which should make it clearer.

foreach ($batches as &$batch) {
if (!preg_match ("/^[^_]+_([^_]+)_[^.]+\.batch$/", $batch, $match)) {
if (!preg_match ("/^\d+_([^_]+)_.*\.batch$/", $batch, $match)) {
/*if(!empty($this->config["LOG_DEBUG"])){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this commented?

@mokraemer
Copy link
Contributor Author

I changed the glob back to readdir, as suggested.
I had one big mistake in combining the batch-array.
I still got some issues on rsync caused by proc_open. I think they're finally done. I tried to wait for the other process by using "select" - but ran into an deadlock. This may be improved, currently I do some polling of the process status which is the only reliable source if the process did terminate or not.

@rafalo79 rafalo79 requested review from lucabrunox and removed request for lucabrunox October 19, 2017 15:33
@chewi chewi mentioned this pull request Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants