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.

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

qa: refactor health-ok.sh, drop health-{mds,rgw}.sh #1233

Merged
merged 9 commits into from
Jul 27, 2018

Conversation

smithfarm
Copy link
Contributor

Now that we have the ceph.restart orchestration for testing the restart
functionality, we can simplify the integration testing scripts.

The actual triggering of the ceph.restart orchestration will be done
in the teuthology test yaml, which lives in the Ceph repo.

We retain health-nfs-ganesha.sh as a separate script for now, to
keep health-ok.sh as simple as possible for as long as possible.

@smithfarm smithfarm added the QA label Jul 24, 2018
@smithfarm smithfarm requested a review from jschmid1 July 24, 2018 14:36
@smithfarm smithfarm changed the title Wip qa new health ok qa: refactor health-ok.sh, drop health-{mds,rgw}.sh Jul 24, 2018
@smithfarm
Copy link
Contributor Author

Hm, currently the scripts don't know how to deploy mds without a client (non-storage) node present. Fixing.

@smithfarm
Copy link
Contributor Author

@jschmid1 Is the lint failure expected?

@smithfarm smithfarm added the DNM label Jul 24, 2018
@smithfarm
Copy link
Contributor Author

NOTE: setting DNM because this PR will break the teuthology suse/smoke suite, but otherwise I believe this PR is ready for review.

I'll prepare a downstream Ceph PR to update suse/smoke to work with this PR. Then we can merge both at once.

@smithfarm
Copy link
Contributor Author

smithfarm commented Jul 24, 2018

Testing update. On a single-node Salt cluster with 4 external disks, I ran:

health-ok.sh --mds --rgw
salt-run state.orch ceph.restart

All green.

UPDATE: Oops, wrong orchestration. Should be "ceph.smoketests".

@smithfarm
Copy link
Contributor Author

smithfarm commented Jul 24, 2018

@jschmid1 As you can see, I'm removing the ceph.restart testing from these scripts.

I have a Ceph branch: https://github.com/SUSE/ceph/tree/wip-qa-deepsea-func

I haven't opened a Ceph PR yet because that branch is WIP. But when it's ready it will have tests that reproduce the ceph-restart functional testing that is being removed from the health-ok, etc. scripts by this PR.

@jschmid1
Copy link
Contributor

@jschmid1 Is the lint failure expected?

I saw it failing initially for exactly what you are getting:

17:35:04 ************* Module srv.salt._modules.rgw
17:35:04 E: 22, 0: No name 's3' in module 'boto' (no-name-in-module)
17:35:04 E: 24, 0: No name 'exception' in module 'boto' (no-name-in-module)

those disappear on the next iteration ..

Copy link
Contributor

@jschmid1 jschmid1 left a comment

Choose a reason for hiding this comment

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

lgtm

@jschmid1
Copy link
Contributor

I have a Ceph branch: https://github.com/SUSE/tree/wip-qa-deepsea-func

I get a 404 on that one

@jschmid1
Copy link
Contributor

I'll prepare a downstream Ceph PR to update suse/smoke to work with this PR. Then we can merge both at once.

Let me know when to hit the merge button

@smithfarm
Copy link
Contributor Author

@jschmid1 Sorry, fixed that comment so it reads correctly:

I have a Ceph branch: https://github.com/SUSE/ceph/tree/wip-qa-deepsea-func

osd_restarted "0"
# make sure still in HEALTH_OK
ceph_cluster_status
ceph_health_test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jschmid1 Are you OK to drop this test of the restart functionality? Maybe the test should be re-implemented under ceph.smoketests before we really drop it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Getting rid of it now means that we have at least for a short period of time no restart tests, right?
In that case I'd keep them in for now and remove them after we implemented the restart.functests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed to drop because the tests really are re-implemented in ceph.smoketests and the CI will run that orchestration.

@smithfarm smithfarm force-pushed the wip-qa-new-health-ok branch from b8dd142 to df590ea Compare July 25, 2018 21:38
@smithfarm
Copy link
Contributor Author

@jschmid1
Copy link
Contributor

@smithfarm
Copy link
Contributor Author

@jschmid1 Yeah, sorry, I was originally under the impression that ceph.smoketests already includes the restart tests.

I will reinstate the restart tests (and "--mini") into health-ok.sh.

@jschmid1
Copy link
Contributor

@jschmid1 Yeah, sorry, I was originally under the impression that ceph.smoketests already includes the restart tests.

For master, we do. But do we already run those?

@smithfarm smithfarm force-pushed the wip-qa-new-health-ok branch 2 times, most recently from 5ef54ec to 4422808 Compare July 26, 2018 09:26
@smithfarm
Copy link
Contributor Author

@jschmid1 So, to recapitulate, this PR:

  1. drops health-mds.sh and health-rgw.sh
  2. drops the restart tests that were in health-ok.sh
  3. adds --mds and --rgw options to health-ok.sh, making it a more generic deployment script
  4. makes the final re-run of stage 0 in health-nfs-ganesha.sh unconditional

In parallel, there will be a SUSE/ceph PR that will

  1. add a test that uses health-ok.sh to deploy a cluster with rgw and mds roles, and runs ceph.smoketests on that cluster
  2. revamp the mds and rgw CI tests to work with this PR

Sound right to you?

Now that we have the ceph.restart orchestration for testing the restart
functionality, we can simplify the integration testing scripts.

The actual triggering of the ceph.restart orchestration will be done
in the teuthology test yaml, which lives in the Ceph repo.

Signed-off-by: Nathan Cutler <[email protected]>
This function could have produced a false positive if both the RPM and Ceph version strings
failed the regex match. (In that case, both the variables would have been empty and the
equality test might have passed.)

With this commit, the test explicity checks that the regex match actually produced a string
that can be usefully compared.

Signed-off-by: Nathan Cutler <[email protected]>
@smithfarm smithfarm force-pushed the wip-qa-new-health-ok branch from 4422808 to 4fc7e85 Compare July 26, 2018 10:05
smithfarm added a commit to SUSE/ceph that referenced this pull request Jul 26, 2018
This commit:

* adds suse/smoke/functional test case that triggers ceph.smoketests orchestration
* refactors the other test cases for compatibility with SUSE/DeepSea#1233

Signed-off-by: Nathan Cutler <[email protected]>
@smithfarm
Copy link
Contributor Author

smithfarm commented Jul 26, 2018

@jschmid1 Would it be OK to rename ceph.smoketests (and everything under it) to ceph.functests.1node as part of this PR? That would clear the way for implementing ceph.functests.2nodes . . .

This secret code will make it easier to navigate teuthology logs,
for tests that use health-ok.sh to deploy the cluster.

Just search for YYYY to get to the end of the deployment phase.

Signed-off-by: Nathan Cutler <[email protected]>
@jschmid1
Copy link
Contributor

@jschmid1 Would it be OK to rename ceph.smoketests (and everything under it) to ceph.functests.1node as part of this PR? That would clear the way for implementing ceph.functests.2nodes . . .

Yes, that'd be fine with me.

Currently everything is only required to have at least one node.
In the future the migration smoketests will require at least two

@smithfarm smithfarm requested a review from swiftgist July 26, 2018 12:52
@smithfarm
Copy link
Contributor Author

@jschmid1 OK, added smoketests rename commit - as a result, this PR now conflicts with #1216 so adding @swiftgist as reviewer.

@smithfarm
Copy link
Contributor Author

smithfarm commented Jul 27, 2018

@jschmid1 Latest test run http://137.74.25.20:8081/ubuntu-2018-07-26_22:31:13-suse:smoke-ses6---basic-openstack/

and especially the test that runs functests.1node: http://137.74.25.20/ubuntu-2018-07-26_22:31:13-suse:smoke-ses6---basic-openstack/76/teuthology.log - search for "Unwinding manager deepsea" and scroll up slowly from there :-)

The only failures are in the tests that run functests.1node.{migrate,replace} which are not expected to succeed yet. I also examined the successful jobs and they seem to be doing what I expected.

The related test yaml changes in the SUSE/ceph.git are already in the ses6 branch.

So 👍 to merge from my side.

@jschmid1 jschmid1 merged commit 21a252f into SUSE:master Jul 27, 2018
@smithfarm smithfarm deleted the wip-qa-new-health-ok branch July 27, 2018 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants