Skip to content

nautilus: qa/tasks/mgr/test_progress: fix wait_until_equal#39397

Merged
yuriw merged 2 commits intoceph:nautilusfrom
kamoltat:wip-nautilus-del-period-arg
Apr 12, 2021
Merged

nautilus: qa/tasks/mgr/test_progress: fix wait_until_equal#39397
yuriw merged 2 commits intoceph:nautilusfrom
kamoltat:wip-nautilus-del-period-arg

Conversation

@kamoltat
Copy link
Member

@kamoltat kamoltat commented Feb 10, 2021 •
edited
Loading

Fixes: https://tracker.ceph.com/issues/50006

Nautilus ceph_test_case doesn't have period arg
so remove that in wait_until_equal. Also, increase
time to wait for complete events by using RECOVERY_PERIOD
instead of EVENT_CREATION_PERIOD

Also, Nautilus is missing the commit:
b035379
This is a fix to this issue:
https://tracker.ceph.com/issues/40618

backporting the relevant commits from Octopus PRs:
#39360
backporting relevant commits from master PRs:
#30095

Fixes: https://tracker.ceph.com/issues/48824

Signed-off-by: Kamoltat ksirivad@redhat.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@kamoltat kamoltat added this to the nautilus milestone Feb 10, 2021
@kamoltat kamoltat requested a review from jdurgin February 10, 2021 15:10
@kamoltat kamoltat self-assigned this Feb 10, 2021
@kamoltat
Copy link
Member Author

kamoltat commented Feb 10, 2021

jenkins retest this please

@neha-ojha
Copy link
Member

neha-ojha commented Feb 11, 2021

@kamoltat you need to fix your signed-off-by format, it needs < email >

@kamoltat kamoltat force-pushed the wip-nautilus-del-period-arg branch from 564cc8f to 07dbce7 Compare February 12, 2021 10:00
@tchaikov tchaikov changed the title qa/tasks/mgr/test_progress: fix wait_until_equal nautilus: qa/tasks/mgr/test_progress: fix wait_until_equal Feb 18, 2021
tchaikov
tchaikov previously requested changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

@kamoltat you need to explain why this change is not cherry-picked from master in the commit message. otherwise please use git cherry-pick -x to cherry-pick the fix.

@kamoltat kamoltat force-pushed the wip-nautilus-del-period-arg branch from 07dbce7 to 84c32ec Compare February 22, 2021 13:16
@kamoltat
Copy link
Member Author

kamoltat commented Feb 22, 2021

jenkins test make check

@kamoltat
Copy link
Member Author

kamoltat commented Feb 22, 2021

jenkins test api

@kamoltat kamoltat requested a review from tchaikov February 22, 2021 15:37
smithfarm
smithfarm previously requested changes Feb 23, 2021
View reviewed changes
Copy link
Contributor

@smithfarm smithfarm left a comment

Choose a reason for hiding this comment

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

@kamoltat In my opinion, a better approach here would be to:

  1. wait until the Octopus fix #39360 is merged
  2. cherry-pick the Octopus fix to Nautilus

Would that be OK for you?

@smithfarm
Copy link
Contributor

smithfarm commented Feb 23, 2021 •
edited
Loading

@tchaikov @kamoltat We exhort folks to always "cherry-pick from master" (and, if that can't be done, explain why in the commit message). But there's a wrinkle to that. Actually, fixes should be cherry-picked from the most recent version in which they appear. Usually that's master. But in this case, it's octopus o.O

So, I suggest the fix be cherry-picked from octopus once it lands there.

@ideepika ideepika added needs-qa and removed needs-qa labels Mar 4, 2021
@kamoltat kamoltat force-pushed the wip-nautilus-del-period-arg branch from 84c32ec to ba0e839 Compare March 5, 2021 07:20
@ideepika ideepika requested a review from smithfarm March 9, 2021 14:26
@smithfarm smithfarm added DNM and removed needs-qa labels Mar 9, 2021
@smithfarm
Copy link
Contributor

smithfarm commented Mar 9, 2021

@ideepika This is still waiting for the octopus commit 8df54fe to be merged. See #39360

Is this fix needed for 14.2.17 QE? In that case, I suggest to merge #39360 and then merge this one.

@smithfarm
Copy link
Contributor

smithfarm commented Mar 9, 2021

The reason why #39360 should be merged first is because the commit in this PR is a cherry-pick of the commit in #39360. The cherry-pick should not be merged before the original!

@ideepika
Copy link
Member

ideepika commented Mar 9, 2021

The reason why #39360 should be merged first is because the commit in this PR is a cherry-pick of the commit in #39360. The cherry-pick should not be merged before the original!

Nope, should be fine to keep hold in this case.

@smithfarm smithfarm dismissed their stale review March 9, 2021 16:14

cherry-pick looks good now, but cherry-picked from SHA1 will need to be double-checked after #39360 is merged

@smithfarm smithfarm changed the title nautilus: qa/tasks/mgr/test_progress: fix wait_until_equal [DNM PENDING MERGE OF #39360] nautilus: qa/tasks/mgr/test_progress: fix wait_until_equal Mar 9, 2021
@smithfarm smithfarm dismissed tchaikov’s stale review March 9, 2021 16:16

this change request has been addressed

@kamoltat kamoltat force-pushed the wip-nautilus-del-period-arg branch from ba0e839 to 3ca913e Compare March 22, 2021 18:35
@neha-ojha neha-ojha changed the title [DNM PENDING MERGE OF #39360] nautilus: qa/tasks/mgr/test_progress: fix wait_until_equal nautilus: qa/tasks/mgr/test_progress: fix wait_until_equal Mar 24, 2021
@neha-ojha neha-ojha added needs-qa and removed DNM labels Mar 24, 2021
@smithfarm smithfarm added the nautilus-batch-1 nautilus point releases label Mar 26, 2021
@jdurgin jdurgin changed the base branch from nautilus to nautilus-saved March 31, 2021 00:15
@jdurgin jdurgin changed the base branch from nautilus-saved to nautilus March 31, 2021 00:15
@ideepika
Copy link
Member

ideepika commented Apr 1, 2021

@kamoltat can you take a look:

2021-03-31T20:19:54.931 INFO:tasks.cephfs_test_runner:ERROR: test_osd_came_back (tasks.mgr.test_progress.TestProgress)
2021-03-31T20:19:54.931 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2021-03-31T20:19:54.932 INFO:tasks.cephfs_test_runner:Traceback (most recent call last):
2021-03-31T20:19:54.932 INFO:tasks.cephfs_test_runner:  File "/home/teuthworker/src/github.com_ceph_ceph-c_d204ff17eed117bbc2af3bca37493c1443640102/qa/tasks/mgr/test_progress.py", line 278, in test_osd_came_back
2021-03-31T20:19:54.933 INFO:tasks.cephfs_test_runner:    timeout=self.RECOVERY_PERIOD)
2021-03-31T20:19:54.933 INFO:tasks.cephfs_test_runner:  File "/home/teuthworker/src/github.com_ceph_ceph-c_d204ff17eed117bbc2af3bca37493c1443640102/qa/tasks/ceph_test_case.py", line 192, in wait_until_true
2021-03-31T20:19:54.934 INFO:tasks.cephfs_test_runner:    if condition():
2021-03-31T20:19:54.935 INFO:tasks.cephfs_test_runner:  File "/home/teuthworker/src/github.com_ceph_ceph-c_d204ff17eed117bbc2af3bca37493c1443640102/qa/tasks/mgr/test_progress.py", line 277, in <lambda>
2021-03-31T20:19:54.935 INFO:tasks.cephfs_test_runner:    self.wait_until_true(lambda: self._is_complete(ev2['id']),
2021-03-31T20:19:54.936 INFO:tasks.cephfs_test_runner:TypeError: 'NoneType' object is not subscriptable
2021-03-31T20:19:54.936 INFO:tasks.cephfs_test_runner:
2021-03-31T20:19:54.937 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2021-03-31T20:19:54.937 INFO:tasks.cephfs_test_runner:Ran 1 test in 95.586s
2021-03-31T20:19:54.938 INFO:tasks.cephfs_test_runner:
2021-03-31T20:19:54.938 INFO:tasks.cephfs_test_runner:FAILED (errors=1)

http://qa-proxy.ceph.com/teuthology/yuriw-2021-03-31_19:52:34-rados-wip-yuri8-testing-2021-03-29-0949-nautilus-distro-basic-smithi/6013699/teuthology.log

@kamoltat kamoltat force-pushed the wip-nautilus-del-period-arg branch from 3ca913e to 13f7bc7 Compare April 5, 2021 20:12
@kamoltat
Copy link
Member Author

kamoltat commented Apr 5, 2021

qa/tasks/mgr/test_progress.py

So the problem is actually that it is missing if ev2 is not None: I don’t know why in 5ecd69099d2943e90de88fc7596497c2f86fab91
It it did not get added since all other versions (octopus - current master) has this. But my new push should help fix this.

@smithfarm
Copy link
Contributor

smithfarm commented Apr 5, 2021

Comparing this commit with the octopus commit is is supposed to have been cherry-picked from - df41ea7 - it seems like this commit is doing a lot more.

Would it make sense to look at git blame df41ea7467db3b40776030865896af0102129283 -- qa/tasks/mgr/test_progress.py and find out which commit(s) made those extra changes? And then cherry-pick those commits?

@kamoltat
Copy link
Member Author

kamoltat commented Apr 6, 2021

git blame

that makes sense, I'll try that thank you

rjfd and others added 2 commits April 6, 2021 19:27
@rjfd @kamoltat
qa/mgr/progress: fix timeout error when waiting for osd in event
fe8c141 
Fixes: https://tracker.ceph.com/issues/40618

Signed-off-by: Ricardo Dias <rdias@suse.com>
(cherry picked from commit b035379)

 Conflicts:
	qa/tasks/mgr/test_progress.py - trivial fix
@kamoltat
qa/tasks/mgr/test_progress: fix wait_until_equal
cb6a384 
Octopus ceph_test_case doesn't have period arg
so remove that in wait_until_equal. Also increase
time to wait for complete events by using RECOVERY_PERIOD
instead of EVENT_CREATION_PERIOD

Not needed in masters because only octopus and nautilus
doesn't have a period argument in qa/tasks/mgr/test_progress.py
wait_until_equals() function

Fixes: https://tracker.ceph.com/issues/48824

Signed-off-by: Kamoltat <ksirivad@redhat.com>
(cherry picked from commit df41ea7)

 Conflicts:
	qa/tasks/mgr/test_progress.py - trivial fix
@kamoltat kamoltat force-pushed the wip-nautilus-del-period-arg branch from 13f7bc7 to cb6a384 Compare April 6, 2021 19:36
ideepika
ideepika approved these changes Apr 9, 2021
View reviewed changes
Copy link
Member

@ideepika ideepika left a comment

Choose a reason for hiding this comment

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

tests looks good thanks @smithfarm @kamoltat !
https://trello.com/c/R6ClYBUf/1197-wip-yuri8-testing-2021-04-06-1934-nautilus-old-wip-yuri8-testing-2021-04-05-1412-nautilus-old-wip-yuri8-testing-2021-03-29-0949

@yuriw yuriw merged commit 21b8fb7 into ceph:nautilus Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ideepika ideepika ideepika approved these changes

@jdurgin jdurgin Awaiting requested review from jdurgin

@tchaikov tchaikov Awaiting requested review from tchaikov

@smithfarm smithfarm Awaiting requested review from smithfarm

Assignees

@kamoltat kamoltat

Labels

bug-fix core mgr nautilus-batch-1 nautilus point releases needs-qa wip-yuri8-testing

Projects

None yet

Milestone

nautilus

Development

Successfully merging this pull request may close these issues.

7 participants

@kamoltat @neha-ojha @smithfarm @ideepika @tchaikov @yuriw @rjfd