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.
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?
installadm.c:
-------------
810-814: are we dropping support for creating services for clients older
than this push?
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
installadm_util.c:
------------------
lines 114-118: do you need to free handle?
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?)
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?)
lines 895-896: I believe we support platform, network and cpu?
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
line 189: so we're allowing reusing image directories? (If so, why?)
(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?
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.)
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?)
cgi_get_manifest.py:
--------------------
(nit) line 181: I would recommend partition instead of split
(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?
Otherwise, thank you for keeping the GUI list methods available!
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?
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)
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.
Thank you,
Clay
On Mon, 13 Dec 2010, John Fischer wrote:
All (Ethan, Clay and Joe),
This is a request for a first review of the webserver design project.
I suspect that there will be at least 2 rounds if not more of review.
Please have feedback to me by Friday, December 17th, 2010.
The webrev is located at:
http://cr.opensolaris.org/~johnfisc/webserver-design/
This covers the design as specified in:
http://hub.opensolaris.org/bin/download/Project+caiman/auto_install/AI%2DWebserver%2DSpecification.pdf
I believe it covers the following CRs:
9538 AI webserver not restarting after server reboot
8282 Port sharing with multiple AI webservers
8708 SMF: ai-webserver not restarted if it crashes
8561 Blocker bug for AI Webserver design work
6047 HTTP_PORT should be obtained programmatically, rather than
defined
The design is fairly simple. The changes for the design include:
o replaces the cherrypy webserver with a cgi-bin script called
cgi_get_manifest.py.
- the scripts are very similar except without the cherrypy specific
code
o replacement of port reference for database with servicename
- an implication of the webserver consolidation, majority of the
important code
changes
o shell scripts no longer start/stop webserver
o compatibility for older services - configuration changes
o build changes - Makefile changes
There are a few additional changes from pep8 checks. I didn't
do the majority of the pylint changes because several of the files
require extensive changes.
Let me know if anyone needs a code walk through.
Thanks,
John
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
