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

LEP: Object Support for Longhorn #5832

Closed
wants to merge 2 commits into from

Conversation

jecluis
Copy link

@jecluis jecluis commented Apr 30, 2023

We are proposing Object support for Longhorn, by integrating aquarist-labs/s3gw into Longhorn, and seamlessly provide S3-compatible object endpoints from Longhorn volumes.

Signed-off-by: Joao Eduardo Luis <joao@suse.com>

Please Note: While we believe this to be a decent state, there are many things that are still unknowns and we would love the feedback, and any constructive criticism or suggestions on how to improve will be immensely appreciated!

@jecluis jecluis requested a review from a team as a code owner April 30, 2023 22:57
@jecluis jecluis marked this pull request as draft April 30, 2023 22:57
@innobead
Copy link
Member

innobead commented May 8, 2023

@shuo-wu @PhanLe1010 review this after 1.5.0.

@l-mb
Copy link

l-mb commented May 8, 2023

@shuo-wu @PhanLe1010 review this after 1.5.0.

Hi @innobead, would it be possible to give the team a casual review of the approach already? If so, we could get started on a tentative/draft PR of the actual controller already, which might drive further discussion without waiting a few weeks. We just don't want to head down the entirely wrong way :)

@innobead
Copy link
Member

innobead commented May 8, 2023

@l-mb , NP. I think members will try to review it when available. I have reviewed it in general, and it should be fine generally but some details need to craft over time. Having a controller is correct as described in the PRD.

@innobead innobead requested a review from derekbit July 14, 2023 09:16
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this pull request Jul 14, 2023
m-ildefons added a commit to m-ildefons/longhorn that referenced this pull request Jul 14, 2023
- Add config options and flags for object gateway
- Add CRDs for the object endpoint
- Regenerate depoy/longhorn.yaml

Related-to: longhorn#5832
Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
Copy link
Contributor

@PhanLe1010 PhanLe1010 left a comment

Choose a reason for hiding this comment

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

I have left some questions.

I am still learning S3GW and COSI, so please forgive me if my questions don't make sense or if they are naive

enhancements/20230430-object-endpoint.md Outdated Show resolved Hide resolved
enhancements/20230430-object-endpoint.md Outdated Show resolved Hide resolved
enhancements/20230430-object-endpoint.md Outdated Show resolved Hide resolved
enhancements/20230430-object-endpoint.md Show resolved Hide resolved
enhancements/20230430-object-endpoint.md Outdated Show resolved Hide resolved
enhancements/20230430-object-endpoint.md Outdated Show resolved Hide resolved
@m-ildefons
Copy link

For the current POC implementation I've been playing around with different settings that may or may not be useful in the ObjectEndpoint CRD. This led me to the conclusion that for the features described in this LEP, the following form of a CRD would be best suited:

---
apiVersion: longhorn.io/v1beta2
kind: ObjectEndpoint
metadata:
  name: test-endpoint
  namespace: longhorn-system
spec:
  storageClass: longhorn
  credentials:
    accessKey: foo
    secretKey: bar
  size: 5Gi

The differences to the proposed CRD are:

  • The Image field is missing. This field is unnecessary as the Longhorn manager already knows which image it should use to deploy the s3gw, as that is given as command line option.

  • PublicDomain and SSLCertificate are missing because I haven't implemented the features making use of these settings yet.

  • Volume field has been replaced by the optional storageClass and size fields. These two fields describe the desired properties of the backing volume sufficiently. The controller can then automatically deploy the resources necessary to realize this. This makes it easier on a user to deploy an object endpoint, since they don't have to deploy a volume first and take care of the association with the object endpoint manually.

@giubacc
Copy link

giubacc commented Jul 21, 2023

For the current POC implementation I've been playing around with different settings that may or may not be useful in the ObjectEndpoint CRD. This led me to the conclusion that for the features described in this LEP, the following form of a CRD would be best suited:

---
apiVersion: longhorn.io/v1beta2
kind: ObjectEndpoint
metadata:
  name: test-endpoint
  namespace: longhorn-system
spec:
  storageClass: longhorn
  credentials:
    accessKey: foo
    secretKey: bar
  size: 5Gi

The differences to the proposed CRD are:

  • The Image field is missing. This field is unnecessary as the Longhorn manager already knows which image it should use to deploy the s3gw, as that is given as command line option.
  • PublicDomain and SSLCertificate are missing because I haven't implemented the features making use of these settings yet.
  • Volume field has been replaced by the optional storageClass and size fields. These two fields describe the desired properties of the backing volume sufficiently. The controller can then automatically deploy the resources necessary to realize this. This makes it easier on a user to deploy an object endpoint, since they don't have to deploy a volume first and take care of the association with the object endpoint manually.

This sounds nicely. 👍

Some random questions:

Could it make sense to add Longhorn specific settings for controlling the volume properties?
Or here are we saying that we rely completely on the properties defined in the storage-class?
The use case here seems to suggest that is responsibility of the user to define his storage classes. Could this be automated someway when s3gw is used with LH (that is the privileged scenario)? Or is this the intended design?

@m-ildefons
Copy link

Could it make sense to add Longhorn specific settings for controlling the volume properties? Or here are we saying that we rely completely on the properties defined in the storage-class? The use case here seems to suggest that is responsibility of the user to define his storage classes. Could this be automated someway when s3gw is used with LH (that is the privileged scenario)? Or is this the intended design?

Both variants, expecting a user to pre-create storage classes or offering all settings, have their pros and cons. Expecting the user to create storage classes certainly is much simpler.
One noteworthy thing here is, that when creating a volume through the Longhorn UI, the users is asked for various settings directly, which completely circumvents the storage classes.
I'm not sure if we want to copy this behavior, but it would be a reasonable choice to be consistent with the volume creation dialogue.

We're also not necessarily confined to either/or exclusively. We could create an abstraction that can handle both, it would just be more work, e.g.:

longhorn.ObjectEndpoint{
  ObjectMeta metav1.ObjectMeta {
    Name: <string>
  },
  Spec: longhorn.ObjectEndpointSpec{
    Storage: <longhorn.StorageSettings>
  },
}

longhorn.StorageSettings{
  Size: <resource.Quantity>
  
  //+optional
  StorageClass: <string>
  
  //+optional
  VolumeSettings: <longhorn.VolumeSettings>
},

longhorn.VolumeSettings{
  numberOfReplicas: <string>
  staleReplicaTimeout: <string>
  fsType: <string>
  allowVolumeExpansion: <bool>
  [...]
},

This way both a storage class or volume settings could be specified directly. It's then the task of the controller to ensure that a) no conflicting settings are given (i.e. either the storage class or the volume settings) and b) to provision the correct resources and connect them up.
This mechanism is certainly the most involved on the controller side, but it's within the realm of technical possibilities.

A strong argument for supporting volume settings directly instead of through a storage class is, that as far as I can tell the Longhonr UI does not have a dialogue for creating storage classes. It's up to the user to create them through the use of YAML files and kubectl, which is a bit of an ugly spot on the user experience IMO, especially when we expect the user to do that to make use of all the settings for the object endpoint. But a dialogue for creating storage classes from the Longhorn UI isn't in the scope of this LEP.

@jecluis
Copy link
Author

jecluis commented Jul 24, 2023

Why would we need to have the user defining the storage class? Can't we create one if there isn't one ourselves? Wouldn't that also give us more control over how the data is kept?

@m-ildefons
Copy link

Why would we need to have the user defining the storage class? Can't we create one if there isn't one ourselves? Wouldn't that also give us more control over how the data is kept?

There's a default storage class that we can fall back onto, in case the user hasn't deleted it.

Unrelated to that, another question: Should the ObjectEndpoint resource be namespace or cluster scoped?
The argument for being cluster scoped would be that this would allow us to keep the deployments, services, PVCs confined to the longhorn-system namespace, hiding them from the user.
Making it namespace scoped has advantages for users who utilize (cluster-)roles to limit permissions of actors on the cluster. I'm not sure how important that is.

@giubacc
Copy link

giubacc commented Jul 25, 2023

The argument for being cluster scoped would be that this would allow us to keep the deployments, services, PVCs confined to the longhorn-system namespace, hiding them from the user.

I haven't a strong opinion pro or cons one approach.
I would prefer technical s3gw resources to not pollute the user's namespace ... but being more pro or cons one way could become more obvious in practice.

m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this pull request Aug 11, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
Comment on lines +28 to +35
* Integration of s3gw UI for administration and management purposes. Such an
Enhancement Proposal should be a standalone LEP by its own right.
* Providing Object endpoints for multiple volumes. In this proposal we limit one
object endpoint per Longhorn volume (see longhorn/longhorn#5444).
* Multiple Object endpoints for a single volume, either in Read-Write Many, or
as active/passive for HA failover. While we believe it to be of the utmost
importance, we are considering it future work that should be addressed in its
own LEP.
* Specify how COSI can be implemented for Longhorn and s3gw. This too should be
addressed in a specific LEP.
Copy link
Contributor

Choose a reason for hiding this comment

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

While these are non-goals for this LEP. I feel that we should keep them in mind while designing this PR to make it easier for future work

@jecluis jecluis force-pushed the lep-20230430-object-endpoint branch from 774c067 to f992d3b Compare August 28, 2023 21:28
@jecluis jecluis marked this pull request as ready for review August 28, 2023 21:30
@jecluis jecluis force-pushed the lep-20230430-object-endpoint branch from f992d3b to 7e55591 Compare August 28, 2023 21:31
Comment on lines +192 to +193
During resource creation we will also need to explicitly create a
`Persitent Volume` that will be bound to the `Persistent Volume Claim` mentioned

Choose a reason for hiding this comment

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

We will also need to create the Longhorn volume explicitly.

```golang
type ObjectEndpointSpec struct {
Credentials ObjectEndpointCredentials
StorageClass string

Choose a reason for hiding this comment

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

The StorageClass property is ignored[1], since we manage the volume explicitly ourselves in the controller. Perhaps it should be removed or replaced by other properties that are more meaningful in this context.

[1]:
When explicitly creating a PV and PVC together, the storage class property must match or be an empty string, but all other aspects of the storage class are ignored.
https://kubernetes.io/docs/concepts/storage/persistent-volumes/#reserving-a-persistentvolume

Copy link
Contributor

@PhanLe1010 PhanLe1010 Sep 7, 2023

Choose a reason for hiding this comment

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

Agree to remove the StorageClass property. We still need to hardcode a fixed storageclass like longhorn-objectstore (without any parameter) so that features like PVC expansion can work. This is important because user will need to be able to resize the PVC. Ref the POC https://github.com/longhorn/longhorn-manager/blob/c30fb8b1250ee6d2cecd3e818d3ce7b60719fa23/controller/object_endpoint_controller.go#L433-L434

Comment on lines +197 to +202
While handling `starting`, the resources required to have already been created
through the Kubernetes API, but the controller still has to wait for them to be
ready and healthy before the transition to `running` can be performed.
Coincidentally, this is also the behavior expected when the endpoint is in state
`error`, given the controller will need to wait for resources to be healthy
before being able to move the endpoint to state `running`.

Choose a reason for hiding this comment

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

Is this a requirement or is this documenting what the current iteration of the controller is doing?

Copy link
Author

Choose a reason for hiding this comment

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

It is incorporating what the current implementation is doing.

Comment on lines +85 to +91
Integrating Longhorn with s3gw will require the creation of mechanisms that
allow us to 1) describe an object endpoint; 2) deploy an `s3gw` pod consuming
a volume; and 3) deploy an `s3gw-ui` pod for management and administration. For
the purposes of this LEP, there will always be one single `s3gw-ui` pod
associated with an `s3gw` service, hence one `s3gw-ui` pod per Object Endpoint.
We do not exclude eventually allowing one single `s3gw-ui` pod being associated
with multiple Object Endpoints, but that should be considered as future work.

Choose a reason for hiding this comment

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

This LEP now includes the UI integration as well?

Copy link
Author

Choose a reason for hiding this comment

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

Integration, no. That presumes the administration and management UI becomes part of the Longhorn UI. That has been stated from the beginning that is not part of this LEP.

s3gw-ui support on the other hand has always been part of this LEP.

Comment on lines +241 to +245
### Test plan

It is not clear at this moment how this can be tested, much due to lack of
knowledge on how Longhorn testing works. Help on this topic would be much
appreciated.

Choose a reason for hiding this comment

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

There is an integration test suite in longhorn/longhorn-tests which we can utilize to test the controller's behavior from a K8s client point of view.
There are also unit tests in the controller it self, which can be used to test individual functions against a mocked/faked K8s API.

Comment on lines 63 to 64
* For publicly accessible endpoints, the user must specify a domain name to be
used

Choose a reason for hiding this comment

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

Is being cluster-externally accessible a requirement for this LEP?

Copy link
Author

Choose a reason for hiding this comment

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

Potentially. I don't see why not. It shouldn't be adding a lot more complexity, I think, and requiring another LEP to make stuff externally accessible seems a bit of overkill to me.

Copy link

Choose a reason for hiding this comment

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

It needs to be - otherwise we can't support the backup-from-another-LH-cluster-to-s3 use case.

* This can, potentially, be randomly generated as well.
* For publicly accessible endpoints, the user must specify a domain name to be
used
* The user must provide SSL certificates to be used by the endpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the user must provide an SSL certificate?
I thought that the SSL cert, and ingress stuff are handled by users (not Longhorn) when they want to expose the s3 endpoint to outside of the cluster?

Copy link
Author

Choose a reason for hiding this comment

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

In-cluster deployments may also require SSL -- we know of at least such a user. We can drop this requirement and have the user deploy SSL themselves, but natively supporting this seems the path of least resistance. Is there anything you are worried about?

Copy link

Choose a reason for hiding this comment

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

Can't we document / automate a cert-manager integration? We need SSL for the UIs anyway too.

We define a new Object Endpoint Custom Resource Definition, as follows:

```golang
type ObjectEndpoint struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

When I did some demos about this s3 feature to other team members. There are comments about the name of the CRD. ObjectEndpoint sounds like an URL or link. How about the name ObjectStore which indicate a storage system for s3 objects? Looking at rook, looks like they are also using ObjectStore name https://rook.io/docs/rook/v1.9/CRDs/Object-Storage/ceph-object-store-crd/

Copy link

Choose a reason for hiding this comment

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

Amazon calls theirs "S3 endpoints". And, well, they are the API endpoints associated with the S3 API too, and we can have multiple ones of them. I think either phrasing works and we shouldn't get too invested in this argument :-)

Choose a reason for hiding this comment

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

I agree with Phan here. Using ObjectStore would leave endpoint free to describe TLS and ingress/domain configuration, allowing perhaps the use of multiple endpoints for one store, e.g. like so:

kind: ObjectStore
apiVersion: longhorn.io/v1beta2
metadata:
[...]
spec:
  credentials:
    accessKey: ...
    secretKey: ...
  storage:
    size: 100Gi
    numberOfReplicas: 3
    diskSelector:
      - ssd
  endpoints:
    - name: public
      domainName: foobar.example.com
      tls: ...
    - name: private
      domainName: foobar.longhorn-system.svc.cluster.local


Integrating Longhorn with s3gw will require the creation of mechanisms that
allow us to 1) describe an object endpoint; 2) deploy an `s3gw` pod consuming
a volume; and 3) deploy an `s3gw-ui` pod for management and administration. For
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we support deploying S3GW ui here given the fact that we don't help user to deploy ingress. tls cert by default?

If we can deploy s3gw-ui without having deploying ingress, middleware for CORS, and tls cert, that would be great. But are we able to do this currently in s3gw?

Copy link
Author

Choose a reason for hiding this comment

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

I believe this has been discussed previously. There are on-going efforts to make the s3gw-ui not depend on a specific ingress to handle CORS. This LEP accounts for those efforts.

Comment on lines +228 to +239
### Backup and Restore

Given the data being kept by an Object Endpoint is stored in a Longhorn volume,
we rely on Longhorn's backup and restore capabilities.

However, an Object Endpoint in this context is more than just the data held by a
given volume: there's meta state that needs to be backed up and restored, in the
form of Secrets, endpoint names and their associated volume, etc.

At this point in time we don't yet have a solution for this, but we believe this
should rely on whatever mechanisms Longhorn has to backup and restore its own
state. Insights are greatly appreciated.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am working on designing this part

Copy link

Choose a reason for hiding this comment

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

It'd be very interesting if we could restore an endpoint to a different/new one - so someone can pull up the "old" version while the new one remains around.

During resource creation we will also need to explicitly create a
`Persitent Volume` that will be bound to the `Persistent Volume Claim` mentioned
above. We need to do this so we can opinionate on the file system being used; in
this case, XFS, which `s3gw` requires for performance reasons.
Copy link

Choose a reason for hiding this comment

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

(As a note, we need the reflink support so we can effectively merge multipart uploads etc, which ext4 can't do.)

PhanLe1010 pushed a commit to PhanLe1010/longhorn-manager that referenced this pull request Sep 6, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
PhanLe1010 pushed a commit to PhanLe1010/longhorn-manager that referenced this pull request Sep 6, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn that referenced this pull request Sep 6, 2023
- Add config options and flags for object gateway
- Add CRDs for the object endpoint
- Regenerate depoy/longhorn.yaml

Related-to: longhorn#5832
Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this pull request Sep 6, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this pull request Sep 6, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this pull request Sep 19, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn that referenced this pull request Sep 28, 2023
- Add config options and flags for object gateway
- Add CRDs for the object endpoint
- Regenerate depoy/longhorn.yaml

Related-to: longhorn#5832
Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
the existing `/v1/volumes` API endpoints, given they are semantically distinct
from what we are trying to achieve.

We thus propose the creation of a `/v1/endpoint/object` API endpoint. This route
Copy link

Choose a reason for hiding this comment

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

#### Creating a new "Object Store"

* The user clicks on *Create*
* A modal dialog is shown, with the various Object Endpoint related fields:
Copy link

Choose a reason for hiding this comment

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

These fields are taken from https://github.com/longhorn/longhorn-manager/pull/2136/files#diff-c75a796f2c69c35ee5ab6293f23db5c8d5af9a241f4f2345e96b1fc41d72ddceR55

Is it necessary to request all that fields from the user or can we hardcode some of them?

@votdev votdev force-pushed the lep-20230430-object-endpoint branch from ac2494e to 75de654 Compare October 12, 2023 08:43
m-ildefons added a commit to m-ildefons/longhorn that referenced this pull request Oct 30, 2023
- Add config options and flags for object gateway
- Add CRDs for the object endpoint
- Regenerate depoy/longhorn.yaml

Related-to: longhorn#5832
Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this pull request Nov 2, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this pull request Nov 9, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this pull request Nov 17, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this pull request Nov 20, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
m-ildefons added a commit to m-ildefons/longhorn-manager that referenced this pull request Nov 29, 2023
longhorn/longhorn#5832

Signed-off-by: Moritz Röhrich <moritz.rohrich@suse.com>
@innobead
Copy link
Member

Closing first, and this will be revisited in 1.7.

@innobead innobead closed this Dec 12, 2023
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.

None yet

7 participants