Hi John,
I feel that we haven't been able to address my concerns but I've
been asked to table my concerns at this time.
Reviewing the changes to delete_service.py and
installadm_common.py it appears that all's well there.
Thank you,
Clay
On Tue, 15 Dec 2009, John Fischer wrote:
> 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
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
