Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

nautilus: qa: fix various py3 cephfs qa bugs x2 #34470

Merged
merged 2 commits into from May 11, 2020
Merged

nautilus: qa: fix various py3 cephfs qa bugs x2 #34470

merged 2 commits into from May 11, 2020

Conversation

Vicente-Cheng
Copy link
Contributor

backport tracker: https://tracker.ceph.com/issues/44655

backport of #32533
parent tracker: https://tracker.ceph.com/issues/43515

this backport was staged using ceph-backport.sh version 15.1.1.389
find the latest version at https://github.com/ceph/ceph/blob/master/src/script/ceph-backport.sh

@tchaikov
Copy link
Contributor

@Vicente-Cheng could you please redo the backport? as i think it should only contain a single commit from #32533 .

@Vicente-Cheng
Copy link
Contributor Author

@tchaikov , already rebased.
please check again.

@Vicente-Cheng
Copy link
Contributor Author

jenkins retest this please

@tchaikov
Copy link
Contributor

@Vicente-Cheng thanks for looking into the test failure, i think it's a known issue. see https://tracker.ceph.com/issues/44683

@jdurgin jdurgin changed the base branch from nautilus to nautilus-saved April 15, 2020 17:30
@jdurgin jdurgin changed the base branch from nautilus-saved to nautilus April 15, 2020 17:30
@smithfarm smithfarm added this to the nautilus milestone Apr 21, 2020
@smithfarm smithfarm requested a review from kshtsk April 21, 2020 08:46
@smithfarm smithfarm added nautilus-batch-1 nautilus point releases needs-qa labels Apr 21, 2020
kshtsk
kshtsk approved these changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@kshtsk kshtsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@yuriw
Copy link
Contributor

yuriw commented Apr 22, 2020

wip-yuri-testing-2020-04-22-1449-nautilus

@neha-ojha neha-ojha added the cephfs Ceph File System label Apr 24, 2020
@neha-ojha
Copy link
Member

#34563 (comment) applies here too.

@yuriw
Copy link
Contributor

yuriw commented May 1, 2020

wip-yuri2-testing-2020-05-01-1612-nautilus

ajarr
ajarr reviewed May 4, 2020
View reviewed changes
qa/tasks/cephfs/test_journal_migration.py
@@ -78,7 +78,7 @@ def test_journal_migration(self):
"--path", "/tmp/journal.json"], 0)
p = self.fs.tool_remote.run(
args=[
"python",
"python3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this change. Isn't nautilus still tested with python2?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has caused failures as expected,
http://pulpito.ceph.com/yuriw-2020-05-01_18:26:19-fs-wip-yuri2-testing-2020-05-01-1612-nautilus-distro-basic-smithi/5006837/

Copy link
Contributor

@kshtsk kshtsk May 4, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not required, the reason of failure from the log above is that the script is python2 but python is python3, so there is a syntax error. Instead this should be rewritten so the print uses parenthesis, also I would recommend to use remote.sh to avoid extra StringIO handling as follows:

event_count_str = self.fs.tool_remote.sh('''
    python -c "import json; print(len(json.load(open('/tmp/journal.json'))))"
''').strip()
event_count = int(event_count_str)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only manually backport the part of 7e87f80 for this backport pr.
That should be better for this backport pr.
The suggestions of @kshtsk say(use remote.sh to avoid extra StringIO) I think that should be another backport pr?
This backport pr should only for py3 in cephfs qa bugs.

ajarr
ajarr requested changes May 4, 2020
View reviewed changes
Copy link
Contributor

@ajarr ajarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See, https://github.com/ceph/ceph/pull/34470/files#r419534422

@Vicente-Cheng
Copy link
Contributor Author

Vicente-Cheng commented May 4, 2020 •
edited

Hi @ajarr,
It looks like nautilus still tested with python2.
If so, does the py3 related fixed would not backport to nautilus?
Or hold the py3 realted fixed and use python2 for test?

kshtsk
kshtsk previously requested changes May 4, 2020
View reviewed changes
Copy link
Contributor

@kshtsk kshtsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment in regards to event_count calculation.

@kshtsk
Copy link
Contributor

kshtsk commented May 4, 2020

Hi @ajarr,
It looks like nautilus still tested with python2.
If so, does the py3 related fixed would not backport to nautilus?
Or hold the py3 realted fixed and use python2 for test?

@Vicente-Cheng at the moment all py3 related fixes in master are supposed to be py2 backward compatible, that is why they might and must be backported, however.

@Vicente-Cheng
Copy link
Contributor Author

Vicente-Cheng commented May 5, 2020 •
edited

Hi @kshtsk,
I check the fix about py3 fix for print in test_journal_migration.py.
That fixes is on another backport pr #34171.
I am not sure that could I pick this py3 fix (for print syntax error) here instead #34171?

Maybe I pick this fix and #34171 would drop this fix from batch commits?

Update: I manually backport the part of 7e87f80 like above says. If you have other suggestions please tell me : ) thanks

@batrick @Vicente-Cheng
qa: fix various py3 cephfs qa bugs
c8eafea 
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
(cherry picked from commit 6ea2730)

Conflicts:
	qa/tasks/cephfs/test_pool_perm.py
	  - manually modify the py3 compatibility related fix
@batrick @Vicente-Cheng
qa: fix various py3 cephfs qa bugs x2
c61236a 
Fixes: https://tracker.ceph.com/issues/43515
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
(cherry picked from commit ae28ed1)
@tchaikov
Copy link
Contributor

tchaikov commented May 6, 2020

Hi @kshtsk,
I check the fix about py3 fix for print in test_journal_migration.py.
That fixes is on another backport pr #34171.
I am not sure that could I pick this py3 fix (for print syntax error) here instead #34171?

Maybe I pick this fix and #34171 would drop this fix from batch commits?

Update: I manually backport the part of 7e87f80 like above says. If you have other suggestions please tell me : ) thanks

@Vicente-Cheng if your backport is merged before #34171, surely i will drop the corresponding commits in that PR. the whole point of #34171 is 1) to collect all python3 compatibility changes for nautilus 2) to test them.

@yuriw
Copy link
Contributor

yuriw commented May 6, 2020

wip-yuri3-testing-2020-05-06-1934-nautilus

@ajarr ajarr dismissed kshtsk’s stale review May 11, 2020 09:31

the issue has been fixed

@yuriw yuriw merged commit a6653c4 into ceph:nautilus May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

@ajarr ajarr ajarr approved these changes

@kshtsk kshtsk kshtsk left review comments

Assignees
No one assigned
Labels
cephfs Ceph File System nautilus-batch-1 nautilus point releases needs-qa tests wip-yuri3-testing
Projects
None yet
Milestone
nautilus
8 participants
@Vicente-Cheng @tchaikov @yuriw @neha-ojha @kshtsk @ajarr @smithfarm @batrick