Skip to content

47-joblogs-path#51

Merged
wvengen merged 2 commits intoq-m:mainfrom
vlerkin:47-joblogs-path
Apr 1, 2025
Merged

47-joblogs-path#51
wvengen merged 2 commits intoq-m:mainfrom
vlerkin:47-joblogs-path

Conversation

@vlerkin
Copy link
Copy Markdown
Collaborator

@vlerkin vlerkin commented Apr 1, 2025

#47
The new changes affect two methods:

KubernetesJobLogHandler.handle_events and LibcloudObjectStorage.upload_file

handle_events now extract two more labels from the job pod to identify project and spider names and then passes them to the upload_file function.

upload_file uses project name, spider name and extract job_id from a local_path parameter to construct a new object name that is used to create a specific directory like structure in the remote storage.

…s them through for upload function; in upload function, use spider and project names to construct a new upload path so the log files are grouped by the provided names; fix typos in k8s manifest
@vlerkin vlerkin changed the title extract spider and project names from the labels of a job pod and pas… 47-joblogs-path Apr 1, 2025
@vlerkin vlerkin requested a review from wvengen April 1, 2025 08:28
Copy link
Copy Markdown
Member

@wvengen wvengen left a comment

Choose a reason for hiding this comment

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

Thank you, straightforward implementation, that will be useful!
Some small notes, otherwise good to go 🚀

kubernetes.yaml Outdated

[joblogs]
logs_dir = /data/joblogs
logs_dir = /tmp/joblogs
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why this change?
Using /tmp by default is dangerous, as a pod restart (or moving to another node) can lose joblogs.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah, I forgot to change it, I was using tmp for testing it

verify_hash=False,
headers=None
)
logger.debug("HEY I AN UPLOADING")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

left over ;)

@wvengen
Copy link
Copy Markdown
Member

wvengen commented Apr 1, 2025

p.s. please mention the issue in the PR body, so that it is more easily findable in Github.

Copy link
Copy Markdown
Member

@wvengen wvengen left a comment

Choose a reason for hiding this comment

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

Thank you! ✨

@wvengen wvengen merged commit 6317217 into q-m:main Apr 1, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants