Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
30 changes: 30 additions & 0 deletions CONFIG.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,36 @@ For Kubernetes, it is important to set resource limits.
TODO: explain how to set limits, with default, project and spider specificity.

### [joblogs] section

The joblogs section is used to configure the job logs feature. It is not enabled by default, but can be activated if you
choose to use it. The job logs feature allows you to collect logs from the Kubernetes cluster and store them in a specified
directory. The logs are collected from the pods running on the cluster and can be compressed using a specified method.
The job logs feature is implemented only for Kubernetes and is not available for Docker.

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.

Let's reference #37 here.


When the log file was uploaded to the specified storage, the pod is labeled with org.scrapy.log_file_uploaded=true, it also
contains labels to specify an end location of the log files in the storage.
Since the label field has limitation in the number of symbols, an end location is split into several labels, each of them
contains a part of the end location. The labels are named as follows:
* org.scrapy.project
* org.scrapy.spider
* org.scrapy.job_id
* org.scrapy.extension

How can you build the end location from these labels? Complete the following line
* logs/`org.scrapy.project`/`org.scrapy.spider`/`org.scrapy.job_id`.`org.scrapy.extension`

Comment thread
wvengen marked this conversation as resolved.
Outdated
**Important**: if you used the `compression_method` parameter, the extension of the log file then have two extensions. The
first extension is `.log` and the second extension is extracted to the label `org.scrapy.extension`.

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.

The main motivation for the current issue, is that when the config parameter compression_method is changed, you cannot find the logs anymore without iterating files. Now including the compression extension solves this, which is nice. But it doesn't tell us what the compression is that was used for the logfile - you still need to analyse the extension and derive the compression method.

So I would rather add a compression_method label, and derive the extension. Or, include both the extension and the compression method (so that an external consumer of these labels doesn't need to maintain a mapping between compression method and extension - though it does need to retain a mapping between compression method and decompressor, already, so perhaps this is not an extra burden).

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.

If I follow your logic, it would be better to have two labels: extension which is .log and compression which is either one of the methods or none, right?

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.

(previous comment deleted) - yes!

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.

and I don't think we'd need the extension label then

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.

why we wouldn't need the extension label? how then a person should assume the final destination without knowing if the file was or was not compressed? I mean it is possible to check the config but since we may rollout update the managing pod and put a new config that has a different compression method or do not use compression at all, it would be pretty hard to trace back the extension of the file. I can entirely drop it if you like, just concerned if it makes the life of the user harder since we can reconfigure things without killing the jobs that sit on the cluster in the completed state.

it is not possible to provide extension as .log.gz because the dot . is not allowed in labels according to the k8s documentation (mentioned in the ticket)

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.

It is not convenient to return a compression method since it is a separate class and we have a compression extension, it would force us to have a mapping in a log handler class which is not its responsibility, I would still vore for putting the extension in the label and not the method. For the user converting one to another or vice versa is the same amount of mental load. Since we already have the final destination it would be cleaner to use the given info. But if it's important I can add a mapping there.

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.

For now I add two labels: extension and compression (compression extension, not method) but as I mentioned if this is something must be (method instead of the extension) then we can add some code, although I do not think it is a clean code

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.

I now I understand that my request for adding a compression method label instead of extension, would make the code more complex. I need to look further into this to understand it better, and see if there could be another way. Thanks for explaining!

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.

If you are concerned that we won't be able to decompress a file with "a quasi random compression extension", there are python packages like https://github.com/pycompression/xopen that automatically assume the compression method from the extension of the file and decompress it.

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.

If there are other technical concerns, feel free to share, I can search for possible solutions.


For example, if you used
`compression_method = gzip`, the log file will be named as
* logs/`org.scrapy.project`/`org.scrapy.spider`/`org.scrapy.job_id`.log.`org.scrapy.extension`
Comment thread
wvengen marked this conversation as resolved.
Outdated

If you haven't specified a compression method, then none was applied and the final location of the log file is
* logs/`org.scrapy.project`/`org.scrapy.spider`/`org.scrapy.job_id`.`org.scrapy.extension`

The joblogs section contains the following parameters:
Comment thread
wvengen marked this conversation as resolved.
Outdated

* `logs_dir` - a directory to store log files collected on k8s cluster (implemented only for Kubernetes). If you are using a Persistent Volume, keep in mind, that the provided path should be mounted in the deployment manifest. Read and write permissions should be granted to allow actions with log files in the provided directory, thus a [securityContext](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/) was added to the deployment manifest.
* `compression_method` - a method to compress log files. Available options are `gzip` `bzip2`, `lzma`, `brotli` and `none`. If no compression_method is provided, it defaults to `none`.

Expand Down
1 change: 1 addition & 0 deletions kubernetes.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ metadata:
rules:
- apiGroups: [""]
resources: ["pods"]
# add "patch" if you use joblogs feature

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.

let's make it add "patch" to verbs if you use the joblogs feature (to make it even clearer that it is about the next line)

verbs: ["get", "list", "watch"]
- apiGroups: [""]
resources: ["pods/exec"]
Expand Down
63 changes: 62 additions & 1 deletion scrapyd_k8s/joblogs/log_handler_k8s.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,62 @@ def __init__(self, config):
raise ValueError("Configuration error: 'logs_dir' is missing in joblogs configuration section.")
self.object_storage_provider = LibcloudObjectStorage(self.config)

def _add_object_name_label(self, pod, object_name):
"""
Adds extension as label to the pod.
Adds label that the log file was uploaded.

Parameters
----------
pod : V1Pod
The Kubernetes pod object.
object_name : str
The object name in storage.

Returns
-------
bool
True if the label was added successfully, False otherwise.
"""
try:
extension = os.path.splitext(object_name)[1][1:]
v1 = client.CoreV1Api()

# Create a body for the patch operation
patch_extension = {
"metadata": {
"labels": {
"org.scrapy.log_file_extension": extension
}
}
}

patch_upload_label = {
"metadata": {
"labels": {
"org.scrapy.log_file_uploaded": "true"

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.

If there could be multiple uses of object storage, I propose include joblogs in the labels:
org.scrapy.joblogs.compression_method
org.scrapy.joblogs.uploaded

}
}
}

# Patch the pod with the new label
v1.patch_namespaced_pod(
name=pod.metadata.name,
namespace=self.namespace,
body=patch_extension
)
v1.patch_namespaced_pod(
name=pod.metadata.name,
namespace=self.namespace,
body=patch_upload_label
)

logger.info(f"Added log_file_extension labels '{extension}' and log_file_uploaded: 'true' to pod '{pod.metadata.name}'")
return True
except Exception as e:
logger.error(f"Failed to add labels to pod '{pod.metadata.name}': {e}")
return False

def get_existing_log_filename(self, job_id):
"""
Retrieves the existing temporary log file path for a job without creating a new one.
Expand Down Expand Up @@ -322,7 +378,12 @@ def handle_events(self, event):
logger.info(
f"Removed local log file '{log_filename}' since it already exists in storage.")
else:
self.object_storage_provider.upload_file(project, spider, log_filename)
object_name = self.object_storage_provider.upload_file(project, spider, log_filename)
if object_name:
try:
self._add_object_name_label(pod, object_name)
except Exception as e:
logger.error(f"Exception while adding label for job '{job_id}': {e}")
os.remove(log_filename)
logger.info(f"Removed local log file '{log_filename}' after successful upload.")
else:
Expand Down
7 changes: 6 additions & 1 deletion scrapyd_k8s/object_storage/libcloud_driver.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ def upload_file(self, project, spider, local_path):

Returns
-------
None
str or None
The object name in storage if upload is successful, None otherwise.

Logs
----
Expand Down Expand Up @@ -172,6 +173,10 @@ def upload_file(self, project, spider, local_path):
f"Successfully uploaded compressed file '{object_name}' to container '{self._container_name}'.")
else:
logger.info(f"Successfully uploaded file '{object_name}' to container '{self._container_name}'.")

# Return object_name on successful upload
return object_name

except (ObjectError, ContainerDoesNotExistError, InvalidContainerNameError) as e:
logger.exception(f"Error uploading the file '{object_name}': {e}")
except Exception as e:
Expand Down