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

auto upgrade from n-1|n-2 releases #1934

Closed
wants to merge 1 commit into from
Closed

auto upgrade from n-1|n-2 releases #1934

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions teuthology/task/install/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from teuthology import contextutil, packaging
from teuthology.parallel import parallel
from teuthology.task import ansible
from teuthology.suite import util

from distutils.version import LooseVersion
from teuthology.task.install.util import (
Expand Down Expand Up @@ -215,6 +216,43 @@ def install(ctx, config):
'python-ceph']
rpms = ['ceph-fuse', 'librbd1', 'librados2', 'ceph-test', 'python-ceph']
package_list = dict(deb=debs, rpm=rpms)

source_branch = config.get('branch') # n-1 | n-2
if source_branch == 'n-1' or 'n-2':
import json
import subprocess
curlurl = 'curl -s https://chacra.ceph.com/repos/ceph-release/ | jq "keys"'
status, output = subprocess.getstatusoutput(curlurl)
Copy link
Member

Choose a reason for hiding this comment

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

This should not (or does not need to) be a dynamic part of the teuthology install task. We can construct the list of releases to upgrade from when creating the job matrix. Then it can be an explicit part of the job config / install task.

I recommend we table this for now until I can spend some time on the teuthology code to dynamically add dimensions to the matrix.

Copy link
Author

Choose a reason for hiding this comment

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

We can construct the list of releases to upgrade from when creating the job matrix

then would incur manual efforts which completely kills the purpose, the chacra API maintains the list, why not use it instead of constructing an additional one?

Copy link
Member

Choose a reason for hiding this comment

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

It is not manual effort. We're moving the dynamic release lookup to the matrix generation. The advantage there is we can construct a matrix that upgrades for multiple minor releases of n-1 or n-2, not just the current HEAD.

Copy link
Author

Choose a reason for hiding this comment

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

Ah I remember from the talk post-standup a few weeks back, so the idea is to move even one step forward than this and not just to limit to few major releases but rigorously carry out tests on multiple minor releases, makes sense but the code you highlighted to be not part of the teuthology code:

Then it can be an explicit part of the job config / install task.

would mean to get this into the YAMLs which would complicate the process, if there is some sort of abstraction i.e. to only have some placeholders in places in configs/install tasks like n_1_minor_releases would mean to fetch all the reef releases(just for instance keeping squid as ref point) and create the dynamic matrix you're referring to. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I remember from the talk post-standup a few weeks back, so the idea is to move even one step forward than this and not just to limit to few major releases but rigorously carry out tests on multiple minor releases, makes sense but the code you highlighted to be not part of the teuthology code:

Then it can be an explicit part of the job config / install task.

would mean to get this into the YAMLs which would complicate the process, if there is some sort of abstraction i.e. to only have some placeholders in places in configs/install tasks like n_1_minor_releases would mean to fetch all the reef releases(just for instance keeping squid as ref point) and create the dynamic matrix you're referring to. What do you think?

You don't need placeholders. Just use an override for the install: task config.

Copy link
Author

Choose a reason for hiding this comment

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

an override would have to simple like the ones we have right now like for instance:

overrides:
  ceph:
    conf:

doesn't that means we'd still have to get the code to get the dynamic thing done in the teuthology code itself? Otherwise how we code it explicitly? Lua in YAMLs?

Copy link
Member

Choose a reason for hiding this comment

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

doesn't that means we'd still have to get the code to get the dynamic thing done in the teuthology code itself?

teuthology-suite would adjust a directory like

upgrade/tasks/0-from/{install, {base/{squid,reef}}}

where install.yaml is something vanilla like

install: {}

and reef.yaml would have specially interpreted Lua code that expand to v18.2.0, v18.2.1, v18.2.2, reef.

Once it reaches the premerge/postmerge step, these dynamically created fragments already exist (and that's why we can't use the existing premerge/postmerge framework).

Copy link
Author

Choose a reason for hiding this comment

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

doesn't that means we'd still have to get the code to get the dynamic thing done in the teuthology code itself?

teuthology-suite would adjust a directory like

upgrade/tasks/0-from/{install, {base/{squid,reef}}}

so we'd still need to maintain releases manually for this and this is what I'm aiming to remove, i understand your idea about dynamic matrix for testing minor releases but if you want upgrade/tasks/0-from/{install, {base/{n-1,n-2}}} I think there needs to be some code in teuthology that can replace the placeholders with respective releases. Flow:

have generic upgrade YAMLs with install.upgrade: n-1 -> this gets passed to the teuthology code, it figures out what n-1 is -> passes the release to the minor-release-matrix builder code

and for the first step making use of the chacra API is highly useful

where install.yaml is something vanilla like

install: {}

and reef.yaml would have specially interpreted Lua code that expand to v18.2.0, v18.2.1, v18.2.2, reef.

Once it reaches the premerge/postmerge step, these dynamically created fragments already exist (and that's why we can't use the existing premerge/postmerge framework).

supplied_branch = ctx.config.get('branch')
# get rid of very old releases (just for brevity)
releases = (json.loads(output))[7:]
last_release = ""
second_last_release = ""
if util.git_branch_exists('ceph', supplied_branch):
if releases[-1] == 'test':
if supplied_branch == 'main' or supplied_branch not in releases:
last_release = releases[-2]
second_last_release = releases[-3]
elif supplied_branch in releases:
dest_branch_index = releases.index(supplied_branch)
last_release = releases[dest_branch_index - 1]
second_last_release = releases[dest_branch_index - 2]
else:
if supplied_branch == 'main':
last_release = releases[-1]
second_last_release = releases[-2]
else:
dest_branch_index = releases.index(supplied_branch)
last_release = releases[dest_branch_index - 1]
second_last_release = releases[dest_branch_index - 2]
else:
raise ValueError(f'{supplied_branch} does not exist')

if source_branch == 'n-1':
config.update({'branch': last_release})
else:
config.update({'branch': second_last_release})

install_packages(ctx, package_list, config)
try:
yield
Expand Down
Loading