wip/object endpoint

[m-ildefons]

Longhorn Manager: Object Endpoint Controller

Implement an object endpoint controller for managing s3gw instances through Longhorn CRDs and the Longhorn UI.

Related: https://github.com/aquarist-labs/s3gw/issues/470

[jecluis]

I can see there's a DCO missing somewhere -- the bot says so. Probably better to check that out.

I'll try to review this today. :)

[jecluis]

Generally looks good. Some questions, mostly.

[jecluis]

Am I misunderstanding this, or is this function just a stub?

[m-ildefons]

This is just a stub. I should have put a // TODO here.

This function is executed for example, when you do a kubectl edit objectendpoint foobar-endpoint, or through the "Edit" function in the UI. But I'm starting to doubt that this is a good idea at all. Maybe we should just forbid editing object endpoints and make the user delete/recreate them? The problem is that there isn't much to edit and what's there probably shouldn't be editable, since I have no idea how to sanely respond to something like the storage class changing? I could make the controller re-deploy on a new volume, but do I have to migrate the data? What if the size is changed and the new size is insufficient for the data?
There are lots of corner cases in this discussion that I don't yet know how to handle, so at the moment I'm inclined to say that you just can't edit an object endpoint.

[jecluis]

Can we choose what we allow to be edited? E.g., how will we enable debug on the service if we need to? Can't it be through an update to the endpoint?

[m-ildefons]

There are admission webhooks that can validate manifests submitted to the API based on pretty arbitrary rules and we could probably implement that to make sure nothing nonsensical is submitted.
Failing such a webhook will result in an error on the client side. For the UI we can (and should) design the dialogues such that it's impossible to do the wrong thing.
But that precludes that there actually is a sensible way to edit an objectendpoint, which I'm less and less sure about the longer I think about it.

[jecluis]

Okay, so my follow up question, albeit tangential to this bit, is: how would we change something like debug levels on the endpoint?

[m-ildefons]

You can always do a kubectl edit on the respective deployment, assuming that the controller doesn't have logic built in to revert that change (which it currently doesn't have).

But offering that capability through the CRD would require this kind of option to be modeled in the CRD of course. Then the controller would need logic to adapt the deployment/etc, when the generation of the object endpoint K8s object changes.

[irq0]

If this doesn't actually edit, is there a return type like ENOTSUP?

[m-ildefons]

Unfortunately there isn't. This API is based on the assumption that only supported and implemented functionality is exposed as API endpoint.

[jecluis]

This formatting seems unnecessary. Was this a result of gofmt or something?

[m-ildefons]

I broke the long line, so I could better see where I should put the additional argument

[jecluis]

I get that helps, but it also unnecessarily changes the formatting of the code around you. It doesn't look like one argument per line is the format being used. Do you know what happens if this code is run through gofmt or the likes? I'd imagine the code base to be automatically formatted.

[m-ildefons]

I run gofmt every time I save in my editor, so I guess it's fine by that, but I'll revert this if it creeps you out.

[jecluis]

I don't particularly care. It's just one of those things that I imagine could irk someone reviewing it against the original code base.

[m-ildefons]

I know that formatting-only changes are bad practice, but I figured since there is an actual code change here, I may as well. But now I've put back the long lines, so it's kinda moot anyways.

[jecluis]

What's the semantic of Done() here? Should it be different depending on whether it's being called after we have actually run through oec.syncObjectEndpoint() below, or whether it's being called due to quit being true?

[m-ildefons]

I think you're missing some context here:
https://pkg.go.dev/k8s.io/client-go/util/workqueue
https://go.dev/ref/spec#Defer_statements

This is the only place where an item is removed from the queue. Done() does this if the item is not marked as dirty again during processing, or if the item may be retried:

Done marks item as done processing, and if it has been marked as dirty again while it was being processed, it will be re-added to the queue for re-processing.

Forget() works differently, as it marks an item as not-to-be-retried, but doesn't remove it from the queue just yet.

Forget indicates that an item is finished being retried. Doesn't matter whether it's for perm failing or for success, we'll stop the rate limiter from tracking it. This only clears the rateLimiter, you still have to call Done on the queue.

So the logic here is:

if quit:

• leave the queue as-is and return
  else:
• process item (this possibly marks the item as non-retry-able or as dirty)
  if the item is dirty: add it back to the queue
  else if the item failed but can be retried: add it back to the queue
  else: mark the item as finished processing

[jecluis]

I think you're missing some context here: https://pkg.go.dev/k8s.io/client-go/util/workqueue https://go.dev/ref/spec#Defer_statements

I was missing the workqueue context, for sure. In case of defer, that was just a misunderstanding of how the compiler understands sequence of defer statements, which actually took me writing a small program to validate that my understanding was wrong ;)

So the logic here is:

if quit:

* leave the queue as-is and return
  else:

* process item (this possibly marks the item as non-retry-able or as dirty)
  if the item is dirty: add it back to the queue
  else if the item failed but can be retried: add it back to the queue
  else: mark the item as finished processing

Gotcha. Thanks :)

[jecluis]

This is definitely not obvious. If we're calling on a function named handleError(), one expects this function to be dealing with an error. It seems however that what we're doing is handling state from processNextWorkItem() that should be dealt there instead.

I mean, if err == nil should be validated before calling this function. At most this function should assert to ensure error != nil.

[m-ildefons]

I found this confusing too, but stuck with it as this is consistent with the other controllers.
I guess the reasoning here is that handleError handles the content of the error return value regardless of what that is.

There is good reason to do it this way, as we're relying on the error return value to tell us if we should re-try a work item or if we can finalize it and remove from the queue. And if we should finalize it as successful or as "all retries failed this error is final".

[jecluis]

But we're not doing any other logic on this function IF err == nil, so there's no point in doing that bit in this function. Regardless of whether other controllers do that, this is confusing, and adds nothing of real, tangible value.

Handling the error, IF err != nil would be handled the same way, to tell us if we should retry or not.

[m-ildefons]

So what you're suggesting is that we split handling the error value into two parts accross two functions:

func (oec *ObjectEndpointController) processNextWorkItem() bool {
	key, quit := oec.queue.Get()
	if quit {
		return false
	}
	defer oec.queue.Done(key)
	
	err := oec.syncObjectEndpoint(key.(string))
	if err == nil {
	  oec.queue.Forget(key)
  } else {
	  oec.handleError(err, key)
	}

	return true
}

[...]

func (oec *ObjectEndpointController) handleError(err error, key interface{}) {
	if oec.queue.NumRequeues(key) < maxRetries {
		oec.logger.WithError(err).Errorf("")
		oec.queue.AddRateLimited(key)
		return
	}

	utilruntime.HandleError(err)
	oec.logger.WithError(err).Errorf("")
	oec.queue.Forget(key)
}

This would make it even more confusing IMO, since now you have two places where we handle error values. And more importantly two places where we decide what to do with the item on the queue next. The handleError() function makes sure that based on the error value (which may be nil, indicating success) the item is retried or removed from the queue, I don't see why we should handle the success case in a different place and more importantly why we should spread the code for making the decision to re-queue over several places.
The point of this function isn't to handle only failures, it's to handle the re-queue decision based off of the value of err, which happens to be of type error but may also be nil.

[jecluis]

So what you're suggesting is that we split handling the error value into two parts accross two functions:

func (oec *ObjectEndpointController) processNextWorkItem() bool {
	key, quit := oec.queue.Get()
	if quit {
		return false
	}
	defer oec.queue.Done(key)
	
	err := oec.syncObjectEndpoint(key.(string))
	if err == nil {
	  oec.queue.Forget(key)
  } else {
	  oec.handleError(err, key)
	}

	return true
}

[...]

func (oec *ObjectEndpointController) handleError(err error, key interface{}) {
	if oec.queue.NumRequeues(key) < maxRetries {
		oec.logger.WithError(err).Errorf("")
		oec.queue.AddRateLimited(key)
		return
	}

	utilruntime.HandleError(err)
	oec.logger.WithError(err).Errorf("")
	oec.queue.Forget(key)
}

This would make it even more confusing IMO, since now you have two places where we handle error values.

No we don't. We have a place where we handle in case it's not an error, and a place where we handle in case it's an error. As far as I know, err == nil means "no error", so no error to be handled.

And more importantly two places where we decide what to do with the item on the queue next. The handleError() function makes sure that based on the error value (which may be nil, indicating success) the item is retried or removed from the queue, I don't see why we should handle the success case in a different place and more importantly why we should spread the code for making the decision to re-queue over several places. The point of this function isn't to handle only failures, it's to handle the re-queue decision based off of the value of err, which happens to be of type error but may also be nil.

Then rename the function. That is no longer handling an Error. It's doing something else.

[irq0]

Does handleErr add value to the reader? Suggest to pull the logic in processNextWorkItem. Having the queue logic in one place makes it easier for the reader to grok the whole thing

[m-ildefons]

Doing the queue item handling all in one place sounds reasonable. I've pulled the result handling logic into the processNextWorkItem function.

[jecluis]

is this a typo? controlleeHandler()?

[m-ildefons]

Nope, see controller/uninstall_controller.go:174

[jecluis]

Then yikes, that's a really weird name 🤷

[jecluis]

isn't this being called regardless of whether we hit an error condition or not?

[m-ildefons]

It is being called regardless, but depending on whether err is set or nil, we will return the set error or retain nil:

Wrapf returns an error annotating err with a stack trace at the point Wrapf is called, and the format specifier. If err is nil, Wrapf returns nil.

https://pkg.go.dev/github.com/pkg/errors#Wrapf

[jecluis]

ack

[jecluis]

I suspect this line should not be removed.

[m-ildefons]

Good spot

[jecluis]

context.TODO() ? Does this mean we need to work on this at some point?

[m-ildefons]

TODO returns a non-nil, empty Context. Code should use context.TODO when it's unclear which Context to use or it is not yet available (because the surrounding function has not yet been extended to accept a Context parameter).

https://pkg.go.dev/context#TODO

In this case, as in many other places in the code, the kubernetes client expects a context of some sort: https://pkg.go.dev/k8s.io/client-go/kubernetes/typed/core/v1#SecretInterface
This context is used to communicate deadlines, cancelation signals etc. We're already relying on the controller's queue to handle retries and errors correctly, so we don't need to handle those cases here, hence we just give an empty non-nil context.

[jecluis]

ack

[jecluis]

is this file automatically generated? If not, why are we removing stuff from things that don't seem related to our stuff at all?

[m-ildefons]

This is automatically generated. I'd have to try to track this down using the git history, but I trust that this is accurately generated. There is a slight possibility that things like version differences between my environment and the one used for generating the crds.yaml on the master branch caused the unexpected diff. Or that the file on master was edited by hand.

[jecluis]

gotcha

[m-ildefons]

Thanks for going through this and prompting me to go back and look up the exact details on several of the mechanisms at play here.
I'll fix the DCO signoff problem before creating a PR against the Longhorn repo as I also want to squash away some of the more minor commits

[jecluis]

Thanks for going through this and prompting me to go back and look up the exact details on several of the mechanisms at play here. I'll fix the DCO signoff problem before creating a PR against the Longhorn repo as I also want to squash away some of the more minor commits

I think it's reasonable to merge this into this repo and then open the PR from this branch. I suspect it will allow us to better track that. But whatever you prefer should work.

[irq0]

How is this thing tested?

There are a couple of functions without docstring. Suggest adding them. Many weren't obvious in what they do.

There were unused returns. Suggest running a suite of linters:

A good starting point would be golangci-lint --config=scripts/golangci.yml
with config from ceph-csi
https://github.com/ceph/ceph-csi/blob/devel/scripts/golangci.yml.in

[irq0]

Does handleErr add value to the reader? Suggest to pull the logic in processNextWorkItem. Having the queue logic in one place makes it easier for the reader to grok the whole thing

[irq0]

If this doesn't actually edit, is there a return type like ENOTSUP?

[irq0]

Is there a better name for this? Why separate function under almost the same name with little code?

[m-ildefons]

One function is an the API handler that creates an HTTP response for the REST API. The other is a private function that actually compiles the list, which is used by the websocket server directly, as it doesn't need to wrap the response with HTTP headers. Both do about the same, which is in this case return a list of storage classes.
The naming is consistent with the implementation for other API endpoints.

[irq0]

Check return code?

[m-ildefons]

for function() {} just loops function so long as function returns something truthy.

[irq0]

Yes and it does look simple.

How is error handling modeled here?
What does the bool return mean? End worker execution? An error that processNext.. could not handle occurred? If so, why let worker swallow that error and not bubble that up?

[irq0]

Nested logic tends to be harder to understand than necessary. On each nest the reader has to keep state.
Suggest denesting once (here and in the following):

if err != nil {
   return err
}
if dpl.Status... > 0 {
  return nil
}

[irq0]

This name is misleading. The return isn't RO, it "suggests" a behavior to the caller. Suggest renaming it mutable or making it the default and letting the caller make a choice of copy/no copy

[m-ildefons]

I'm sticking to the naming convention with the rest of the datastore here, where the RO suffix implies that the returned object should be used read-only and no suffix implies a deep copy taking place.

[irq0]

Suggest adding line breaks

[m-ildefons]

This file is generated.

[irq0]

-a

[irq0]

Failure states? Starting -> Stopping possible?
Starting -> unknown possible?

[irq0]

Why does sorted deserve its own function? What is special about the sort?
Suggest to at least add the info to the name by what this is sorted

[m-ildefons]

The datastore (i.e. the cache of K8s objects) keeps the data as a map of keys to objects. Keys are normally just the names of the K8s objects but can be something else as well as they must of course be unique. But they are always string like data. This means however that the datastore has a canonical notion of order between two objects, which is just the order of the keys.
To preserve the order when querying the datastore for a list of all objects endpoints the endpoints are put into the return value sorted by their keys.
This is different from just generally requesting a list of object endpoints as an "unsorted" list function would not guarantee the order of object endpoints in the list to be stable between two invocations.
The function name indicates that the returned list is sorted by "something", implying that the order of object endpoints will be stable between invocations at the cost of the sorting operation.

The overarching goal here is to let the API return consistent results. Keep in mind that there may be multiple instances of the longhorn-manager. Each instance may keep the contents of the datastore laid out differently in memory and K8s can decide to route different API requests to differnt instances. So not only does this make the result of the listing API consistent between two consecutive calls to the same longhorn-manager instance, it also makes it consistent between two different instances.
While it's not impossible to remove this guarantee from the API and just let the frontend bring the data into a consistent format, it reduces the probability of 'glitchy' corner cases in the frontend.

I'd be fine with adding a comment, but the name of the function itself is just fine as is. It returns a somehow-sorted list of object endpoints.

[irq0]

The second paragraph raises some questions.
What is a glitchy corner case and how are they handled?
Can duplicates occur in the result?
What if two manager instances disagree? Do they disagree on order or also on entries?

Is this function exported as RPC?

[m-ildefons]

I'm thinking about the likes of the following "glitchyness":
Reloading the same page of the UI repeatedly and the order changing at random. This would e.g. be annoying if you have so many object endpoints that you need pagination to display the list. Then adding or deleting one (implicitly reloading the list) could totally change which endpoints end up on the page you're viewing. With sorting, that shifts at most by one, which is expected as well. The point of making sure this function has as much consistency in its behavior as possible is, that the UI then doesn't need to handle inconsistencies, i.e. you don't need to make sure the UI sorts the data before displaying it.

Duplicates can not occur as a single request is routed-to and handled-by one instance only.
Sorting will ensure that the instances don't disagree on order.
The source of truth in all cases is the K8s API. Each longhorn-manager instance keeps an in-memory cache of the objects in the K8s API and updates that itself. This of course means that there may be a brief time-window when the managers disagree on entries.

This function is indirectly exported as RPC. It is what backs the "listing" functionality, i.e. when you send a GET request to the /v1/objectendpoints API endpoint, this function will compile the list of object endpoints that will then be marshalled into a JSON and put into the response's body.