-
Notifications
You must be signed in to change notification settings - Fork 116
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.
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
Add ability to save diagnostics data to artifacts #5583
Conversation
Logs:
Artifacts API:
Inside the container:
|
8576563
to
7c3bc56
Compare
Open questions:
|
|
35ed9e7
to
415b612
Compare
Done. Logs:
|
415b612
to
91d25e3
Compare
pulpcore/app/viewsets/content.py
Outdated
@@ -86,6 +87,14 @@ def destroy(self, request, pk): | |||
data = {"detail": msg} | |||
return Response(data, status=status.HTTP_409_CONFLICT) | |||
|
|||
@action(detail=True) | |||
def download(self, request, pk): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new action for all artifacts, not just the ones produces by task diagnostics, it needs a separate changelog as .feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot just do that without talking about RBAC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What RBAC? There are some discrepancies that should be resolved in a separate PR first. We say that only admins can see the endpoint, yet, regular users can upload raw artifacts to Pulp: #5525.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading other peoples artifacts is a new level here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add that as an action to the tasks viewset instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
Will I need to remove permission_classes = (permissions.IsAuthenticated,)
on the ViewSet in order to define an access policy statement for this specific action? If yes, this would mean that the endpoint will be locked for users, and as @gerrod3 pointed out in his comment in the attached issue, every uploading scenario in pulp-cli will start to fail.
Or, do you want me to create a workaround with a custom permission class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add that as an action to the tasks viewset instead?
Can you elaborate, please? Would you like to return a task instead of an immediate response?
91d25e3
to
f02265e
Compare
After discussing this with @mdellweg, we reached a consensus on updating this PR. The current implementation is not the best one. Right now, it adds a new Key points we talked about:
Open questions:
|
Being able to download the Artifact was what I initially wanted so this is looking good. Is the idea to have the new action API be something like |
Yes. And, this should not have any side effect since we can protect it with the RBAC. |
719e1e0
to
0a2ce88
Compare
The artifacts can be now downloaded from the task's detail endpoint:
|
@@ -242,6 +242,19 @@ def purge(self, request): | |||
) | |||
return OperationPostponedResponse(task, request) | |||
|
|||
@action(detail=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be an attached serializer.
Also i think we need to mark the action as "GET".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The action decorator will route GET requests by default". I am going only to attach the serializer there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the best I could do:
"ProfileDataResponse": {
"type": "object",
"properties": {
"urls": {
"type": "object",
"additionalProperties": {
"type": "string",
"format": "uri"
}
}
},
"required": [
"urls"
]
},
http :5001/pulp/default/api/v3/tasks/0190db0f-e1d9-71fb-a514-fb8d8fbee514/profile_data/
{
"urls": {
"memory_profile": "http://localhost:5001/pulp/content/default/3ae491a65a6819a786e90583e44bba65cf2ebba89e03b3db9913e19cd4e848e5/0190db0f-e306-7909-91b4-50e839307282?expires=1721729565&validate_token=8f4bdddf4c68e08473881524710f6ccf6ae3579bb14a93882d527ce70257d15b:7425724ea8fee084509927e0a0cd8233cd4338db9213d63ba963a0b1e4b6ec80"
}
}
I could not make this working without having the "urls" key. I am not sure it is right to define a raw generic schema for the response that returns only "object" without "properties".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more specific we are, the more levels will the bindings create objects instead of dicts.
This is a playground for subtly breaking backwards compatibility of the generated bindings when in fact both specs describe the very same api in correct ways.
0a2ce88
to
16cc00b
Compare
pulpcore/app/models/task.py
Outdated
@@ -100,6 +113,8 @@ class Task(BaseModel, AutoAddObjPermsMixin): | |||
pulp_domain = models.ForeignKey("Domain", default=get_domain_pk, on_delete=models.CASCADE) | |||
versions = HStoreField(default=dict) | |||
|
|||
profile_data = models.ManyToManyField("Artifact", through=ProfileData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
profile_data = models.ManyToManyField("Artifact", through=ProfileData) | |
profile_artifacts = models.ManyToManyField("Artifact", through=ProfileData) |
16cc00b
to
95b1034
Compare
pulpcore/app/viewsets/task.py
Outdated
@@ -87,7 +88,7 @@ class TaskViewSet( | |||
"statements": [ | |||
{"action": ["list"], "principal": "authenticated", "effect": "allow"}, | |||
{ | |||
"action": ["retrieve", "my_permissions"], | |||
"action": ["retrieve", "profile_data", "my_permissions"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure this will not be the granular RBAC you promoted, but will enable an admin with sufficient Ssechel make it granular. Don't we need to provide a permission for it too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can introduce a new permission for this. Yet, I would say that people who can preview a task, should also see its profile data.
responses=inline_serializer( | ||
"ProfileDataResponse", | ||
fields={"urls": DictField(child=URLField())}, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
95b1034
to
c05feb0
Compare
14ff3aa
to
304214b
Compare
pulpcore/app/models/task.py
Outdated
name = models.TextField() | ||
|
||
class Meta: | ||
unique_together = ("artifact", "name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unique_together = ("artifact", "name") | |
unique_together = ("task", "name") |
pulpcore/app/models/task.py
Outdated
@@ -286,6 +301,7 @@ class Meta: | |||
] | |||
permissions = [ | |||
("manage_roles_task", "Can manage role assignments on task"), | |||
("view_task_profile_artifacts", "Can preview profile data for task"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
("view_task_profile_artifacts", "Can preview profile data for task"), | |
("view_task_profile_artifacts", "Can view profile data for task"), |
The default setting is `False`. When set to `True`, each task records various diagnostics (listed below) | ||
and stores them as separate artifacts. To download the data, issue GET requests to `${TASK_HREF}profile_artifacts/`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably mention that they will be cleaned up automatically.
304214b
to
593f459
Compare
593f459
to
b199816
Compare
b199816
to
3ecfa97
Compare
closes #5422