Project

General

Profile

Bug #5052

Frontend config <attr type="expr" ...> issue

Added by Parag Mhashilkar over 6 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Target version:
Start date:
12/06/2013
Due date:
% Done:

90%

Estimated time:
First Occurred:
Occurs In:
Stakeholders:
Duration:

Description

As per the frontend docs, type expr is a condor constant or condor expression

type: Type of the attribute. Supported types are 'int', 'string' and 'expr'. Type expr is equivalent to condor constant/expression in condor_vars.lst

But when used with following example, it looks like code tries to do an eval on this expression. This does not work since the value is condor expression.

 <attr name="CLAIM_WORKLIFE" glidein_publish="True" job_publish="False" parameter="True" type="expr" value="ifThenElse(DynamicSlot =?= True, 600, -1)"/>

Above example results in frontend group crashing with following exception

[2013-12-06 09:25:25,453] INFO: Checking groups ['main']
[2013-12-06 09:25:25,454] WARNING: [<subprocess.Popen object at 0xd88d90>]: Traceback (most recent call last):
  File "/opt/glideinwms/frontend/glideinFrontendElement.py", line 1060, in <module>
    main(int(sys.argv[1]), sys.argv[2], sys.argv[3])
  File "/opt/glideinwms/frontend/glideinFrontendElement.py", line 989, in main
    paramsDescript = glideinFrontendConfig.ParamsDescript(work_dir, group_name)
  File "/local/home/factory/master/glideinwms/frontend/glideinFrontendConfig.py", line 168, in __init__
    self.expr_objs[k]=compile(val,"<string>","eval")
  File "<string>", line 1
    ifThenElse(DynamicSlot =?= True, 600, -1)
                            ^
SyntaxError: invalid syntax

[2013-12-06 09:25:25,454] WARNING: Child main exited. Checking if it should be restarted.

So either this is a bug in the code or the docs are wrong and needs to be investigated.

History

#1 Updated by Parag Mhashilkar almost 5 years ago

  • Assignee set to Marco Mambelli
  • Target version changed from v3_2_x to v3_2_13

#2 Updated by Parag Mhashilkar over 4 years ago

  • Assignee changed from Marco Mambelli to HyunWoo Kim

#3 Updated by HyunWoo Kim over 4 years ago

  • % Done changed from 0 to 70

I think I now understand how to resolve this ticket.
Here is what I did this morning:

I modified /usr/lib/python2.6/site-packages/glideinwms/frontend/glideinFrontendConfig.py

class ParamsDescript(JoinConfigFile):
    def __init__(self,base_dir,group_name):

        for k in self.data.keys():
            type_str,val=self.data[k]
            if type_str=='EXPR':
#HK5052         self.expr_objs[k]=compile(val,"<string>","eval")
                self.expr_objs[k]=val #HK5052 

I modified /usr/sbin/glideinFrontendElement.py

            glidein_params = copy.deepcopy(self.paramsDescript.const_data)
            for k in self.paramsDescript.expr_data.keys():
                kexpr = self.paramsDescript.expr_objs[k]
#HK5052         glidein_params[k] = glideinFrontendLib.evalParamExpr(kexpr, self.paramsDescript.const_data, glidein_el)
                glidein_params[k] = kexpr #HK5052   

And then modified /etc/gwms-frontend/frontend.xml

<name="CLAIM_WORKLIFE"          glidein_publish="True"  job_publish="True" parameter="True"  type="expr"   value="ifThenElse(DynamicSlot =?= True, 600, -1)" />

[root@hepcloud-devfe hyunwoo]# service gwms-frontend reconfig

[hyunwoo@hepcloud-devfe ~]$ condor_submit condor-test.sub

So, two requirements have been fulfilled:
1. In the factory

[root@hepcloud-devfac hyunwoo]# condor_status  -any -l  -constraint 'MyType=="glideclient"' | grep CLAIM
GlideinParamCLAIM_WORKLIFE = "ifThenElse(DynamicSlot =?= True, 600, -1)" 

2. In the glidein

[root@ec2-52-88-150-230 glide_pBEdz2]# grep -r WORKLIFE *
glidein_config                               : CLAIM_WORKLIFE   ifThenElse(DynamicSlot =?= True, 600, -1)
main/condor_vars.lst                         : GLIDEIN_CLAIM_WORKLIFE_DYNAMIC  I         3600           +       N       N       -
main/condor_vars.lst                         : GLIDEIN_CLAIM_WORKLIFE          I         -1             +       N       N       -
main/condor_config.dedicated_starter.include : CLAIM_WORKLIFE = ifThenElse(DynamicSlot =?= True, $(GLIDEIN_CLAIM_WORKLIFE_DYNAMIC), $(GLIDEIN_CLAIM_WORKLIFE))
main/condor_config.multi_schedd.include      : CLAIM_WORKLIFE = ifThenElse(DynamicSlot =?= True, $(GLIDEIN_CLAIM_WORKLIFE_DYNAMIC), $(GLIDEIN_CLAIM_WORKLIFE))

#4 Updated by HyunWoo Kim over 4 years ago

  • Status changed from New to Feedback
  • Assignee changed from HyunWoo Kim to Marco Mambelli

There are just 2 changes:
1. glideinFrontendConfig.py
self.expr_objs[k]=val #HK to address ticket 5052 compile(val,"","eval")
2. glideinFrontendElement.py
glidein_params[k] = kexpr #HK to address ticket 5052. glideinFrontendLib.evalParamExpr(kexpr, self.paramsDescript.const_data, glidein_el)

I tested this changes and confirmed that glideclient shows a new attribute that I added in frontned.xml with type=eval
and also that this new attribute appears in the startd (see my previous comment above)

#5 Updated by Marco Mambelli over 4 years ago

  • Status changed from Feedback to Assigned
  • Assignee changed from Marco Mambelli to HyunWoo Kim

I have two feedbacks.
1. there is no need for comments like "#HK to address ticket 5052. glideinFrontendLib.evalParamExpr(kexpr, self.paramsDescript.const_data, glidein_el)”

Your name, old code and the ticket # (you can add it in the commit comment) can be retrieved form git and they add little information and make the code less readable. You should remove them.

2. The problem is bigger than the one solved:
In the configuration documentation (attar section description):
type is "Type of the attribute. Supported types are 'int', 'string' and 'expr'. Typeexpr is equivalent to condor constant/expression in condor_vars.lst”

But the type attribute is used in the code in 2 different ways:
- for parameters (e.g. /var/lib/gwms-frontend/vofrontend/params.cfg file) it defines CONST/EXPR
CONST (anything not “expr") means that the parameter is a constant, EXPR (“expr”)means that the parameter is a python expression evaluated each time it is used
- for condor variables it means that the variable for condor is a integer/I “int” or a quoted string/S “string” or a condor expression/C (unquoted string) “expr”
An attribute could be both a parameter and a condor variable, so the value “expr” has 2 conflicting meanings.

The solution proposed patches partially the problem but:
- it leaves the CONST/EXPR distinction, the multiple data structures to support it
- even if it treats both branches in a similar way removing the compilation

I would avoid that and go one of the following routes:
1. remove completely CONST/EXPR distinction, e.g. removing the paramsDescript.expr_data dictionary, ...
2. (my favorite) change the parsing of attr and leave the functionality in place:
2a. in creation/lib/cvWParamDict.py, function add_attr_unparsed_real
change:
- is_expr=(attr_obj.type=="expr")
+ is_expr = False # attr_obj.type=="expr” is now used for HTCondor expression in condor variables
2b. add a try/except block around the compile instruction (in frontend/glideinFrontendConfig.py) to catch SyntaxError and say in the log file that there is an error in the attribute, without crashing

Solution 2 would allow in the future to add a new keyword. e.g. “pyexpr” to have again evaluated python expressions if needed.

In short:
- I would revert the changes (start a new clean branch v3/5052_2)
- I would do the changes for solution 2suggested above

I can di that if you prefer and send it back to you for feedback

#6 Updated by HyunWoo Kim over 4 years ago

I talked with Marco on Friday and modified the code as he suggested.
I then created a new branch 5052_2.
I will merge it into branch v3_2

#7 Updated by HyunWoo Kim over 4 years ago

  • % Done changed from 70 to 90

merged.

#8 Updated by HyunWoo Kim over 4 years ago

  • Status changed from Assigned to Closed


Also available in: Atom PDF