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

Accept and apply workdir-root cli option in testcloud plugin #2838

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Accept and apply workdir-root cli option in testcloud plugin #2838

Show file tree
Hide file tree
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion tmt/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3754,7 +3754,9 @@ def images(self) -> bool:
successful = True
for method in tmt.steps.provision.ProvisionPlugin.methods():
# FIXME: ignore[union-attr]: https://github.com/teemtee/tmt/issues/1599
if not method.class_.clean_images(self, self.is_dry_run): # type: ignore[union-attr]
if not method.class_.clean_images(
self, self.is_dry_run, self.optPath(
self.opt('workdir-root'))): # type: ignore[union-attr]
successful = False
return successful

Expand Down
54 changes: 34 additions & 20 deletions tmt/steps/provision/testcloud.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
ShellScript,
cached_property,
configure_constant,
effective_workdir_root,
field,
retry_session,
)
Expand Down Expand Up @@ -92,12 +93,6 @@ def import_testcloud() -> None:
TPM_CONFIG_ALLOWS_VERSIONS = hasattr(TPMConfiguration(), 'version')


# Testcloud cache to our tmt's workdir root
TESTCLOUD_DATA = (
Path(os.environ['TMT_WORKDIR_ROOT']) if os.getenv('TMT_WORKDIR_ROOT') else WORKDIR_ROOT
) / 'testcloud'
TESTCLOUD_IMAGES = TESTCLOUD_DATA / 'images'

# Userdata for cloud-init
USER_DATA = """#cloud-config
chpasswd:
Expand Down Expand Up @@ -294,6 +289,17 @@ def _report_hw_requirement_support(constraint: tmt.hardware.Constraint[Any]) ->
return False


def get_workdir_root(workdir_root_option: Optional[str]) -> Path:

if workdir_root_option:
testcloud_data = effective_workdir_root(workdir_root_option) / 'testcloud'
elif effective_workdir_root():
testcloud_data = effective_workdir_root() / 'testcloud'
else:
testcloud_data = WORKDIR_ROOT / 'testcloud'
return testcloud_data


@dataclasses.dataclass
class TestcloudGuestData(tmt.steps.provision.GuestSshData):
# Override parent class with our defaults
Expand Down Expand Up @@ -551,10 +557,12 @@ def _guess_image_url(self, name: str) -> str:

def wake(self) -> None:
""" Wake up the guest """

self.debug(
f"Waking up testcloud instance '{self.instance_name}'.",
level=2, shift=0)
self.prepare_config()

assert testcloud is not None
self._image = testcloud.image.Image(self.image_url)
if self.instance_name is None:
Expand Down Expand Up @@ -611,9 +619,11 @@ def prepare_config(self) -> None:
self.config.DOWNLOAD_PROGRESS = self.debug_level > 2
self.config.DOWNLOAD_PROGRESS_VERBOSE = False

testcloud_data = get_workdir_root(self.opt('workdir-root'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

testcloud plugin does not have workdir-root option, and it should not really have one, or even care about one. At this point of time, the workdir root has been already established by tmt, and used to spawn self.workdir, for example. Instead of guessing what the right value is - with all the involved inputs - we should try to access this single source of truth. We can save the workdir root besides the self.workdir attribute, or we can take self.workdir and use .parent as needed.

And as a concept shared by all plugins, and actually steps and plans as well, there should be one implementation, plugin-agnostic. Putting it into a single plugin does not seem correct to me. Seems more like a property or attribute of tmt.utils.Common, for all classes to use when needed.


# Configure to tmt's storage directories
self.config.DATA_DIR = TESTCLOUD_DATA
self.config.STORE_DIR = TESTCLOUD_IMAGES
self.config.DATA_DIR = testcloud_data
self.config.STORE_DIR = testcloud_data / 'images'

def _combine_hw_memory(self) -> None:
""" Combine ``hardware`` with ``--memory`` option """
Expand Down Expand Up @@ -765,13 +775,13 @@ def start(self) -> None:
""" Start provisioned guest """
if self.is_dry_run:
return
# Make sure required directories exist
os.makedirs(TESTCLOUD_DATA, exist_ok=True)
os.makedirs(TESTCLOUD_IMAGES, exist_ok=True)

# Prepare config
self.prepare_config()

# Make sure required directories exist
os.makedirs(self.config.DATA_DIR, exist_ok=True)
os.makedirs(self.config.STORE_DIR, exist_ok=True)

# Kick off image URL with the given image
self.image_url = self.image

Expand All @@ -795,7 +805,7 @@ def start(self) -> None:
except (testcloud.exceptions.TestcloudPermissionsError,
PermissionError) as error:
raise ProvisionError(
f"Failed to prepare the image. Check the '{TESTCLOUD_IMAGES}' "
f"Failed to prepare the image. Check the '{self.config.STORE_DIR}' "
f"directory permissions.") from error
except KeyError as error:
raise ProvisionError(
Expand Down Expand Up @@ -863,7 +873,6 @@ def start(self) -> None:
image=self._image,
connection=f"qemu:///{self.connection}",
domain_configuration=self._domain)

self.verbose('name', self.instance_name, 'green')

# Decide if we want to multiply timeouts when emulating an architecture
Expand Down Expand Up @@ -1010,6 +1019,10 @@ class ProvisionTestcloud(tmt.steps.provision.ProvisionPlugin[ProvisionTestcloudD
# Guest instance
_guest = None

@property
def testcloud_images(self) -> Path:
return get_workdir_root(self.opt('workdir-root')) / 'images'

def go(self) -> None:
""" Provision the testcloud instance """
super().go()
Expand Down Expand Up @@ -1073,20 +1086,21 @@ def guest(self) -> Optional[tmt.Guest]:
def _print_local_images(self) -> None:
""" Print images which are already cached """
self.info("Locally available images")
for filename in sorted(TESTCLOUD_IMAGES.glob('*.qcow2')):
for filename in sorted(self.testcloud_images.glob('*.qcow2')):
self.info(filename.name, shift=1, color='yellow')
click.echo(f"{TESTCLOUD_IMAGES / filename}")
click.echo(f"{self.testcloud_images / filename}")

@classmethod
def clean_images(cls, clean: 'tmt.base.Clean', dry: bool) -> bool:
def clean_images(cls, clean: 'tmt.base.Clean', dry: bool, workdir_root: Path) -> bool:
""" Remove the testcloud images """
testcloud_images = workdir_root / 'testcloud/images'
clean.info('testcloud', shift=1, color='green')
if not TESTCLOUD_IMAGES.exists():
if not testcloud_images.exists():
clean.warn(
f"Directory '{TESTCLOUD_IMAGES}' does not exist.", shift=2)
f"Directory '{testcloud_images}' does not exist.", shift=2)
return True
successful = True
for image in TESTCLOUD_IMAGES.iterdir():
for image in testcloud_images.iterdir():
if dry:
clean.verbose(f"Would remove '{image}'.", shift=2)
else:
Expand Down
8 changes: 6 additions & 2 deletions tmt/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,14 +251,18 @@ def is_pidfile(cls, exit_code: Optional[int]) -> bool:
T = TypeVar('T')


def effective_workdir_root() -> Path:
def effective_workdir_root(workdir_root_option: Optional[str] = None) -> Path:
"""
Find out what the actual workdir root is.

If ``TMT_WORKDIR_ROOT`` variable is set, it is used as the workdir root.
If ``workdir-root`` cli option is set, it is used as the workdir root.
Otherwise, ``TMT_WORKDIR_ROOT`` variable is used, if it is set.
Otherwise, the default of :py:data:`WORKDIR_ROOT` is used.
"""

if workdir_root_option:
return Path(workdir_root_option)

if 'TMT_WORKDIR_ROOT' in os.environ:
return Path(os.environ['TMT_WORKDIR_ROOT'])

Expand Down