Handle guest logs #2647

skycastlelily · 2024-01-26T09:35:04Z

Fetch log content and save it to the workdir/provision/ List the log dir in guests.yaml

Pull Request Checklist

tmt/steps/provision/local.py

skycastlelily · 2024-01-30T07:24:03Z

Updated^^,added an Optional log_path param to handle_guest_logs,in case we want to let users specify the log dir in the future,though users can specify --workdir-root.

…

On Tue, Jan 30, 2024 at 12:08 AM Miloš Prchlík ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tmt/steps/provision/local.py <#2647 (comment)>: > @@ -133,6 +133,43 @@ def pull( extend_options: Optional[list[str]] = None) -> None: """ Nothing to be done to pull workdir """ + def acquire_log(self, log_name: str) -> Optional[str]: + """fetch and return content of a requested log""" + if log_name == 'dmesg': + return self.execute(Command('dmesg')).stdout + return None + + def store_log( + self, + log_name: Optional[str], + log_path: Optional[Path], Got it, I was thinking maybe users want to store the logs to dir they ran tmt command:) Maybe, that sounds like something users might do, but then again, it would not be something for store_log() to decide, but for other parts of tmt, e.g tmt.base.Run which implements tmt run: this class would decide where to put logs, and it would task provision step with the right directory. — Reply to this email directly, view it on GitHub <#2647 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFR23AR6H3AJ5E723H3M2DYQ7CNXAVCNFSM6AAAAABCL3RX52VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNBZGA2TOMRUGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

tmt/steps/provision/__init__.py

happz · 2025-02-17T08:14:08Z

As briefly discussed on Slack, there might be a better implementation, bundling together log name & a way to fetch it in a class. That should give us better handling and responsibilities, plus making nicer spot for logs that do not need to be fetched at all, like #3511

happz · 2025-02-24T10:04:37Z

tmt/steps/provision/__init__.py

@@ -993,6 +1008,8 @@ def get_data_class(cls) -> type[GuestData]:
    #: Guest topology hostname or IP address for guest/guest communication.
    topology_address: Optional[str] = None

+    guest_logs: list[GuestLog] = []


it's already in Guest class, guest_ prefix is superfluous. Guest.guest_logs vs Guest.logs

a a class-level mutable list, I would be worried that self.guest_logs.append() below would add guest-specific logs to one shared list. The list should probably be initialized in __init__() instead.

happz · 2025-02-24T10:04:55Z

tmt/steps/provision/__init__.py

@@ -1646,6 +1663,60 @@ def essential_requires(cls) -> list['tmt.base.Dependency']:

        return []

+    def acquire_log(self, logname: str) -> Optional[str]:


This method is probably not needed anymore.

happz · 2025-02-24T10:06:37Z

tmt/steps/provision/__init__.py

+        """
+        raise NotImplementedError
+
+    def store_log(self, path: Path, content: str, logname: Optional[str] = None) -> None:


This method can move to GuestLog class, right? There's log.fetch() followed by store_log(), why not have e.g. save() method accepting this arguments, which would then call self.fetch() and self.store()?

happz · 2025-02-24T10:08:34Z

tmt/steps/provision/mrack.py

+
+        if self.name == 'dmesg':
+            return self.guest.execute(Command('dmesg')).stdout
+        return tmt.utils.get_url_content(self.url) if self.url else None


1 dmesg should have its own subclass, right? It has a different implementation of fetch() after all
2. I'm not sure it would be always possible to run self.guest.execute(), the guest may be dead. It would be better to download Beaker's console.log instead.

happz · 2025-02-24T10:09:12Z

tmt/steps/provision/mrack.py

+
+@container
+class GuestLogBeaker(tmt.steps.provision.GuestLog):
+    def __init__(self, guest: GuestBeaker, name: str, url: Optional[str] = None) -> None:


With container, you shouldn't need custom __init__(), one will be generated automagically for you.

happz · 2025-02-24T10:11:23Z

tmt/steps/provision/__init__.py

+
+        guest_logs = guest_logs or self.guest_logs or []
+
+        dirpath = dirpath or (self.workdir / 'logs' if self.workdir else None) or Path.cwd()


There should be a logdir property of Guest class that does this: every part of code interested in finding where guest's logs are should use guest.logdir instead of constructing the part on their own. Then you should be able to simply run guest.logdir.mkdir(parents=True, exists_ok=True) instead of checking self.workdir again below.

happz · 2025-02-24T10:13:18Z

tmt/steps/provision/__init__.py

+        if self.workdir is None:
+            return
+
+        logs_dir = self.workdir / 'logs'


Yes, guest.logdir should be used, and same below - if member.name == "logs" should become if member.name == guest.logdir.name- the magical string "logs" appears way too many times through the code, if we decide to rename it to "guest-logs" tomorrow, all these strings would need to be changed. Withguest.logdir`, and using it in conditions and calls, we would need to change just one place.

Another point: trying to provide custom implementation to preserve some (dynamic) filenames, and by doing so, we get different levels of logging and different behavior from one step only. That's not good. Maybe we should first change how Step.prune() works, and instead of file/directory names, we should use regular expressions. Then we could just add logs/.*\.txt to Provision._preserved_workdir_members and keep using the default prune() method. That would be a nice, self-contained PR right there ;)

I guess the problem is plugin.prune will remove the whole workdir even if there are preserved members in it:
and here is the mr to solve it:https://github.com/teemtee/tmt/pull/3548^^

Fetch log content,save it to provided location or current dir

skycastlelily · 2025-02-25T09:38:49Z

wow,thanks for your super quick response and valuable feedbacks,impressed again,and updated^^

skycastlelily requested review from psss, lukaszachy, happz, thrix and janhavlin as code owners January 26, 2024 09:35

happz reviewed Jan 26, 2024

View reviewed changes