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

feat: creates consumers if not present in NATS #88

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

Conversation

danielcibrao-form3
Copy link
Contributor

@danielcibrao-form3 danielcibrao-form3 commented Oct 26, 2022

The jetstream-controller its an operate that should ensure that a system is in a given state. During our tests we found out that if a consumers or stream are deleted via code or through nats-box they are not created again by the controller.

From my understanding this is happening because we are checking if the observed generation number and generation number match in the CRD, which on this case (a manually delete consumer) will not change because the CRD was not changed. The following PR aims to resources (consumers and streams) being created in NATS in case they are not found by the client but there is a CRD for it.

@danielcibrao-form3 danielcibrao-form3 force-pushed the dc-feat-create-consumers branch 2 times, most recently from cc0f5c5 to 0626b91 Compare October 26, 2022 15:24
@caleblloyd
Copy link
Contributor

I don't think this really makes sense, resources managed by NACK should not be deleted out from under NACK. The operator typically only responds to changes in K8s resources, not changes in the underlying managed resources.

If it were to reconcile changes, there would need to be a loop that runs at a timed period to check to see if any managed resources have drifted.

If a resource should be force-reconciled, a metadata.annotation could be added:

metadata:
  annotations:
    lastUpdated: "<put the date/time or some other unique string here>"

@danielcibrao-form3
Copy link
Contributor Author

danielcibrao-form3 commented Nov 2, 2022

Hey @caleblloyd thanks for the review and quick response. I understand that mere idea of having something outside of nack deleting a resource (stream/consumer) does not make sense, since nack created those and everything should go through it.

Please allow me to layout what I am trying to achieve this way, when I create a consumer from a CRD I am excepting that consumer to exist in the underlying system. If something deletes that consumer (could be nats-server or a client by mistake), i am expecting my operator to ensure that it maintains the desired state, which is to have that consumer present in NATS.

Most of the operator I know work this way. They make sure that the state is continuously reconciled, and it is working according to what is defined in the CRD. Even K8s Native controllers work that way. I was expecting the same behaviour out of the nats k8s operator.

To provide even more context, this PR come out because our consumers were being deleted from nats-server and we were not sure why, if the operator continuously reconciled the state, it would make things easier.

@caleblloyd
Copy link
Contributor

The continuous reconcile loop would need to be put into the PR for this to be effective then, I believe. I don't think this loop runs on a timer right now. I think it is triggered by events on the CRDs.

@danielcibrao-form3
Copy link
Contributor Author

I am going to that change as well :). As soon as it is ready I will let you know.

The jetstream-controller its an operate that should ensure that a system
is in a given state. During our tests we found out that if a consumers
or stream are deleted via code or through `nats-box` they are not
created again by the controller.

From my understanding this is happening because we are checking if the
observed generation number and generation number match in the CRD, which
on this case (a manually delete consumer) will not change and thus not
trigger a recreation.

The following PR aims to resources (consumers and streams) being created
in NATS in case they are not found by the client but there is a CRD for it.
@tkeller-moxe
Copy link

This is a feature I was expecting nack todo as well as it is how other operators tend to work. Would love to see this implemented!

@mloiseleur
Copy link

I am also interested in this feature. Anything missing in order for this PR to be considered ?

@davidovich
Copy link

see also #95 with comment suggesting adopting the controller runtime.

@DiogoRoloOS
Copy link
Contributor

Is there any blocker in merging this?

@ChristianGmw
Copy link

Any update here? Would love to see this PR 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.

7 participants