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.

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 sort_objects() to make batch resource actions simpler #33

Merged
merged 11 commits into from
Jun 15, 2022

Conversation

ca-scribner
Copy link
Collaborator

@ca-scribner ca-scribner commented Apr 28, 2022

This PR proposes some helpers to address a problem with handling batches of resources. I'm not married to the API, etc., and am happy to adjust and flesh out anything that is missing (for example, create_many()?), but I'm mainly curious atm whether this feature would be appreciated in lightkube or if I should finish it as a separate helper I keep elsewhere.

Working with a batch of resources has an order of operations challenge. For example, this valid YAML contains both a ClusterRoleBinding and the ClusterRole/ServiceAccount it references:

kind: ClusterRoleBinding
roleRef:
  kind: ClusterRole
  name: example-cluster-role-binding
subjects:
- kind: ServiceAccount
  name: example-service-account
...
---
kind: ClusterRole
metadata:
  name: example-cluster-role
...
---
kind: ServiceAccount
metadata:
  name: example-service-account

If users loop over the yaml and create the objects, they will get errors because the ClusterRoleBinding is created before the items it uses. Tools like kubectl or kapp handle this for the user.

This PR is a proposal to similarly automate this issue with lightkube by adding Client.apply_many() and Client.delete_many() helpers. They both accept iterables of resources and, before executing on them, order them in a safer order by kind. afaict this is sufficient to avoid any conflicts, but I'm flexible if there's something missing.

This adds an apply_many function, similar in intent to `kubectl apply -f some-yaml.yaml`, that applies a collection of Resources.  This function also applies the Resources in a safe order by kind, in order to avoid cases like trying to create an instance of a Custom Resource before applying the CustomResourceDefinition.
This adds a delete_many() function, similar to apply_many() and in intent to `kubectl delete -f some-yaml.yaml`, that deletes a collection of Resources.  This function deletes the resources in a safe order by kind, in order to avoid cases like trying to delete an instance of a Custom Resource after the CustomResourceDefinition has already been deleted.

Also added here are tests for this function.
@gtsystem
Copy link
Owner

Did you find the corresponding logic in kubectl source code? Is it just based on resource names like in your implementation or something else?

In general I would like to keep this library sufficiently low level and easy to learn. So I would prefer to avoid new methods that can be created with few lines of code. The key component of your PR is the sorting function and that is something we could include as it's non trivial and can be used for creating more high level functionalities.

So something like

from lightkube import sort_objects

for obj in sort_objects(objs, by='kind-rank'):    # better name and signature TBD
    client.apply(obj)

for obj in sort_objects(objs, by='kind-rank', reverse=True):
    namespace = obj.metadata.namespace if isinstance(obj, NamespacedResource) else None
    client.delete(obj, name=obj.metadata.name, namespace= namespace)

We can also support other types of object sorting in the future.

@ca-scribner
Copy link
Collaborator Author

That makes sense to me - keeping this to a sorting helper works. In my opinion having a _many version of the create/apply/delete would be helpful without complicating the package too much (their names will be intuitive, which I think helps) but I can see your point as well.

I made a mistake in the original post - as far as I can tell, kubectl does not reorder objects before applying. I don't see any ordering in the source, and after testing it explicitly just now I think I have just been lucky in the past and always applied YAML that was already in a safe order. The tool kapp does reorder objects before applying, and their kind-rank is similar or the same to what is here.

@ca-scribner
Copy link
Collaborator Author

Sorry, between business trips and covid I dropped the ball on this. I'll put a workable solution together next week

This adds the `sort_objects()` helper to sort iterables of resource objects in particular orders.  Currently supported is the `kind` order, which orders resources by their kind in an apply-friendly order (avoiding things like applying an instance of a CR before applying the CRD, etc.).

Also supported is an optional `reverse` parameter in the sorting function to reverse the returned order.
@ca-scribner ca-scribner marked this pull request as ready for review June 6, 2022 20:20
@ca-scribner
Copy link
Collaborator Author

I have reverted the previous changes and added a sort_objects like you mentioned. What do you think? I wasn't sure where this function should live - it currently is in client.py, but it feels a bit out of place. Would appreciate suggestions or any requested changes - thanks!

Copy link
Owner

@gtsystem gtsystem left a comment

Choose a reason for hiding this comment

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

I was thinking about codecs as this functionality will be often used in combination with loading definitions, however it doesn't really have to do with conversions..

Maybe we can just add it into a separate module for now (core/sort_objects.py ?) and import the function name in __init__.py.

Would be also good to have an example of usage of this function as part of docs/codecs.md.

lightkube/core/client.py Outdated Show resolved Hide resolved
lightkube/core/client.py Outdated Show resolved Hide resolved
lightkube/core/client.py Outdated Show resolved Hide resolved
lightkube/core/client.py Outdated Show resolved Hide resolved
@ca-scribner ca-scribner changed the title Add Client.apply_many() and Client.delete_many() for batch resource operations Add sort_objects() to make batch resource actions simpler Jun 13, 2022
Moves sort_objects to a new sort_objects.py.  Misc other minor refactors.
@ca-scribner
Copy link
Collaborator Author

@gtsystem thanks for the great feedback. How does it look now?

I will write up a example for codecs.md tomorrow

@gtsystem gtsystem merged commit e2c4139 into gtsystem:master Jun 15, 2022
@gtsystem
Copy link
Owner

Thanks a lot, changes are merged.

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