NETOBSERV-219 PoC Operators #59
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jpinsonneau
force-pushed
the
operators
branch
from
a62325b
to
d1988ab
Compare
jpinsonneau
force-pushed
the
operators
branch
from
dc9c06c
to
ff9be54
Compare
jpinsonneau
force-pushed
the
operators
branch
from
ff9be54
to
27db3a4
Compare
There was a problem hiding this comment.
Correct me if I am wrong here but my understanding is that each operator can only be deployed per cluster once.
Because of this, I think that the dependency is too strong :
- what happens if the operator is already deployed in the cluster
- if we deploy the operator and someone else create a second resource instance for it, this resource will get destroyed if we remove the flowcollector object
I think managing the instances like you did is great but the operator part raise many concerns.
And just for my understanding, did we try using the operator dependency feature?
You can actually install multiple times the same operator in different namespaces. The only situation I didn't tested is if the operator is globally installed. Then maybe the subscription in our namespace will fail. Here is an example with grafana operator:
That's look great ! Thanks for sharing. I will test that ASAP. |
Ok, I did not know we could restrict an operator to some managed namespace. Thanks for this! This solve many of my concerns. |
There was a problem hiding this comment.
@jpinsonneau a question ... if I change the CR adding or removing an operator from the list ... with that operator also will be uninstalled ???
For now no since I wasn't sure about installing the operators in the same namespace. |
|
I tried to import all operators configs but there is some limitations on CRD size so we should add only needed configs
The operators + instances will be created as soon as corresponding Example: grafana:
instanceSpec:
config:
security:
admin_user: admin
admin_password: admin
ingress: true
dashboardSpec:
configMapRef:
name: grafana-dashboard
key: json
optional: true |
|
I'm back on testing this, sorry for the delay! Maybe it's just the time that the custom loki catalog gets deployed ? |
|
Similar for grafana: |
|
/hold |
|
Seems like Loki fails to deploy for me. My LokiStack: Maybe that's due to the early state of the loki operator? |
|
I changed the storageClassName back to gp2 but it didn't make it to the LokiStack, it's still showing standard |
|
Also, it seems like the created operators are not removed after removing the CR, or editing it to disable them |
|
@jotak here are some answers:
On AWS using
Yes ! Error status are not revelent at all and it just crashs if your storage / bucket is not correctly set. You then need to manually uninstall
My initial plan was to use global operators. I changed this to our operator namespace: |
|
New changes are detected. LGTM label has been removed. |
jpinsonneau
force-pushed
the
operators
branch
2 times, most recently
from
b776321
to
70f019d
Compare
|
|
||
| // install operators if main instance is required | ||
| // dependent objects will be ignored if InstanceSpec is nil | ||
| if target.Spec.Loki.InstanceSpec != nil { |
There was a problem hiding this comment.
Same comment than here #78 (comment) :-)
We need an explicit flag, due to restrictions when the CR is created from a console form (not yaml). Checking nil pointers won't work (they'll never be nil).
By the way, I haven't checked in deep if the Kafka config is consistent with what @OlivierCazade is doing here: #78 , do you know? Things like TopicName will be defined in just one place, right?
There was a problem hiding this comment.
There is not much difference between the configs. I'll adapt after #78 merge
|
Added |
|
@jpinsonneau: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| } | ||
| } | ||
|
|
||
| func grafanaSubscription(name string, namespace string) *ov1alpha1.Subscription { |
There was a problem hiding this comment.
As discussed in slack, I think it's necessary for productization to make these subscriptions optional, and configurable, in order to adapt upstream vs downstream.
I think a good option would be to externalize these as Subscription yaml resources, that will be easily adaptable between upstream and downstream.
As a consequence, it must be considered in code that subscriptions can potentially be missing (e.g.: no community loki operator), which should not be considered an error.
catch sigterm and exit go routines cleanly
JIRA: https://issues.redhat.com/browse/NETOBSERV-219
This PoC allow strimzi, loki and grafana operators install + instances creation simply specifying operator(s) name(s) in FlowCollectorSpec.Operators
It create a Subscription using operator-framework api that will deploy the operator and then create the kafka instance using strimzi-client-go, the strimzi api interface generated with crd-codegen. It also allow us to manipulate
KafkaUser,KafkaTopicand so on.Since loki operator is not yet available in a publicCatalogSource, a custom one containing an updated version of the operator has been released:namespace has been replaced by network-observabilitygateway has been removedauth has been disabledThe reconcilier will add this source to the namespace if it doesn't exists. We could use that to tests our operator releases before publishing=> Loki Operator has been released in the
redhat-operatorscatalog.Tenant-IDand TLSInsecureSkipVerifyhas been added to be able to connect until https://issues.redhat.com/browse/NETOBSERV-309 implementation.Check netobserv/network-observability-console-plugin#147
/!\ you need to create the aws bucket before creating
FlowCollectorinstance:You can check deploy-example-secret.sh for infos