On 02/21/12 06:07, Tomas Dzik wrote:
Hi Ethan,
thanks for your comments.
The updated webrev is here:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7120099-2/
I intentionally didn't run hg recommit. I will do it when you will be
OK with the fix.
For the rest please see in-line:
Dne 16.02.12 22:39, Ethan Quach napsal(a):
On 02/07/12 08:14, Tomas Dzik wrote:
Hi Ethan,
thanks a lot for your review and comments. I fixed most of them. For
the rest please see in-line.
New webrev is here:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7120099-1
Please let me know whether it is acceptable now.
Dne 3.02.12 01:49, Ethan Quach napsal(a):
Tomas,
Thanks for fixing this. A few comments ...
282-297 - Couldn't you just call AIdb.getTableCriteria() with
humanOutput=True at line 282 to prevent having to call it again at
292?
I intentionally call it twice here because its hard to distinguish
empty criteria in human readable output.
Why do we need to distinguish empty criteria if the calls are being
called with onlyUsed=True; doesn't that only return the criteria that
are non-empty?
No - that returns criteria which are non-empty in service. Say we have
service "solaris11u1-i386-06". This service has several manifests:
solaris11u1-i386-06
orig_default arch = i86pc
mac = AA:BB:CC:DD:EE:FF
ipv4 = 10.0.2.100 -
10.0.2.199
cpu = i386
mem = 1024 MB
network = 10.0.0.0
platform = i86pc
zonename = some-zone-name-1
some-new_manifest-1 arch = sparc
mac = AA:BB:CC:DD:EE:FF
mem = 1024 MB
some-new_manifest-2bbbbbbb Default None
And onlyUsed=True here means to return only criteria used in any of
the manifest. It's not nice but I didn't write these functions.
As you can see:
>>> AIdb.getCriteria(aiqueue, table=AIdb.MANIFESTS_TABLE,
onlyUsed=True, strip=False)
[u'arch', u'MINmac', u'MAXmac', u'MINipv4', u'MAXipv4', u'cpu',
u'platform', u'MINnetwork', u'MAXnetwork', u'MINmem', u'MAXmem',
u'zonename']
>>> AIdb.getCriteria(aiqueue, table=AIdb.MANIFESTS_TABLE,
onlyUsed=False, strip=False)
[u'arch', u'hostname', u'MINmac', u'MAXmac', u'MINipv4', u'MAXipv4',
u'cpu', u'platform', u'MINnetwork', u'MAXnetwork', u'MINmem',
u'MAXmem', u'zonename']
And output of AIdb.getTableCriteria() for manifest with no criteria:
>>> AIdb.getTableCriteria("some-new_manifest-2bbbbbbb", 0, aiqueue,
AIdb.MANIFESTS_TABLE, humanOutput=True, onlyUsed=True)
(None, u'', u'', None, None, None, None, None, None, None, None, None)
>>> AIdb.getTableCriteria("some-new_manifest-2bbbbbbb", 0, aiqueue,
AIdb.MANIFESTS_TABLE, humanOutput=False, onlyUsed=True)
(None, None, None, None, None, None, None, None, None, None, None, None)
As you can see there is only difference in MAC address (MINmac,
MAXmac) which is converted to empty string which I can easily test,
however it didn't look clean to me.
Anyway I changed it as you suggested and I can return it back if you
feel that original version was cleaner.
Yes, I think this is an issue with getTableCriteria(). As you noted,
the problem is that getTableCriteria() is returning a list of criteria
values for a given manifest or profile based on the used criteria for
the whole service, which seems incorrect. It should be returning the
criteria values that are used just for that manifest or profile
requested, (as it's block comment suggests.)
In your example above, the call to
AIdb.getTableCriteria("some-new_manifest-2bbbbbbb") should have have
returned an empty list to begin with. If you were to delete
"orig_default" and "some-new_manifest-1", that same call would then
return an empty list which seems very inconsistent.
The current implementation accounts for this variance by having the
caller adjusts itself based on what is returned which, while it works,
not by any means a clean implementation. Can you file a separate bug to
get this cleaned up?
For this current fix, could you revert it back to the way it was. (I
just realized that what you had in the original webrev was an exact copy
of the old implementation.)
889 - Is it possible to make this class support both -- printing
manifests for all services, or for just one? To me, it seems
awkward for
the code at 1487,1491 to use two different classes to print one
case or
the other. Why can't this class just take an optional service_name
argument for cases where only one service needs to be printed?
I would prefer not to do it because both options substantially differ
- they use different header and different output format. Also there is
different output format if there is just one manifest/profile
printed by
installadm list -p
root@S11:~# installadm list -p
Service/Profile Name Criteria
-------------------- --------
solaris11u1-i386-06
some-new-profile None
and if the same manifest is printed using installadm list -p -n
root@S11:~# installadm list -p -n solaris11u1-i386-06
Profile Criteria
------- --------
some-new-profile None
Understood. I would be ok with changing that to make the header and
output always be consistent with the first form of output above. For
example,
root@S11:~# installadm list -p
Service/Profile Name Criteria
-------------------- --------
solaris11u1-i386-06
some-new-profile None
service2
profile-foo None
profile-bar None
profile-blah None
service3
profile-X None
root@S11:~# installadm list -p -n service2
Service/Profile Name Criteria
-------------------- --------
service2
profile-foo None
profile-bar None
profile-blah None
Also ServicesManifestList() class contains list of
ServiceManifestPrint() classes and prints them by iterating through
them. So basically ServicesManifestList() is container for
ServiceManifestPrint().
However if you insist on changing it I can do it, but it would not be
nice and the switch between these 2 options would just be moved into
class itself which is not nice either. Just let me know.
Yes, I would prefer that. Given the above, I think doing this makes more
sense.
I did it.
Thanks. Looks much better now. The output looks fine to me as well.
One new question I have is regarding how lines get printed. The objects
seem to all return strings, and the entirety of everything that will get
printed is accumulated into a large string buffer. Are there any
limitations with this if the output is very large?
I am not aware of any limitation. I tried (on VirtualBox machine) to
create 2000 profiles, each with 4 criteria and to print them using
installadm -p. It took 45.417 sec (measured by time) and occupied
(according to top) about 45 MB of virtual memory. I did the same test
on virtual machine without my fix and it took 45.283s of real time and
occupied approx 42 MB of virtual memory.
OK, thanks for doing that test at least; I was wondering if this
approach would have ill effects on memory usage, but I guess not.
Also, I just tried your new list.py; there now seems to be an additional
empty line of output right after the command is invoked. This was not
there before, nor was it there in the first webrev.
I added it because I wanted to stay consistent with previous format. I
removed it again because it seems that you don't like it ;-).
thanks,
-ethan
Best regards,
Tomas D.
thanks,
-ethan
Best regards,
Tomas D.
thanks,
-ethan
On 01/20/12 04:10, Tomas Dzik wrote:
Hi all,
I would like to kindly ask whether someone has some spare time to
review my changes.
I know that these changes are quite substantial.
Best regards,
Tomas D.
Dne 30.12.11 15:46, Tomas Dzik napsal(a):
Hi all,
I would like to ask you for a code review for:
7120099 - "installadm list -p" should show criteria
This fix contains also changes to the way how "installadm list -m"
works.
Webrev:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7120099/
Testing:
1) Sources are pep8 clean
2) Test suite in the gate pass
3) I created several services with different manifests (Default,
Active,
Inactive) with several sets of criteria and verified that these are
printed as before
4) I also created profiles with different criteria and verified how
they
are printed.
5) I tried to change the version of installed service and verified
that
code correctly handles this exception.
I changed a little bit format how ignored criteria are printed (as
suggested in one of the previous code reviews).
Instead:
# installadm list -m -n default-i386
Manifest Status Criteria
-------- ------ --------
orig_default Default (Ignored: ipv4 = 192.168.1.2)
(Ignored: mem = 2048 MB - unbounded)
I now print:
# installadm list -m -n default-i386
Manifest Status Criteria
-------- ------ --------
orig_default Default (Ignored:
ipv4 = 192.168.1.2
mem = 2048 MB - unbounded
)
Best regards,
Tomas D.
_______________________________________________
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
_______________________________________________
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
