On 08/12/10 03:23 PM, Brock Pytlik wrote:
On 08/12/10 01:59 PM, Shawn Walker wrote:
On 08/11/10 07:00 PM, Brock Pytlik wrote:
On 08/ 9/10 01:03 AM, Shawn Walker wrote:
...
As an aside, it'd be nice to find a less wordy name for that
subcommand. Is there a reason it needs to be 'add-signing-ca-cert'
specifically?
Suggestions are welcome. It's add because you're adding, not removing.
It's signing because it's the manifest signing cert, not the certs used
for ssl. It's a ca because it's a CA cert and not an intermediate cert.
I suppose cert could be lopped off, though at this point in the dev
cycle, I'd rather not make changes like this unless they offer clear and
substantial benefits.
So, my suggestions/notes are:
* Dan recently noted that it was useful for subcommands to indicate
the domain they are working in in the name of the subcommand. So,
as an example, my current changeset has 'get' and 'get-pub'. You
may want to ask Dan about this.
* I think you could shorten the name to 'add-signing-ca' at least,
with the implication being that you add a signing cert authority
by providing a certificate file. I don't think you need the
'-cert' in the subcommand name.
...
I think the locking here is fairly complex because they keep getting
picked up and dropped. I think it would be easier to understand and
maintain if we did either what we've done in other places w/ wrappers
which are responsible for taking and dropping the locks, or keep the
existing approach but make each function take the lock only if
self.__locked is False, and then dropping the lock only if it was taken
in that function's invocation.
I don't understand the view of the locking here being complex. The
lock() method is only called in two places. The only bit of logic here
that I could possibly understand that being applied to is the unlock()
that happens during _generic_update_index() if
rebuild_index_from_scratch has to be called.
I could try to do what you describe, but there are some additional
safeguards I'd have to put in place that would actually make this more
complicated. In the case of rebuild_index_from_scratch, since the
index directory is being removed anyway, it's necessary to drop the
lock and obtain it again because of the file lock that's needed.
*shrug* Ok, I had a lot of trouble understanding when locks were taken
and dropped, and how that interacted with the rats nest of calls that
the indexer makes, but if I'm the outlier, that's fine.
The simple version is, locking only happens when _generic_update_index
gets called or when rebuild_index_from_scratch is called since
everything that modifies the index passes through those functions.
Why not have the indexer use the same makedirs as misc provides? The
reason for having it raise search_errors was that it was a module shared
by both client and server, while api_errors were client only. Since this
no longer the case (server modules import misc which imports api_errors)
we might as well consolidate both the exception space and the makedirs
functions.
It was only to preserve the existing behaviour.
I think this is a good time to nuke that behavior.
So the one downside I see to that is right now, because we know it's
specifically a search related failure, we sometimes tell the user to
execute 'pkg rebuild-index' or some such command.
If we wanted to remove this custom search permissions exception, then
we'd also have to consider having the permissions exception be able to
indicate what type of permission exception has occurred (search, ?, etc.).
modules/server/depot.py:
What's the timeout of .5 seconds on line 2026 buy?
I'll add a comment, but that's basically necessary to keep the depot
server from burning through a large amount of CPU and so that the
depot server doesn't hang on shutdown (it waits at least timeout
seconds before exiting, which if you don't specify a timeout means
forever).
I could specify a longer timeout, but that directly affects shutdown
time and the amount of time before the depot might start a queued task.
.5 secs makes sens to me in that case. I wasn't clear whether having no
timeout would cause a hang or not.
Yes, I've added a comment because I had forgot as well and had to verify
the behaviour without the timeout.
...
Do the background processes happen in the same process or are they
forked out by cherrypy? If it's that same process, then that concerns me
as indexing previously happened in a separate process from the depot,
which meant the memory it used was returned to the system when the
indexing was done, instead of being attached to the depot for as long as
it was running.
If the memory usage of the depot server after indexing is a concern
for the user, they can restart it, or alternatively use pkgrepo on the
filesystem path of the repository the depot's serving. I'm happy to
document that in the man pages.
It was intentional that they happen in the same process. Ultimately, I
didn't feel that the complexity or problems involved with forking were
worth it to solve a resource issue.
Ok, please include that information at least in a flag day since it
means that people deploying pkg.depotd should potentially change how
they do things. Have you discussed this w/ Eric to make sure that we'll
be making the appropriate changes?
That will be part of the flag day process.
There's no way to abort a task running in the background?
No, and that's intentional. I don't want to implement an overly
complex mechanism here. There was no way to stop a background indexing
either in the past, so this isn't any worse than what we already had
in this way.
I don't understand why you don't want to allow the user some method of
stopping a potentially very long running action short of nuking their
entire depot. I'm fine with simple mechanisms, but not when the desire
for code simplicity makes users lives more difficult. If they can't
queue up more than one background task, I think some kind of abort
really is necessary (and I'd argue even with the ability to queue up
background processes, abort would be useful).
I'm going to have to pass on this for now. In general, I don't believe
this will affect most consumers in a negative fashion, and I don't have
the time to implement a better mechanism right now.
The current background task mechanism was the only reasonable compromise
I could make at the moment to allow pkgrepo users to perform some
administrative tasks with a network repository while still having a sane
way to check on its basic status.
Is the user told that the operation they just instructed the depot to do
is being done in the background and that they'll need to check the logs
for errors? Suppose a user wanted to refresh-packages and wait to do the
next step until that was finished, how would they accomplish that? Would
they loop polling stauts/0 until the operation they cared about was
marked as finished? It seems that, based on what I understand about how
indexing locking currently works, this wouldn't necessarily work because
the user could have their status/0 request processed in one of the times
between indexing dropping and picking up the lock, meaning the status
would be reported as online instead of indexing.
The same problem existed previously with indexing. Long term, we need
to figure out a better way to provide more detailed status information
to the user but that's not this wad. I'll open an RFE for it.
If you weren't dropping the lock in the middle of indexing, then you
couldn't encounter the race condition I mentioned. Previously, a user
would have started a depot with exit ready. Does that mechanism still
exist? If so, I'm fine with this being put down as an rfe. If not, I
think this needs to be addressed.
That mechanism still exists via pkgrepo at the moment when accessing the
repository via the filesystem. When the pkgrepo command exits, the
operation is finished in that case.
Cheers,
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
