NETOBSERV-764: Loki v1beta2 model follow-up #474
Conversation
|
@jotak: This pull request references NETOBSERV-764 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. In response to this:
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. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #474 +/- ##
==========================================
+ Coverage 55.00% 55.20% +0.20%
==========================================
Files 47 49 +2
Lines 6460 6441 -19
==========================================
+ Hits 3553 3556 +3
+ Misses 2667 2645 -22
Partials 240 240
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
jotak
added
the
ok-to-test
|
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:aec9c8d make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-aec9c8dOr as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-aec9c8d
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
github-actions
bot
removed
the
ok-to-test
| LokiModeManual LokiMode = "Manual" | ||
| LokiModeLokiStack LokiMode = "LokiStack" | ||
| LokiModeMonolithic LokiMode = "Monolithic" | ||
| LokiModeMicroservices LokiMode = "Microservices" |
There was a problem hiding this comment.
Why these should be camel case when we use uppercase for agent type, deployment mode, HPA status, Loki auth, SASL, Exporter type ?
There was a problem hiding this comment.
cf #394 (comment)
We did it wrong on the others.
Now we have a mix, we should eventually make them all consistent, but from now on, like we did on Features, we should adopt the correct convention
There was a problem hiding this comment.
so to avoid having a mix I would suggest to fix everything at once for v1beta2 since it's already an API change. WDYT ?
There was a problem hiding this comment.
I agree to do it before releasing v1beta2, but rather on a different PR to not mix it up
There was a problem hiding this comment.
I created a task: https://issues.redhat.com/browse/NETOBSERV-1374
We should add there everything that needs to be cleaned up before we do the first release, ie. any breaking change that we might still find before 1.5
| if len(spec.LokiStack.Namespace) > 0 { | ||
| dotNamespace = "." + spec.LokiStack.Namespace | ||
| } | ||
| gatewayURL := fmt.Sprintf("https://%s-gateway-http%s.svc:8080/api/logs/v1/network/", spec.LokiStack.Name, dotNamespace) |
There was a problem hiding this comment.
We could fallback on FlowCollectorSpec.namespace here to have something easier to read and consistent
There was a problem hiding this comment.
this would add a dependency on spec.namespace in the function input, which would break the logic in Convert_v1beta2_FlowCollectorLoki_To_v1alpha1_FlowCollectorLoki (https://github.com/netobserv/network-observability-operator/pull/474/files#diff-23e9a27bd609a9414ced41fdaf8063ade03a51b3154d85ad4edfdae228dd5531R139-R150) where we need to run this conversion only with v1beta2.FlowCollectorLoki as an input, not the whole spec. So removing the dependency on spec.namespace actually helps there.
|
I added the changes to automatically create the roles/binding for reading/writing loki logs (these things: https://github.com/netobserv/documents/blob/main/examples/loki-stack/role.yaml ) |
| { | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: constants.LokiCRReader, | ||
| }, | ||
| Rules: []rbacv1.PolicyRule{{ | ||
| APIGroups: []string{"loki.grafana.com"}, | ||
| Resources: []string{"network"}, | ||
| ResourceNames: []string{"logs"}, | ||
| Verbs: []string{"get"}, | ||
| }}, | ||
| }, |
There was a problem hiding this comment.
Should we put that guy on console plugin side when enabled ?
Also the role binding only mention the writer, not the reader 🤔
There was a problem hiding this comment.
It's actually not related to the console plugin, there is no binding that we use for reading (see here https://github.com/netobserv/documents/blob/main/examples/loki-stack/role.yaml it's the same: no reader binding) - the reader binding would have been useful in the HOST mode but since it's deprecated we don't worry about it).
The reader role is really just provided for convenience, so that users can just run something like :
oc adm policy add-cluster-role-to-user netobserv-reader user1
for multi-tenancy.
I've put it in code next to the writer role so that they're grouped together, although I agree that it doesn't relate to flp. As an alternative I can create a new loki package for that
There was a problem hiding this comment.
(done creating a loki package via 4d5bc3e)
|
@jotak: This pull request references NETOBSERV-764 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. In response to this:
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. |
|
@jotak: This pull request references NETOBSERV-764 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. In response to this:
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. |
|
@jpinsonneau the update-bundle check fails apparently because I'm using a different version of [edit] no worries, fixed in last commit |
|
Can you please remove |
API changes: - Improve default values in monolithic and microservices modes to stick with our guides (e.g. http://loki-distributor:3100/ as the ingester URL) - Rename modes so that they match upstream Loki naming - In microservices mode, the querier URL isn't optional - Add ability to configure tenant in these modes - Add documentation in places where it was missing; fix some typos - Strong-type LokiMode; set the enum a CamelCase as this is recommended; set it as a unionDiscriminator Implementation changes: - Use a "Hub" struct to convert any Loki mode into something alike Manual config; this avoids having to define many getter helpers each doing a switch/case. - Adapt some tests to better cover nominal use case, e.g. certificates_test now uses LokiStack mode
@jpinsonneau done |
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:3196567 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-3196567Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-3196567
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
API changes:
Implementation changes:
Also, when a lokistack is referenced, the corresponding role YAML is now automatically be created/updated. It sets the correct FLP service account depending on whether kafka is used.
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.