RealtimeAPI CRD

[miguelvr]

checklist:

• run make test and make lint
• test manually (i.e. build/push all images, restart operator, and re-deploy APIs)

[RobertLucian]

We should also confirm the following 2 things:

1. That updating the requested replicas for an API (through the tweaking of min_replicas/max_replicas) has the same behavior as what's currently on master.
2. That re-deploying an API (realtime) that hasn't had its API spec updated doesn't trigger a rolling-update process, as it's normally expected from the refresh command.

[miguelvr]

You removed the kubebuilder default annotations I had. Those were intentional and necessary for the controller to work well. Please revert

[RobertLucian]

@miguelvr the Kubebuilder default annotations were removed for a good reason. With them in there, there was a decent chance that if the user had set a value to 0 (say min_replicas for instance, could be any other that allows 0 as a value), then the controller would step in and give it the default value, which is really bad because the operator would no longer be able to download the API spec - they would no longer match because the controller deliberately changed the spec.

And couldn't just double-comment them because they would then appear in the description of each field, which is also bad. They are definitely necessary if we weren't to have the CLI/operator and if the user were to create the RealtimeAPI resource(s) directly, yes. But until then, I'm not seeing a good reason for having them around, not as long as they also break the functionality.

The controller seems to be working well, no issues with it (as of this moment). Where are these annotations required?

[miguelvr]

There's no chance the user has to set anything because the operator defaults everything. By removing them, the controller does not work properly without the operator. This is not good for different reasons, but the main one is development. I spent quite a while figuring out the correct defaulting scheme

[RobertLucian]

Sure, the operator defaults, but that doesn't matter as long as it still sets the value to zero (whichever field that is). The outcome is that the functionality is broken. It just doesn't work. Tested it and it fails.

By removing them, the controller does not work properly without the operator.

Where exactly does the controller not work properly? I have tested it and it seems to work well. Could you please provide a granular explanation as to where this is required?

I spent quite a while figuring out the correct defaulting scheme

Don't see why they would be required as long as we don't do require the users to create their realtimeapi deployments directly. Could you expand on this one?

Pertaining to the above paragraph, we concluded last night that we wouldn't be expecting the user to create a realtimeapi resource directly using a subset of the available fields in the api spec. We expect the operator to be the sole creator of these resources (for now). This becomes important only when we'd expect the user to do this (i.e. if we were to remove the operator/cli).

[miguelvr]

The controller is 100% independent from the "cortex operator" it shouldn't rely on specific defaults to be set. Therefore it needs to work by applying a Kubernetes manifest. This is how the development of the controller should look like.

If there are no proper defaults set, the controller will try to access stuff that is expected to be set and it will lead to nil pointer dereferences. Tried this and saw it failing.

Whatever you saw failing, it needs to be fixed with the correct annotations, not by removing all the defaulting ones

[miguelvr]

In the case of zeros being allowed, then a pointer should be used to avoid the defaulting.