Fix manager() adding extra quotes #1122
Conversation
vstumpf
changed the title
|
I've made this WIP due to some tests failing. I think the best way to continue this fix is to change how string arrays are handled with PTL. Expect a design doc soon. |
vstumpf
force-pushed
the
bugs/manager-hotfix
branch
from
0e50de0
to
e433c5b
Compare
vstumpf
changed the title
vstumpf
force-pushed
the
bugs/manager-hotfix
branch
from
e433c5b
to
f197fe9
Compare
|
I've opened this up for review again. |
vstumpf
force-pushed
the
bugs/manager-hotfix
branch
from
f197fe9
to
bfbd496
Compare
vstumpf
force-pushed
the
bugs/manager-hotfix
branch
from
eedf2f9
to
2c8df46
Compare
| self.server.create_import_hook("test", a, hook_body, overwrite=True) | ||
| return |
There was a problem hiding this comment.
Why did you add this return? I don't think it should be here.
test/tests/functional/pbs_qmgr.py
Outdated
| (' -c "p n %s comment"' % self.mom.hostname) | ||
| ret = self.du.run_cmd(self.server.hostname, | ||
| (' -c "p n %s comment"' % self.mom.shortname) | ||
| ret = self.du.run_cmd(self.server.shortname, |
There was a problem hiding this comment.
Why are you changing this to self.server.shortname?
test/tests/selftest/pbs_manager.py
Outdated
| MGR_CMD_CREATE, RSC, attr, id=r, runas=ROOT_USER, logerr=False) | ||
| self.assertEqual(rc, 0) | ||
| self.server.manager(MGR_CMD_SET, SERVER, | ||
| {"resources_available.foo2": ["A'A", 'B"B', 'C C']} |
There was a problem hiding this comment.
Should there be a test for commas and newlines too?
There was a problem hiding this comment.
It's weird, even though the code is there in pbs, it doesn't support new lines or commas in string array values.
test/tests/selftest/pbs_manager.py
Outdated
| attr['flag'] = 'h' | ||
| r = 'foo2' | ||
| rc = self.server.manager( | ||
| MGR_CMD_CREATE, RSC, attr, id=r, runas=ROOT_USER, logerr=False) |
There was a problem hiding this comment.
You don't need to run this as ROOT_USER. PTL makes the user who runs pbs_benchpress a manager. That is sufficient.
test/tests/selftest/pbs_manager.py
Outdated
| """ | ||
| attr = {} | ||
| attr['type'] = 'string_array' | ||
| attr['flag'] = 'h' |
There was a problem hiding this comment.
Just curious, why use 3 lines when this should all fit on one?
test/tests/selftest/pbs_manager.py
Outdated
| nodes = self.server.filter(SERVER, | ||
| {'resources_available.foo2': | ||
| """A'A,B"B,C C"""}) | ||
| self.logger.info(nodes) |
There was a problem hiding this comment.
You're querying the server, nodes is a poor variable name. Also, don't log a blanket variable. Either turn it into a full log message, or maybe assert instead.
|
@vstumpf, Are the attached test results the latest ? Logs show failures(TestManager suite). Please attach the latest logs. |
|
Sorry, that was an old version of the test. I ran the new test without resaving the output. |
Thanks @vstumpf |
vstumpf
force-pushed
the
bugs/manager-hotfix
branch
from
4a2042c
to
868c17c
Compare
|
I've rebased, please take this forward @bhroam |
Describe Bug or Feature
A previous check-in #1089 introduced a bug where PTL would quote string values if it contained certain characters. This would fail with string arrays. It would also fail with tests that added quotes to the strings so qmgr could import it; it would double-quote some strings. This will cause tests that set Fairshare order to fail.
Describe Your Change
manager() will now also take lists of strings.
Design: https://pbspro.atlassian.net/wiki/spaces/PD/pages/1281458177/manager+should+use+python+lists+for+string+arrays
String Arrays
If any of the strings in the list have quotes or special characters, it will separately set each one, so qmgr does not get confused as to what is a string boundary and what is actually in the string.
Otherwise, it will join the string with commas and fall through to the strings case.
Strings
If the first and last character are both single or double quotes, don't add quotes.
Attach Test Logs or Output
after-fix-self.txtafter-fix8.txt
after-fix9.txt
after-fix10.txt