-
Notifications
You must be signed in to change notification settings - Fork 306
suite: derive ceph branch automatically when using --rerun #2091
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
base: main
Are you sure you want to change the base?
Conversation
teuthology/suite/__init__.py
Outdated
| if conf.ceph_branch is not None and conf.ceph_branch != ceph_branch: | ||
| log.warning('--ceph specified but does not match with ' | ||
| 'rerun --ceph branch: %s', | ||
| ceph_branch) | ||
| else: | ||
| log.info('Using rerun ceph branch=%s', ceph_branch) | ||
| conf.ceph_branch = ceph_branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is a bit tangled, also there is case when value of --ceph is overridden (though with the same value). Also I think you still want to always print the branch out, which is currently used, don't you?. So, maybe:
if conf.ceph_branch is None:
conf.ceph_branch = ceph_branch
else:
if conf.ceph_branch != ceph_branch:
log.warning(f"--ceph specified but does not match the rerun job's branch {ceph_branch}")
log.info(f"Using ceph branch={conf.ceph_branch}")
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kshtsk yes agreed! I had discussed this with Zack as well, will ping you once I address it :)
ee4a9f6 to
c08e30c
Compare
|
This is what a rerun would look like now: The main difference is that we are picking up the ceph branch and sha1 from the rerun configuration instead of defaulting to main : |
Using --rerun without --ceph currently defaults to the main branch. This change sources the branch from the original run being rerun, making reruns more intuitive. Warns if --ceph is specified and does not match the original branch. Fixes: https://tracker.ceph.com/issues/68872 Signed-off-by: Aishwarya Mathuria <amathuri@redhat.com>
c08e30c to
830fbdd
Compare
| The suite to schedule | ||
| --wait Block until the suite is finished | ||
| -c <ceph>, --ceph <ceph> The ceph branch to run against | ||
| [default: {default_ceph_branch}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please remind me why this line has been deleted? I thought, there should be just added extra comment, that in case using --rerun the branch is taken from the referred run instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default being applied in docopt meant that we couldn't differentiate between "user passed --ceph main" from "user did not pass --ceph at all"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had TEUTH_CEPH_BRANCH environment variable which default the --ceph when it is not provided. Still unsure if we want to keep this functionality.
zmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! @kshtsk, what do you think?
Using --rerun without --ceph currently defaults to the main branch. This change sources the branch from the original run being rerun, making reruns more intuitive. Warns if --ceph is specified and does not match the original branch.