Clay,
No problem with the delay. Having the extra set of eyes look at it is worth
the wait. I appreciate your help.
Responses below. I'll update the webrev shortly.
Ethan and Dave,
Any comment on this question from Clay?
General Architecture:
---------------------
It appears that installadm disable -t <service> is no a no-op as it
doesn't change a service's SMF state property so we'd still run aimdns
for it (as pointed out when aimdns went in) and now the webserver
would still run for it too and it will keep the service from
transitioning to maintenance -- should we get rid of disable -t or do
people really use it?
Thanks,
John
On 12/28/10 12:31 AM, [email protected] wrote:
Hi John,
My apologies this got dropped for so long. This looks good! My
only catches are as follows:
delete-service:
---------------
511: why was this indentation chosen? I usually try to match under the
punctuation(parens, brackets, braces, etc.) or simply do a four
space
indent.
Placing this under the parens would have caused the indentation to another
line for the os.path.join(). I have changed the indentation to be:
otherMenu =
com.GrubMenu(file_name=os.path.join(baseDir,
menuName))
menuName lines up with baseDir.
old lines: 736+737: where did the service refresh move? As after removing
a service you must refresh it or it will linger in
aimdns and the webserver won't it?
No. The stop_service() function changes the 'status' to 'off'. This
automatically
causes a refresh to happen.
installadm.c:
-------------
810-814: are we dropping support for creating services for clients older
than this push?
No. What happens during the create the port number gets added to various
client files. With this change the port number becomes the default port
(currently
5555). The old code used to figure out what the next port number would be.
We simply are choosing to use the default port. The cgi-bin code keys
off of
the port number and service name in determining the path for the client
picking
which ever one is more appropriate for the request.
architectural: If we are removing support for creating older services,
(Perhaps for should we do away with the backwards compatible setup on
Ethan/Dave?) line 835 -- and just use $serverIP for srv_address?
This would prevent the "fallback" mechanism from working
when the boot server and AI server differ and mDNS AI
broadcasts don't reach the AI client for some reason but
would simplify our use-cases
Not quite following here.
installadm_util.c:
------------------
lines 114-118: do you need to free handle?
No. That is handled within installadm.c at line 814 because it is
allocated at line 812 of installadm.c.
list.py:
--------
line 304: If I understand this correctly, this assumes the server
is on a class C network which is not always true -- further it
only looks at one network. My apologies for not realizing this
during Multihomed!
(Why not use a similar approach as delete_client which look
under the entirity of /etc/netboot?)
It probably should. The line was changed only because pep8 complained
it is not part of the actual project. pep8 did not like the fact that
there were
no spaces around the '+' sign.
lines 766/768: it seems these two comments are contradictory (we're not
922/924: ensuring new server setup so much as testing it in
line 767/923?)
I'll change ensure to test.
lines 895-896: I believe we support platform, network and cpu?
Do we? I am not 100% sure about that one. Also this comment change
again was part of the pep8 changes.
setup-service.sh:
-----------------
(nit) line 148: I would suggest using "AI${name}/status" instead of
"AI$name/status" but it's not necessary -- it simply helps
one's code scan speed and calls out there is a variable
there
Updated.
line 189: so we're allowing reusing image directories? (If so, why?)
I have not changed the architecture here but only enhanced what was
already allowed. I am not familiar enough with the history of why this
was allowed before.
(nit) line 232: it would be nice to offer the user a reason why using
this manifest is of concern -- e.g. "Warning: Using
system default manifest instead of image specific
manifest" -- also do we still support creating a service
from an image which existed before we started putting
manifests on the ISOs?
I have not changed the functionality only the readability of the code
at lines 234-235 (i.e., I broke the warning into 2 lines). It would seem
to me that one would be a warning and the other would be the expected
behavior. If the expected behavior is to use the system default manifest
then you are correct that using the image manifest should produce a
warning. The original code seems to indicate that using the default system
manifest rather then the image manifest is the warning. Have things
changed? Also again I am not changing the functionality here.
httpd.conf:
-----------
line 262: how do you provide access to wanboot.conf via the server? (This
line was enacted for multihomed so we didn't have to copy
wanboot.conf and IPS could properly manage it.)
Oh, man. I think I got bit in a code merge. I'll merge the conf files
stuff.
svc-install-server:
-------------------
lines 74-99: There is no provision for one to add or remove a legacy
service short of stopping and restarting the AI service?
(E.g. this code doesn't run on a refresh?)
Nice point. I'll do the same thing within a refresh.
cgi_get_manifest.py:
--------------------
(nit) line 181: I would recommend partition instead of split
Keith said the same thing. Hahaha.... This is actually part of the original
code. Just a copy and paste job. I'll use partition.
(nit) lines 192-204: LXML can generate HTML if that'd be easier for
maintainability (see lines 244-251 for an example)
(observation) lines 271-288: if performance of serving the manifests
becomes too slow, this is a potential
bottleneck if the manifests grow
really large (e.g. for derived manifests) as
CherryPy and Apache will stream files (and do
other tricks) (see
http://helpful.knobs-dials.com/index.php/CherryPy#Content)
architectural: Would it be easier to make a function to print to stderr &
stdout since that's done pretty often through out -- with
potential for typos between the two?
Probably. But since this functionality is not part of the spec and won't
be accessed by users it is a low priority.
Otherwise, thank you for keeping the GUI list methods available!
Your are welcome. But sshh.... ;-) It's not part of the spec.
General Architecture:
---------------------
It appears that installadm disable -t <service> is no a no-op as it
doesn't change a service's SMF state property so we'd still run aimdns
for it (as pointed out when aimdns went in) and now the webserver
would still run for it too and it will keep the service from
transitioning to maintenance -- should we get rid of disable -t or do
people really use it?
Not sure how it is actually used. Ethan and Dave?
There should be a check so that some clever jerk doesn't make an AI
service named image-server causing all sorts of havoc (I didn't catch
one)
I am not sure what would happen if someone did that one. Not sure it is
a security issue other
than a denial of service. But then again it would take a system
administrator to do it.
Not directly related but you can get rid of all of the queuing in
AI_database.py as that was only needed to support a multithreaded
application -- the old AI webserver.
Ah... OK. I'll move the run portion of the thread code down into the DB
class.
Thank you,
Clay
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
