Skip to content
This repository was archived by the owner on Jul 28, 2026. It is now read-only.

[dev.operator-integrations] Add Integration into operator resource hierarchy - #1417

Merged
rfratto merged 6 commits into
grafana-cold-storage:dev.operator-integrationsfrom
rfratto:integrations-crds
Mar 2, 2022
Merged

[dev.operator-integrations] Add Integration into operator resource hierarchy#1417
rfratto merged 6 commits into
grafana-cold-storage:dev.operator-integrationsfrom
rfratto:integrations-crds

Conversation

@rfratto

@rfratto rfratto commented Feb 23, 2022

Copy link
Copy Markdown
Contributor

PR Description

This PR is a reimplementation of #1288 given the recent changes to the operator integrations RFC (#1224).

It introduces one new CRD, Integration, which defines an integration to run. The new CRD is injected into the resource hierarchy and discovered during reconcile, which is verified by a test.

Additional changes:

  • pkg/operator/config.Deployment has been moved to pkg/operator/apis/monitoring/v1alpha1.Deployment. This allows us to take advantage of the DeepCopy function generation, which was previously manually maintained.
  • .gitattributes has been modified to mark CRDs as generated so they don't inflate the size of diffs during PR review, though I think this will only change future PRs and not this one.
  • Imports to pkg/operator/apis/monitoring/v1alpha1 are now always aliases to gragent for consistency. gragent was picked for its brevity and should be mostly grokkable by readers.

Which issue(s) this PR fixes

Related to #1414.

Notes to the Reviewer

While the new resources are discovered during reconcile, nothing is done with them at the moment. Incorporating into the reconcile and deploying the integrations will be done over a few follow up PRs.

PR Checklist

  • CHANGELOG updated (N/A)
  • Documentation added (N/A)
  • Tests updated

…1alpha1

This allows us to easily generate the DeepCopy implementation for the
struct instead of manually creating it.

This commit also makes gragent the consistent name to refer to
pkg/operator/apis/monitoring/v1alpha1, which is short and
understandable.
@rfratto
rfratto requested a review from captncraig February 23, 2022 19:44
Comment thread .gitattributes
@@ -0,0 +1 @@
pkg/operator/crds/*.yaml linguist-generated=true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will impact future PRs, but not this one 🤔

Comment on lines +12 to +21
// Label selector to find Integration resources to run. When nil, no
// integration resources will be defined.
Selector *metav1.LabelSelector `json:"selector,omitempty"`

// Label selector for namespaces to search when discovering integration
// resources. If nil, integration resources are only discovered in the
// namespace of the GrafanaAgent resource.
//
// Set to `{}` to search all namespaces.
NamespaceSelector *metav1.LabelSelector `json:"namespaceSelector,omitempty"`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are called instanceSelector and instanceNamespaceSelector for metrics and logs, so this is inconsistent with that.

OTOH, those refer to selecting MetricsInstances and LogsInstances, so it might make sense to drop the prefix here.

Comment thread pkg/operator/apis/monitoring/v1alpha1/types_integrations.go
@rfratto
rfratto requested a review from rlankfo February 28, 2022 14:46

@captncraig captncraig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - My biggest concern is usability with the config and volumes and things being pretty free form fields.

It will probably be hard for users to know how to use this without pre-built snippets for each particular integration in the docs.

But no major design objections from me.

Comment thread pkg/operator/apis/monitoring/v1alpha1/types_integrations.go Outdated

@rlankfo rlankfo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1

@rfratto
rfratto enabled auto-merge (squash) March 2, 2022 15:03
@rfratto
rfratto merged commit 6147a1e into grafana-cold-storage:dev.operator-integrations Mar 2, 2022
@rfratto
rfratto deleted the integrations-crds branch March 2, 2022 15:35
rfratto added a commit to rfratto/agent that referenced this pull request Apr 12, 2022
…erarchy (grafana-cold-storage#1417)

* add CRDs for integrations

* move pkg/operator/config.Deployment to pkg/operator/apis/monitoring/v1alpha1

This allows us to easily generate the DeepCopy implementation for the
struct instead of manually creating it.

This commit also makes gragent the consistent name to refer to
pkg/operator/apis/monitoring/v1alpha1, which is short and
understandable.

* Mark CRDs as generated to hide their diffs from GitHub PR reviews

* test Integrations in resource hierarchy

* go mod tidy

* fix doc error for ConfigMaps
rfratto added a commit that referenced this pull request Apr 12, 2022
* [dev.operator-integrations] Add Integration into operator resource hierarchy (#1417)

* add CRDs for integrations

* move pkg/operator/config.Deployment to pkg/operator/apis/monitoring/v1alpha1

This allows us to easily generate the DeepCopy implementation for the
struct instead of manually creating it.

This commit also makes gragent the consistent name to refer to
pkg/operator/apis/monitoring/v1alpha1, which is short and
understandable.

* Mark CRDs as generated to hide their diffs from GitHub PR reviews

* test Integrations in resource hierarchy

* go mod tidy

* fix doc error for ConfigMaps

* [dev.operator-integrations] pkg/operator/config: Generate config for integrations (#1503)

* pkg/operator/config: Support integration-specific configs

* document why we want to skip adding a __replica__ label for integration metrics

* [dev.operator-integrations] Reconcile operator integrations (#1556)

* pkg/operator: refactor pod template generation into its own function

Extract the generation of pod templates out into its own function: the
implementation of generating the DaemonSetSpec (for logs) and
StatefulSetSpec (for metrics) was incredibly similar, and this will be
doubly true for integrations. A shared function for generating the
template will help remove duplication and avoid bugs where template
generation becomes out of sync for the various generated resources.

* pkg/operator: move more common logic to generatePodTemplate

* pkg/operator: make generating pod templates its own function

* pkg/operator: share generated ObjectMeta between pod controllers

* pkg/operator: reconcile integrations

* pkg/operator: resolve lint errors

* pkg/operator: e2e test deployment of integrations

* docs: update docs for RBAC additions

* pkg/operator: fix lint errors

* pkg/operator/apis: fix "order resources" typo (other resources)

* pkg/operator: elaborate why deleteManagedResource gets called

* update changelog
@github-actions github-actions Bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 3, 2024
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants