Design for a supported way to change default setup in PTL

Developers
1

Hi All,

Please review the new design for a supported way to change default setup in PTL while running PBS tests posted at: https://pbspro.atlassian.net/wiki/spaces/PD/pages/557088789/Design+for+a+supported+way+to+change+default+setup+in+PTL

Please let me know of any comments or suggestions.

Thanks,
Sarita

2

@saritakh Thanks for working on this enhancement and posting the design document.

I have one comment -

  • It is important to know what exactly will be saved as part of save_configuration. Like, If I use this new option on a multi-mom setup then would it save both the moms and load both the moms after every test?
3

Thanks for the comment @arungrover,
I have updated the design page with list of configurations that will be saved in save_configuration() method. Yes, the configurations of moms listed in pbsnodes will also be saved before running every test.

4

Thanks for adding more information to the document @saritakh.

I guess you might want to mention that in case of multi-sched PTL will have sched_priv and the files in that directory for each scheduler. Can you please make that change.

Rest of it looks good to me.

5

Design looks good to me.

6

Thanks for proposing this change Sarita, I think it’ll be extremely helpful in testing out new features (and motivate test writers to write tests without relying on setUp()).

Just one clarification: it seems like this new option will make PTL ditch the
“out-of-box” configuration in favor of keeping the existing PBS configuration on the machine and reverting to it after each test. It wasn’t clear from the document that this is what the option will do, so my only comment is that, if possible, please mention this more explicitly in the doc.

7

Thank you @arungrover, @hirenvadalia and @agrawalravi90,

@arungrover,
I have updated the design page to list multiple scheduler directories if present.

@agrawalravi90,

I think the case you have mentioned is a slight variant of third flow from left to right in the flow chart but without S3. I found that flow missing and now added it.

‘it seems like this new option will make PTL ditch the
“out-of-box” configuration in favor of keeping the existing PBS configuration on the machine and reverting to it after each test.’
==>> This happens only when the option ‘–use-current-setup’ is specified; if not specified PTL setUp() will revert ot Out-Of-Box.

I have updated the design page with a table of specifications and behavior of setUp() and setUpClass() for each case. Also, included one more flow in the chart which now explains options 1 to 6 from the table specified.
I hope the table and the chart clarifies the whole set of changes to PTL.

@all,
Please let me know of any more comments or ambiguity in the document.

8

Thanks for providing all the details Sarita. However, I feel like the document could be simplified. How about dividing it up into 2 sections: external changes and internal details:
External changes:
a) new option “–use-current-setup”. For the description of this new option, I think you just need to mention that this will cause PTL to use the PBS configuration on the machine instead of the “out-of-box” configuration. If custom setUp()/setUpClass() are defined, they will take precedence.
b) PBSTestSuite.tearDownClass() will now revert things to either the “out-of-box” configuration, or the machine configuration before testing if “–use-current-setup” is provided.

Internal Changes:
a) setUpClass() will call save_configuration(), which setUp() will call to load the configuration for each test.
b) <list of all commands that save_configuration() will save>
c) The flow charts, which I think you need only 3: one with “out-of-box” setup, one with --use-current-setup, and one with custom setUp, setUpClass, tearDown & tearDownClass.

I found the table a bit confusing, so I suggest removing it.

Also, I have a question:
What if somebody overwrites setUpClass() method in their testsuite? With your proposed changes, setUp() will rely on setUpClass() to save the configuration that setUp will load, so how would it work if setUpClass is overwritten?

9

Hi @saritakh, thanks for the document. I have few comments:

A. setUpClass() & tearDownClass() method updates:

PTL will now revert PBS under test to the set of configurations that it started on at the end of every test run. This is done by saving the configurations at test suite's setUpClass() method and reverting to the same on test suite's tearDownClass() method.

–> Above is only true if “–use-current-setup” is passed. correct? Also reading the first line it sounded like PTL was not doing it at all. you might want to change that.

–> to make document more readable I will define the new option first and then mention about the updates to setup class/teardownclass and setup. Clearly setup class and setup changes are only applicable when new option is specified. Please mention this explicitly.

–> following doesn’t sound like a good idea to me. For example, what if the configurations are conflicting? I think this is what Ravi is asking above.
pbs_benchpress --use-current-setup User updated PBS - B Defines certain configurations C reverts to PBS - B + C reverts to B

–> load_configuration() will load these saved configurations if altered on the PBS cluster
just for my understanding, if only 2 files are updated then only those will get reverted. correct?

@agrawalravi90 I found the table more clear than the flow diagram. I think we can leave both there as flow diagram is covering the scenario from the table itself.

10

@saritakh under row 4 column “setup() action” why does it say that it reverts to out of box configuration for the first time. Should it not just revert to what is set in setupclass?

11

The default setUpClass() will save initial configuration and if the test suite has custom setUpClass() then user must explicitly call save_configuration before ‘return’ statement in custom setUpClass().
(ref design doc Note : point no 4)

12

Thanks for clarifying Kumar. Instead of creating an additional limitation, how about this:

  • The first instance of setUp() will create the correct configuration and call save_configuration(). It will also set a flag (say PBSTestSuite.config_saved = True).
  • Then, each subsequent instance of setUp() can check if the flag is set and call load_configuration(). Otherwise, it will do what’s necessary to create the configuration and call save_configuration() (and set the flag).

That way, setUp() will not depend on setUpClass(). What do you think?

13

What if user overrides setUp() in their test suites? Again user has take care of checking flags and saving configuration right?
So if user has to do some extra stuff which is one off per test suite then don’t you think it should go into setUpClass() instead setUp()? because setUp() is called per test case and setUpClass() is per suite.

14

If users override setUp(), then they do what they want :slight_smile: Their setUp will be run before every test, regardless of what it does, we don’t have to worry about that. We just need to make sure that when PBSTestSuite’s setUp/setUpClass/tearDown/tearDownClass can work independently of each other as any of them could be replaced by a custom function.

15

Thanks Anamika, Arun & Ravi for the inputs and suggestions,
We (@hirenvadalia, @kjakkali & @borlesanket) discussed Ravi’s proposal of using setUp() along with a flag and we are going ahead with it; I am working on the new flows and will update the same as soon as it is ready.

16

@All,

I have updated the design page with a new approach; wherein the functionality of methods are differing.
Please review the same and let me know of any comments/suggestions/confusions.

Thanks for waiting patiently for this and please find my replies to your previous comments in my following notes.

17

I would prefer keeping the design sections as is with some shuffling instead of segregating into internal and external design.

“a) new option “–use-current-setup”. For the description of this new option, I think you just need to mention that this will cause PTL to use the PBS configuration on the machine instead of the “out-of-box” configuration. If custom setUp()/setUpClass() are defined, they will take precedence.”
Sarita ==> The custom setUp()/setUpClass() will not take precedence, but will be applied over the saved configuration.

“Also, I have a question:
What if somebody overwrites setUpClass() method in their testsuite? With your proposed changes, setUp() will rely on setUpClass() to save the configuration that setUp will load, so how would it work if setUpClass is overwritten?”
Sarita ==> In the overwritten methods setUpClass(), setUp(), tearDown() & tearDownClass(), the test writer needs to take care of equivalent functionality which is the case right now as well.
Regarding, the dependencies, even now the methods are dependent on each other. For example: setUpClass() creates objects and setUp() uses the same objects.

Please re-review.

18

“–> Above is only true if “–use-current-setup” is passed. correct? Also reading the first line it sounded like PTL was not doing it at all. you might want to change that.”
Sarita ==> I have changed the wordings now. Also, PTL current does not revert to OOB at the end of the test. This is not related to --use-current-setup option, but it was lacking in PTL.

“–> to make document more readable I will define the new option first and then mention about the updates to setup class/teardownclass and setup. Clearly setup class and setup changes are only applicable when new option is specified. Please mention this explicitly.”
Sarita ==> I have shuffled the order of points mentioned.
Also, the entire changes are not applicable to ‘use-current-setup’. The functionality of reverting at the end of the test run and saving the configurations (test suite level) done in setUpClass() are not dependent on ‘use-current-setup’.

“–> following doesn’t sound like a good idea to me. For example, what if the configurations are conflicting? I think this is what Ravi is asking above.
pbs_benchpress --use-current-setup User updated PBS - B Defines certain configurations C reverts to PBS - B + C reverts to B”
Sarita ==> I have updated the table as per new flow. Also, the test writer is responsible for conflicting configurations. PTL just provides the way to do configurations.

“just for my understanding, if only 2 files are updated then only those will get reverted. correct?”
Sarita ==> Yes, if they are updated in the test, then they only will be reverted

Please re-review.

19

I removed that note and updated the table according to latest flow. I had added it with some confusion. Please review now.

20

@All,
Gentle reminder!!
Please review the latest updated design with below summarized list of changes to previous version:

  1. calls to revert_to_defaults() methods moved to setUpClass()
  2. save_configuration() method will be called internally at below:
    setUpClass() i.e. when use-current-setup option is present
    setUp() i.e. when feature test suite has certain configurations defined in custom setUpClass() method
  3. load_configuration() method will be called internally at below to revert PBS under test to respective states:
    tearDown()
    tearDownClass()
  4. a flag is used to conditionally save configuration the first time in setUp() method
  5. flow chart and the summary table are updated as per above changes