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

enhancement: extend the p2p preheat policy #252

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chlins
Copy link
Member

@chlins chlins commented Nov 6, 2024

Propose to extend the p2p preheat policy for enhancing the preheat scenario in AI era.

## Goal

1. Some common and abstractable parameters that are not strongly bound to P2P providers can be abstracted as a field in a policy.
2. Also provides an extended parameter for users to specify provider-related or unique configurations.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the extra_attrs are provider specific, how will Harbor send the extra_attrs?
IMO this has to be answered to make sure other p2p providers can consume the new attributes.

Additionally, why not make the "scope" also part of extra_attrs, if at this moment only dragonfly can consume it?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the harbor codebase, each p2p provider will have a driver implementation capable of parsing or handling extra_attrs independently to determine how to transmit them. We emphasize the scope separately from extra_attrs because it's a universally applicable, vendor-neutral property in p2p scenarios. Placing it as a standalone attribute is more user-friendly than including it within extra_attrs, as the latter is intended for vendor-specific attributes. We consider scope a standard p2p option.

Currently, I think we do not have much choice for image p2p solution, harbor integrates dragonfly and kraken, and it seems kraken project is no longer in an active maintenance state, as it's last release version was still published four years ago.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the harbor codebase, each p2p provider will have a driver implementation capable of parsing or handling extra_attrs independently to determine how to transmit them.

Is it already in the code base to handle extra_attrs? I don't see how the Driver interface handles the extra_attrs or even the policy. If there are any changes need to made in the driver side they should also be reflected in this design.

We emphasize the scope separately from extra_attrs because it's a universally applicable, vendor-neutral property in p2p scenarios.

I agree "scope" can be a common concept for P2P scenario, but different vendors may parse the param differently, for example, a vendor may support distributing the artifacts to certain regions or nodes with certain labels. Therefore, I wish to suggest we leave it in the "extra_attrs" to provide more flexibilities for providers.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to change the Driver interface, we only need to add the field in the Preheat function parameters, then the driver self can get it and process it by their own logic. I can supplement this field into the design.

I agree with what you said that different vendors may have different requirements, such as region or label, etc., so these custom items for vendors can all be placed in extra. However, I still believe that scope should be a very general and clear concept that can be shared or recognized by all p2p solutions, so the separate abstraction can bring a better user experience. Because although extra can contain anything, from my personal perspective, the user experience of extra is not very friendly, and its concept is quite broad.

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.

5 participants