Re: [RFC] Squid process model

[Amos Jeffries]

On 2/11/2013 2:11 a.m., Amos Jeffries wrote:
> Hi all,
>   As some of you are no doubt aware that one of the design issues we are
> facing with Squid these days is the process model. The current model has
> a very init.d centric design and shoots itself in the foot when
> encoutering third-party daemon management systems such as upstart,
> systemd, and a few other less popular ones. Not that it supports many
> uses of init.d very well either.
> 
> I have been thinking of updating the -N, -n -z, -k command line options
> behaviour very slightly to make things a bit more flexible in a backward
> compatible way.
> 
...
> 
>  -n - Windows service name
> 
> The Windows build of Squid requires a -n option to point at the
> particular named service which is running in the background. Which
> defaults to the name "squid" when omitted.
> 
> Making this option available outside Windows shows some promise. With
> the service name being used as prefix for shm_*() paths, default pid
> file name and similar things which are required to be identical between
> all processes in a Squid instance this will restore the ability to run
> multiple independent Squid services on the same machine regardless of
> whether SMP support is used or not.

The opening up of -n is now done in trunk. Anything which needs to be
unique for the instance/service should begin to make use of the global
char* service_name as part of its uniqueness.


I am working now on updating the foo.ipc socket paths. That is tracked
in http://bugs.squid-cache.org/show_bug.cgi?id=3608 with a patch
available shortly.


PID file is the trickiest piece. I am thinking we will need to convert
the squid.conf string directive from file+path into just path, and
generate the filename from the service_name instead of allowing manual
configuration of it.


Amos

Build failed in Jenkins: 3.HEAD-coadvisor #180

[noc]

See 

--
Started by an SCM change
Building remotely on co-advisor in workspace 

Cleaning workspace...
$ bzr checkout --lightweight http://bzr.squid-cache.org/bzr/squid3/trunk/ 

Getting local revision...
$ bzr revision-info -d 
info result: bzr revision-info -d 
 returned 0. Command 
output: "13245 squ...@treenet.co.nz-20140124015715-kqpjisr3dlse9786
" stderr: ""
RevisionState revno:13245 
revid:squ...@treenet.co.nz-20140124015715-kqpjisr3dlse9786
[3.HEAD-coadvisor] $ /bin/sh -xe /tmp/hudson517083391892530637.sh
+ /home/jenkins/script/makeOneTest.pl --config=/home/jenkins/script/config.cfg 
--audited=http://eu.kinkie.it/coadvisor-artifacts/52/archive/result --jjid=180
Number of potential regressions found: 1
Audited results revision: 13003
(http://eu.kinkie.it/coadvisor-artifacts/52/archive/result)
Revision under test: 13245
(http://eu.kinkie.it/coadvisor-artifacts/180/archive/result)
Details:
success unstabletest_case/rfc2616/ccExtension-max-agex-toSrv
Build step 'Execute shell' marked build as failure
Archiving artifacts

Re: [PATCH] client-side redesign pt1 - Comm::TcpReceiver

[Amos Jeffries]

On 22/01/2014 6:45 p.m., Alex Rousskov wrote:
> On 01/07/2014 02:52 AM, Amos Jeffries wrote:
>> Updated patch attaced for audit.
>>
>> This one includes all the currently known bits for server-side delay
>> pools so no audit omissions this time around.
>>
>> On 4/01/2014 8:16 a.m., Alex Rousskov wrote:
>>> On 12/03/2013 10:05 PM, Amos Jeffries wrote:
 This patch abstracts the TCP socket operations out of existing
 client-side code into a class which can be re-used for any TCP socket code.
>>>
>>>
 +namespace Comm {
 +
 +class TcpReceiver : virtual public AsyncJob
 +{
 +public:
>>>
>>> Missing class description. 
> 
> You may want to add an "XXX: finalize and describe scope" comment in
> lieu of that description for now.
> 

Done.

> 
 +void
 +Comm::TcpReceiver::stopReading()
 +{
 +/* NP: This is a hack only needed to allow TunnelStateData
 + * to take control of a socket despite any scheduled read.
 + */
 +if (reading()) {
 +comm_read_cancel(tcp->fd, reader_);
 +reader_ = NULL;
 +}
 +}
> 
>>> Why is this called a hack?
> 
> 
>> It only exists because tunnel.cc takes over the FD. Really nasty (the
>> dropped data race condition).
> 
> Let's call this method stopReadingXXX() so that it is clear that any
> caller is "doing it wrong" and to discourage use.

Done.

> 
>> +bool
>> +Comm::TcpReceiver::maybeMakeSpaceAvailable()
>> +{
>> +/*
>> + * why <2? Because delayAwareRead() won't actually read if
>> + * you ask it to read 1 byte.  The delayed read(2) request
>> + * just gets re-queued until the client side drains, then
>> + * the I/O thread hangs.  Better to not register any read
>> + * handler until we get a notification from someone that
>> + * its okay to read again if the buffer cannot grow.
>> + */
>> +
> 
> That comment does not match the code. The method is not scheduling any
> reads, it is just allocating buffer space. I would just add a simpler
> comment like this:
> 
> // XXX: <2 is probably for delayAwareRead() in callers to work, as
> // discussed in HttpStateData::maybeReadVirginBodymethod().
> 

Er. That method is erased now. The comment (and code) was *moved* into
the spot you are talking about.
I've done a slight change to the comment to make it mention how the
read(2) is related to this methods code.

> 
>> +Comm::TcpReceiver::readIoHandler(const CommIoCbParams &io)
> 
> Drop the "Io" part because "read" already implies "Io"?
> 

Done.

> 
>> +assert(Comm::IsConnOpen(tcp));
>> +assert(io.conn->fd == tcp->fd);
> 
> These should probably be Must()s since we are running under job protection.
> 

Okay. Done.

> 
>> +/*
>> + * Don't reset the timeout value here.  The timeout value will be
>> + * set to Config.Timeout.request by httpAccept() and
>> + * clientWriteComplete(), and should apply to the request as a
>> + * whole, not individual read() calls.  Plus, it breaks our
>> + * lame half-close detection
>> + */
> 
> 
> Too specific to the servers/ code (given your desire to use this for
> clients/, servers/, and even adaptation services).

Polished up.


>> +if (size < 0) {
>> +if (!ignoreErrno(xerrno)) {
>> +debugs(5, 2, tcp << " read failure: " << xstrerr(xerrno));
>> +return true;
>> +} else if (!inBuf.hasContent()) {
>> +debugs(5, 2, tcp << ": no data to process (" << xstrerr(xerrno) 
>> << ")");
>> +}
>> +}
>> +
>> +return false;
> 
> 
> The ignoreErrno() == true case above does not work well for the caller:
> If there was a minor error, the caller should keep reading, not
> re-processing the old inBuf content that has not changed. More on that
> further below (**).

Added calls to try growing the buffer and read again without processing.

For now I have also picked to treat the full buffer case same as 0-sized
read. With a call to the maybeFinishedWithTcp() and setting up
half-closed monitor to watch for full closure.

> 
>> +if (readWasError(io.flag, io.size, io.xerrno)) {
>> +noteTcpReadError(io.xerrno);
>> +io.conn->close();
>> +return;
>> +}
> 
> The io.conn->close() part should probably be inside noteTcpReadError(),
> which kids may overwrite and call. More on that futher below (**).

Maybe. I left it out to ensure that no matter when the child called its
parents function the connection would only be closed after the calls all
return. Makes it harder for them to step on each others feets.


> 
>> +// if the connection is still possibly sending
>> +// the child class may be able to stop immediately
>> +if (const char *reason = maybeFinishedWithTcp()) {
>> +stopSending(reason); // will close connection
>> +return;
>> +}
> 
> Missing "not" somewhere in the top comment?
> 

No. just bad phrasing. Polished it up.

> 
>> +if (io.fla

Re: BZR local server?repo?

[Kinkie]

Hi,
  my 2c.

I use a more centralized approach:
bzr co bzr+ssh://remote.addr/repo
and then just bzr ci, optionally --local if I am disconnected.

On Thu, Jan 23, 2014 at 10:38 AM, Henrik Nordström
 wrote:
> tor 2014-01-23 klockan 09:53 +0200 skrev Eliezer Croitoru:
>> Since I do have a local server I want to have an up-to-date bzr replica.
>> I can just use checkout or whatever but I want it to be be updated etc.
>>
>> I am no bzr expert so any help about the subject is more then welcome.
>
> What I do is that I set up a shared bzr repository that collects the
> branches I want to monitor/backup, then have a cron job to update
> branches in there from their source repositories.
>
> bzr init-repo --no-trees /path/to/shared/repo
> cd /path/to/shared/repo
> bzr branch remote_url local_branch_name
> bzr branch ... [repeat per barach to mirror]
>
> then a cron job that runs
>
> bzr pull --overwrite
>
> in each branch to keep them updated
>
> The reason for --overwrite is to handle if/when history of the master
> repo is tweaked... This is optional, and without --overwrite you will
> need to manually recover the mirroring in such events, which is also a
> good thing as it alerts when something bad is done to the mirrored
> repository history.
>
> The reason for --no-trees is to be able to also push working branches to
> the same repository for backup purposes. And don't really need checked
> out copies of the sources of each branch on the server
>
> If server side checkouts is needed then it's easily created separately
>
>bzr checkout --lightweight /path/to/shared/repo/branch
>
> --lightweight is entierly optional, and depends on what you want to use
> the checkout for.
>
> Regards
> Henrik
>



-- 
/kinkie

Re: BZR local server?repo?

[Henrik Nordström]

tor 2014-01-23 klockan 09:53 +0200 skrev Eliezer Croitoru:
> Since I do have a local server I want to have an up-to-date bzr replica.
> I can just use checkout or whatever but I want it to be be updated etc.
> 
> I am no bzr expert so any help about the subject is more then welcome.

What I do is that I set up a shared bzr repository that collects the
branches I want to monitor/backup, then have a cron job to update
branches in there from their source repositories.

bzr init-repo --no-trees /path/to/shared/repo
cd /path/to/shared/repo
bzr branch remote_url local_branch_name
bzr branch ... [repeat per barach to mirror]

then a cron job that runs

bzr pull --overwrite

in each branch to keep them updated

The reason for --overwrite is to handle if/when history of the master
repo is tweaked... This is optional, and without --overwrite you will
need to manually recover the mirroring in such events, which is also a
good thing as it alerts when something bad is done to the mirrored
repository history.

The reason for --no-trees is to be able to also push working branches to
the same repository for backup purposes. And don't really need checked
out copies of the sources of each branch on the server

If server side checkouts is needed then it's easily created separately

   bzr checkout --lightweight /path/to/shared/repo/branch

--lightweight is entierly optional, and depends on what you want to use
the checkout for.

Regards
Henrik

Re: [PATCH] client-side redesign pt1 - Comm::TcpReceiver

[Amos Jeffries]

On 22/01/2014 10:32 p.m., Henrik Nordström wrote:
> tis 2014-01-21 klockan 22:45 -0700 skrev Alex Rousskov:
> 
>> All the TCP clients and servers you are willing to include (as future
>> TcpReceiver kids) in the current project scope have at least one thing
>> in common -- they all read and write protocol messages over [persistent]
>> TCP connections. Why do you think it is a good idea to focus on just the
>> reading part so much? If you want a common parent for all those agents,
>> why not handle both actions in that common parent, especially if you
>> already find it necessary to add a little bit of "send" part into the
>> TcpReceiver?
> 
> I haven't looked in detail at what is being done, but also keep in mind
> that if you layer TLS ontop of a TCP connection then the TLS layer needs
> both read & write events to the TCP connection.
> 
> From a code design perspective it should be the same interface for
> application protocol data for both TCP and TLS.
> 
> For some food of thought I would throw SCTP into that mix as well, which
> adds another layer ontop of the socket to track which SCTP channel the
> application level connection runs on. So we might have aplication /
> TLS / SCTP channel / SCTP(protocol).
> 
> Any design that tries to make application level code (i.e. http/ftp
> protocol handlers etc) needing to be aware of TcpReceiver/Sender is
> plain wrong imho.

Yes. I was loosely thinking of completing this TcpAgent then working on
TlsAgent, etc. with the same interface methods. COAPS and UDP I/O
streams are also on the wishlist as other *Agent types.

With Alex proposal to merge the Comm::Write API into these fully I think
we have a good design to go forward with that can be spread across all
those transport types relatively easily.

> 
>> If tomorrow we have both TcpReceiver and TcpSender, and all their kids
>> have both classes as parents, with complex inter-parent dependencies
>> that are already showing in TcpReceiver, we are doing something wrong.
> 
> We should only have channels and listeners at application protocol level
> code (HTTP, ftp, whatever). To most code it does not matter what
> transport these use.
> 
> Details of each transport implementation is more free. Not so much need
> to argue on details there. But should not be visible in other code.
> 

Precisely. The long-term plan is to have the clients/ and servers/
classes using these FooAgent classes or their children for abstract
access to buffered byte streams instead of dealing with any connection
specifics themselves.


> 
>>> It only exists because tunnel.cc takes over the FD. Really nasty (the
>>> dropped data race condition).
> 
> yes, CONNECT requests will be a mess. But as above, should take over the
> connection. Any application payload already read need to be either
> handed back to the connection or given to the CONNECT forwarder.
> 
>> Let's call this method stopReadingXXX() so that it is clear that any
>> caller is "doing it wrong" and to discourage use.
> 
> Yes. What is needed is to stop accepting pipelined requests after a
> CONNECT. Stopping reading is justa brute-force way of accomplishing
> that.
> 
>> I do not know or do not remember, sorry. The whole "delay pools cannot
>> deal with single-byte buffers" is a delay pools bug, outside this
>> project scope.
> 
> I think it's an ancient parser issue which hopefully is long since
> fixed.
> 

Thank you. So we have several possible reasons. At least delay pools is
likely to be around for some time yet. So I'm going to leave this as-is
for the purposes of this patch. It is not exactly harmful to any design
as-is just nasty looking code.

Amos

Re: [PATCH] client-side redesign pt1 - Comm::TcpReceiver

[Amos Jeffries]

On 23/01/2014 7:19 p.m., Eliezer Croitoru wrote:
> On 07/01/14 11:52, Amos Jeffries wrote:
>> Updated patch attaced for audit.
> 
> I do not see any patch in the mailing list post, Are we talking about
> "This one with the mk2 patch actually attached."?

Yes.

Amos

Re: BZR local server?repo?

[Robert Collins]

Put bzr update in cron?

Or if you want an exact copy of trunk, use 'bzr branch' then 'bzr
pull' to keep it in sync.

On 23 January 2014 20:53, Eliezer Croitoru  wrote:
> Since I do have a local server I want to have an up-to-date bzr replica.
> I can just use checkout or whatever but I want it to be be updated etc.
>
> I am no bzr expert so any help about the subject is more then welcome.
>
> Thanks,
> Eliezer
>
>