All,
I have updated the webrev for the installadm list subcommand fixes.
It is located at:
http://cr.opensolaris.org/~johnfisc/list
It is updated with all the feedback so far except for the class
implementation and list-manifest suggestions by Clay. We decided
not to do those after a lengthy discussion with Ethan about the
issue(s). If anyone wants a detailed explanation of why please
feel free to ask.
Thanks,
John
John Fischer wrote:
> Clay,
>
> Thanks for the help. I appreciate you taking the time to type this
> up. I'll update the webrev tomorrow morning.
>
> Thanks,
>
> John
>
> Clay Baenziger wrote:
>> Hi John,
>> As we talked about in IRC and some high level thoughts:
>> *Please update your code to be in sync with slim_source (based off
>> the Python 2.6 Python files)
>
> Will do.
>
>> *Please move findTFTProot() from delete_service to
>> installadm_common so that it's more obvious there are multiple
>> consumers. Further it seems, which_arch would be a general
>> function well provided through installadm_common.
>
> Already moved.
>
>> *I'm rather concerned that you're rewriting list-manifest opposed
>> to simply using what's already written. The only bug I see
>> affecting list-manifest is pretty trivial:
>> 4298 installadm list -n should give different output if there is
>> no custom manifest
>> Further, I think this bug:
>> 4646 list: showing added manifest for a non running service.
>> Would probably better be:
>> 4646 list: Should provide warning manifests listed are for a
>> non-running service
>> (as we want to allow publication/deletion/listing of manifests
>> when the service is offline since there's no requirement for the
>> service to be online and this is how one can setup a service to
>> "go live" without affecting other things)
>
> Don't recall why I decided not to mess with list-manifest. I'll
> see if I can find my notes.
>
>> *I think you should take bug 4320 - installadm list -n should use
>> the -c option as default. And you should actually take ownership
>> (and mark as FIXINPROGRESS 4298 - installadm list -n should give
>> different output if there is no custom manifest.
>
> Defect 4320 does not match the design specification for list.
> I'll talk to Ethan about it. The design specification is located
> at:
>
>
> http://hub.opensolaris.org/bin/download/Project+caiman/auto_install/DesigndocdeltaforAISpring2009.2.pdf
>
>
>
> Defect 4298 should report an error if installadm list -m -n service_name
> does not have a custom manifest. This is currently what the list
> subcommand does.
>
>> *Do you have any example output as I can't visualize what this
>> looks like? (Also, have you run the output past Frank as he'd
>> given feedback on list-manifest?)
>
> OK. See the design specification for more detailed output:
>
> Manifest Criteria
> -------- --------
> devpublisher.xml arch = i86pc
> mac = 01:C2:52:E6:4B:E0
> ipv4 = 010.000.002.015
> devpublisher3.xml arch = i86pc
> mac = 01:C2:52:E6:4B:E6 - 01:C2:52:E6:4B:E9
> devpublisher4.xml arch = i86pc
> ipv4 = 192.168.168.151
> devpublisher5.xml arch = i86pc
> ipv4 = 192.168.168.251
> mac = 01:C4:52:E6:4B:E6 - 01:C4:52:E6:4B:E9
> devpublisher6.xml arch = i86pc
> ipv4 = 192.168.168.012
> mac = 01:C4:51:E6:4B:E6 - 01:C4:51:E6:4B:E9
> mem = 2048 MB
> somethingelse.xml arch = i86pc
> mac = 01:C2:E2:E6:4B:E9
>
>
>> Code bits:
>> list.py:
>> line 189 find_sparc_clients
>> line 337 find_x86_clients
>> I like the layout of these functions, however, couldn't
>> this data be rolled up into a x86_client and sparc_client
>> class to allow caching and easier reuse across all AI
>> components? (This would be very useful for the delete*.py
>> bits)
>
> Hmm... OK. This will take me a little while longer. May not make
> tomorrow's new webrev.
>
>> line 364 get_menu_info
>> Please use the GrubMenu class in installadm_common, unless
>> I'm missing what's special here?
>
> Already saw that one when you asked about moving findTFTPboot() into
> installadm_common.py and have a working list using it.
>
>> line 311 (and elsewhere)
>> Instead of concatenating two paths with a '/' please use
>> os.path.join().
>
> Will do.
>
>> line 569
>> "no local service\n" or "no local services\n"? Also (in
>> other places too), please follow PEP8's recommendation to
>> leave operators on the upper line when a line wrap is
>> encountered. And lastly I think you should be lined up
>> with the underscore from above. I think the lines would
>> be:
>> estr = _('%s: error: no local services\n') %\
>> os.path.basename(sys.argv[0])
>
> Ah, OK.
>
>> line 628
>> You're doing an assignment here so rdict and second are
>> the same object:
>> >>> def c(foo):
>> ... b=foo
>> ... del(b[1])
>> ...
>> >>> a
>> {1: 1, 2: 2, 3: 3}
>> >>> c(a)
>> >>> a
>> {2: 2, 3: 3}
>> You might want to see:
>> http://docs.python.org/library/copy.html
>
> Ah. I see. OK will do.
>
>> line 633: this line assumes that rdict has list objects as its
>> values. Otherwise, one will get an AttributeError likely. If
>> you're assuming that each dictionary only contains lists it could be
>> written for quicker execution, I believe, as:
>> >>> a={1: ['a'], 2: [2], 3: [3]}
>> >>> b={1: ['b'], 'c': [4]}
>> >>> def merge(foo, bar):
>> ... c={}
>> ... c.update(foo)
>> ... c.update(bar)
>> ... for key in set(foo.keys()) & set(bar.keys()):
>> ... c[key]=foo[key]+bar[key]
>> ... return c
>> >>> merge(a,b)
>> >>> {1: ['a', 'b'], 2: [2], 3: [3], 'c': [4]}
>
> OK
>
>> line 809: get_criteria_info:
>> Please use more descriptive variable names, I can't parse
>> what's going on here
>
> Changed criteria to mancriteria, crit to key, tk to tkey, tv dropped
> using svalue. Also modified the function as MAX/MIN code was
> essentially the same. See below:
>
> def get_criteria_info(mancriteria):
> """
> Iterates over the criteria which consists of a dictionary with
> possibly arch, min memory, max memory, min ipv4, max ipv4, min mac,
> max mac, cpu, platform, min network and max network converting it
> into a dictionary with on arch, mem, ipv4, mac, cpu, platform,
> network. Any min/max attributes are stored as a range within the
> new dictionary.
>
> Args
> criteria = dictionary of the criteria
>
> Returns
> dictionary of combinded min/max and other criteria
>
> Raises
> None
> """
>
> tdict = {'arch':'', 'mem':'', 'ipv4':'', 'mac':'',
> 'platform':'', 'network':'', 'cpu':''}
>
> for key in mancriteria.keys():
> if not mancriteria[key] or mancriteria[key] == '':
> continue # no criteria for instance key
>
> svalue = AIdb.formatValue(key, mancriteria[key])
>
> if key.find('MAX') == 0 or key.find('MIN') == 0:
> tkey = key[3:] # strip off the MAX or MIN for a new keyname
> if tdict[tkey] != '': # dealing with range
> if tdict[tkey] != svalue:
> if max(svalue, tdict[tkey]) == svalue:
> tdict[tkey] = tdict[tkey]+' - '+svalue
> else:
> tdict[tkey] = svalue+' - '+tdict[tkey]
> else: # first value, not range yet
> tdict[tkey] = svalue
> else: # not a range manifest criteria
> tdict[key] = svalue
>
> return tdict
>
> Hopefully, these changes help.
>
>> line 891: ensure
>
> Several corrected.
>
>>
>> Thank you,
>> Clay
>>
>>
>> On Tue, 1 Dec 2009, John Fischer wrote:
>>
>>> All,
>>>
>>> Updated webrev for this set of changes at:
>>>
>>> http://cr.opensolaris.org/~johnfisc/list
>>>
>>> These changes include feedback during a phone conversation from Ethan
>>> as well as the installadm man page change to document the list
>>> subcommand.
>>>
>>> Thanks,
>>>
>>> John
>>>
>>> John Fischer wrote:
>>>> Ethan, Clay and Sundar,
>>>>
>>>> When you get a chance can you review my webrev at:
>>>>
>>>> http://cr.opensolaris.org/~johnfisc/list
>>>>
>>>> These changes are for the installadm list subcommand fix and
>>>> will address:
>>>>
>>>> 4330 - installadm list should show which server provide the install
>>>> services.
>>>> 4113 - nice to have an option to show what service the client is
>>>> using
>>>> 4298 - installadm list -n should give different output if there is
>>>> no custom manifest
>>>> 4597 - installadm list: expect usage statement when giving service
>>>> name without "-n" flag.
>>>> 4646 - list: showing added manifest for a non running service.
>>>> 5300 - list: does not show informational message for non running
>>>> service.
>>>> 8496 - list: no verbiage indicating that a service does not exist
>>>> when a non-existent service is given.
>>>> 8529 - 'installadm list' command lists same service three times
>>>> 6811 - list: should have similar output between list and list -n
>>>> 8015 - list-manifests output is hard to read
>>>> 9094 - installadm list: prints colons for empty MAC fields, and
>>>> doesn't account for colons or periods in MAC field's width
>>>> 4175 - install list error slips out
>>>>
>>>> The changed and added files are:
>>>>
>>>> usr/src/cmd/installadm/Makefile
>>>> usr/src/pkgdefs/SUNWinstalladm-tools/prototype_com
>>>> usr/src/cmd/installadm/installadm.c
>>>> usr/src/cmd/installadm/installadm.h
>>>> usr/src/cmd/installadm/list.py
>>>>
>>>> These changes also include a change to the Makefile and prototype_com
>>>> file for the delete_service and delete_client to be consistent with
>>>> the other python scripts within the /usr/lib/installadm directory.
>>>>
>>>> The remote service listing is being postponed due to a timing issue
>>>> with multiple remote services that grows with the number of services
>>>> within the domain.
>>>>
>>>> Thanks,
>>>>
>>>> John
>>>> _______________________________________________
>>>> caiman-discuss mailing list
>>>> caiman-discuss at opensolaris.org
>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>> _______________________________________________
>>> caiman-discuss mailing list
>>> caiman-discuss at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
