PTL’s manager() call with expect=True doesn’t work as expected. It will set the requested attributes, but it won’t make sure they are set to the correct thing. It will call expect(), but it does with op=SET. This will make sure the attributes are set, but not necessarily set to the correct thing.
My first thought is to change op=SET to op=EQ. This would work for everything but booleans. You can set a boolean to ‘t’ or ‘f’ or ‘1’ and it will correctly be set to ‘True’ or ‘False’. This means when we do our expect() on the boolean, we’d be looking for the value of ‘t’ and get the value of ‘True’. We can’t just assume any value of ‘t’, ‘1’, or 1 is a boolean because 1 is a valid value for a numeric attribute. You can set strings to ‘t’.
The thing is how do we determine if an attribute is a boolean or not. I see several ways each with their own drawbacks
Do a status() on the object first and look for attributes that have the values of ‘True’ or ‘False’. The downsides here is it will have a false positive for string attributes which just happen to be set to ‘True’ or ‘False’. This also has the overhead of an extra status() call. I guess we don’t have to call expect(). The real purpose of expect is to check several times if an attribute has the correct value. Once we call qmgr, the values are going to be correctly set or not. We could just check the values in the return of that status() call. This means we’re reimplementing some of expect() in manager().
PBS has all of its attributes and builtin resources in XML files as part of its source tree. There is nothing saying that PTL can’t parse those XML files to find the attribute and resource types. The downside here is we’d then have more of a dependence on the PBS source tree and we’d have to query the types of custom resources by doing a status() on the resources.
Keep a separate list of boolean attribute and resources in PTL for this purpose. This is just horrible. Keeping separate lists will immediately go stale the instant someone adds a new boolean attribute. This also still doesn’t give us the custom resources. Those will still need to be queried.
Can anyone else think of a better way to do this?
I’m not sure what the right answer is. They all have downsides. Out of the 3 options I came up with, I’d lean towards 1.
I think I like option 2 as it’s the most reliable. To speed things up, maybe PTL can read the XMLs and get information about all standard attributes and resources at the beginning (as part of setUp or even setUpClass?), then it can just keep the cache of attributes and resources up to date if the test adds more resources.
Option 1 doesn’t handle non-default attributes which are unset (as status() won’t return them) and option 3, I totally agree, is horrible.
Hey @bhroam, as I am thinking more I am leaning towards option 2.
overhead of extra status call is in both 1 and 2 I guess. for option 1 I do share the concern that Ravi has for unset attributes.
Option 2 requires part of the pbs source to be available to run PTL. This is easy for developers testing on their own machines, but if you try and test on the testbed, you would either have to put the source code somewhere on the machine or package PTL with the xml files.
@agrawalravi90@anamika, I understand your worry about option 1, but I don’t think it will happen. You are correct that status() will only return attributes that are set. We will be calling it after we call manager(). This means either the attributes will be set, or if they aren’t set, we should fail.
If we’re worried about the extra status() call, I think we can fix this. It will require a little bit of expect() to be reimplemented. I usually would think this is a horrible idea, but I don’t think it’ll be that much code. With expect(), you want to stat multiple times and wait until something is set. For us, we know it’ll either be set or not immediately. This means the call to status() we are using to check for types can also be used to check if the attributes are correctly. We’ll just have to do a small loop with some assert calls.
This has the added benefit of not having to worry about custom resources. In option 2, we’d still have to query the custom resources differently than the XML files (and keep the cache up to date like @agrawalravi90 said). In option 1, we will have just set the custom resources, so we’ll see them in the status() call.
The main reason I dislike option 2 is the reason @vstumpf mentioned. We will have a hard dependency on something in the PBS source tree. We’ll have to install them somewhere (/usr/lib/python?) so they are available for PTLs use. If we already had such a dependency, I’d think option 2 was the way to go. I just think we should try and do something else before we add such a dependency.
Out of the three options mentioned, second one seems more doable but it will make us package xml files with PTL framework or make them available for PTL to read in some way.
Can I throw in a 4th option where in we ask users to provide the type of the attribute while passing them in expect? By default we can consider everything as string. This may cause us to make changes to a bunch of tests but eventually, people will learn how to use this option.
Something like this
expect({‘Scheduling’: (bool, False)})
Maybe I didn’t understand option 1 then. I thought in option 1 we will first do a status() BEFORE setting the attribute, check if the value is True/False, that will tell us that it’s a boolean, then we will set it and do a status again and confirm that the value was correctly set. In this scenario, for attributes which are already unset, the first status() will not be useful. So, my understanding must be wrong, can you please explain option 1 in more detail?
actually this is not what I understood from option 1. I do get that status will be called after set/unset/create operations. @bhroam in case of set/create you validate that attribute is set. But what about the case of unset/delete as some attributes are unset and some are reset?
Well in most cases we don’t have to pass this information (although it will be better if we do). Users need to do this only in case of booleans and rest of it PTL can treat as string.
@agrawalravi90 The status() call is done after the call to qmgr. This means the attributes would already be set. Option 1 would look something like:
call qmgr to set attributes
call status() to query attributes
if any attribute has the value of ‘True’ or ‘False’, it is a boolean. Match it against PTL_TRUE or PTL_FALSE (they have all the 1, 0, ‘t’, etc in them).
otherwise just match the value to what was set.
@anamika If I remember correctly from @agrawalravi90’s recent change, expect=True only works for create and set. It doesn’t work for unset or delete. I’m not 100% sure how we can fix it for unset since as you said, some values are unset and others are reset to their defaults.
@arungrover I’m not fond of your option 4. It puts the onus on the user to provide the information that PTL should really figure out for itself. If we were going to go down the route of changing what the user provides, I’d force them to always set booleans as ‘True’ or ‘False’, instead of one of the short cuts. I think people should write their tests this way anyway. It will be confusing when their test fails when they set a boolean to 1. The qmgr will succeed. They can see it set properly after the test fails, but their test did fail.
Thanks for the explanation, I see how it will work now, but I’m not sure if I like it. As you mentioned it yourself, it won’t work for attributes whose values are being set to ‘True’ or ‘False’ (probably rare, but since we are brainstorming a long term solution, i think we should make it resilient).
I also think that the approach with changing op to EQ instead of SET won’t be able to handle attributes like ‘queue_type’, for which a user might just pass ‘e’ as the value, even though we get ‘Execution’ when we do a status.
So, I think a more robust solution might be to parse the xmls, create an internal cache of attribute names and their “acceptable” values. I understand that we’ll need to package xmls with ptl, but is that really such a bad thing?
@agrawalravi90 I’m not sure I understand. If we don’t change the op to EQ, how will we tell if values are set properly? I thought the purpose of expect=True was to make sure the values were set properly after the manager() call. Just packaging the XML files with PTL will only give us the types. This will only help us with the boolean issue. It won’t help us with other attributes which can be shortened like queue_type.
Here’s another option. I’m not sure how much I like it. We can just change op=EQ. This will mean that if someone passed expect=True to manager(), they are saying what they are passing in will match in an expect(). If they use a short cut, it will fail and it will be on them. Passing expect=True is just a short hand for doing a manager() followed immediately by an expect(). They can’t do the expect with bool=1. We might consider undoing the recent change which turned expect=True on by default. It would be confusing to the test writer if they said bool=1 and received an expect exception. If they explicitly said expect=True, then they did it to themselves.
Sorry about the confusion Bhroam, I was not suggesting that we not change op to EQ, I was saying that if we ONLY do that, then expects will fail for attributes like queue_type.
The reason why we did expect=True by default was to remove potential race conditions in PTL tests, and there were loads of them, so we should try to keep it if possible and fix expect.
I think we should at least do option 1 and add special case handling for queue_type and other special attributes. Parsing the XML would have helped with booleans, but if that doesn’t sound like a good ROI, then let’s at least do option 1 and we can enhance things further after the support for create and unset is added. What do you think?
I was just going through the discussion. Can we make a test writer as decision maker here(not Framework) ?
If I set scheduling=t through commands manually I will expect scheduling=True should show in qstat -Bf.
and with that in mind why can’t we ask for end use to pass what(value) he is expecting after setting?
Default framework checks for EQ for the value it is setting and if user provides an input then expect that value.
I thought about that @vishwaks. I think it would lead to too much confusion. A test writer uses manager() to set a boolean to 1. The test fails, but afterwards they do a qstat and see the boolean set properly.
Your idea is definitely the most straight forward. We just set op=SET to op=EQ and fix any test that is somewhat lazy about setting their booleans. From that point forward, people will have to use manager() using the external form of the attributes since expect=True is on by default.