Dave, Sundar and Clay,
Just a reminder...
I have update the webrev for the installadm list subcommand fixes
for the issues that you brought up. The updated webrev is located
at:
http://cr.opensolaris.org/~johnfisc/list
Once I get your approval then I think that I am done. I am most
interested in Clay's issues as the class and criteria lists is some
contention.
Also I have updated my source tree via Mercurial merging Clay's changes
that went into installadm.c and delete_service.py.
If I can get closure before you go on break then that would be
great.
Thanks,
John
John Fischer wrote:
> 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
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
