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

Add logging support to operator - #748

Merged
rfratto merged 9 commits into
grafana-cold-storage:mainfrom
rfratto:operator-logs-daemonset
Aug 19, 2021
Merged

Add logging support to operator#748
rfratto merged 9 commits into
grafana-cold-storage:mainfrom
rfratto:operator-logs-daemonset

Conversation

@rfratto

@rfratto rfratto commented Jul 16, 2021

Copy link
Copy Markdown
Contributor

PR Description

Completes logging support for the operator.

Which issue(s) this PR fixes

Closes #628.

Notes to the Reviewer

No docs changes, I'll do those in a follow up PR.

I found a few issues. Once this is merged I'll open issues and separate PRs for each.

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

Comment on lines -156 to -157
// +kubebuilder:resource:path="pod-logs"
// +kubebuilder:resource:singular="pod-logs"

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.

The hyphens really aren't how k8s expects you to refer to multi-word names (daemonsets, not daemon-sets). I only changed this for podlogs but I suspect we'll want to do the same for grafana-agent, prometheus-instance and logs-instance in a later PR.

}

// CreateOrUpdateDaemonSet applies the given DaemonSet against the client.
func CreateOrUpdateDaemonSet(ctx context.Context, c client.Client, ss *apps_v1.DaemonSet) error {

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've said it before and I'll say it again: generics can't come fast enough :)

Comment thread pkg/operator/operator.go
Comment on lines +123 to +128
Owns(applyGVK(&apps_v1.DaemonSet{})).
Owns(applyGVK(&core_v1.Secret{})).
Owns(applyGVK(&core_v1.Service{})).
Watches(watchType(&core_v1.Secret{}), events[resourceSecret]).
Watches(watchType(&grafana_v1alpha1.LogsInstance{}), events[resourceLogsInstance]).
Watches(watchType(&grafana_v1alpha1.PodLogs{}), events[resourcePodLogs]).

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.

The diff here is kind of gnarly because I sorted everything.

Comment on lines +123 to +139
if len(d.Prometheis) == 0 {
key := types.NamespacedName{Namespace: svc.Namespace, Name: svc.Name}

var service core_v1.Service
err := r.Client.Get(ctx, key, &service)
if k8s_errors.IsNotFound(err) {
return nil
} else if err != nil {
return fmt.Errorf("failed to find stale Service %s: %w", key, err)
}

err = r.Client.Delete(ctx, &service)
if err != nil {
return fmt.Errorf("failed to delete stale Service %s: %w", key, err)
}
return nil
}

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.

This is the only new logic in this function.

Comment on lines +167 to +171
// Don't generate anything if there weren't any instances.
if len(d.Prometheis) == 0 {
continue
}

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.

This is the only new logic in this function.

@rfratto rfratto changed the title (Draft) Add logging support to operator (WIP) Add logging support to operator Jul 16, 2021
//
// Note that stages is a string and because SIG API Machinery does not support
// recursive types. Be careful not to mistype anything.
Stages string `json:"stages,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.

I tried a few different ways to avoid this, but this really seems to be the only sane way to do it without creating more CRDs.


// createConfigurationSecret creates the Grafana Agent configuration and stores
// it into a secret.
func (r *reconciler) createConfigurationSecret(

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.

Moved to reconciler_metrics.go.


// createGoverningService creates the service that governs the (eventual)
// StatefulSet. It must be created before the StatefulSet.
func (r *reconciler) createGoverningService(

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.

Moved to reconciler_metrics.go.

}

// createStatefulSets creates a set of Grafana Agent StatefulSets, one per shard.
func (r *reconciler) createStatefulSets(

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.

Moved to reconciler_metrics.go.

@rfratto
rfratto force-pushed the operator-logs-daemonset branch from 9b72261 to d637120 Compare July 20, 2021 18:08
@rfratto

rfratto commented Jul 20, 2021

Copy link
Copy Markdown
Contributor Author

This is ready for review now as an initial implementation. There are some known issues but given the size of this PR I want to do the fixes separately. Ditto with docs.

@rfratto
rfratto marked this pull request as ready for review July 20, 2021 18:09
@rfratto
rfratto requested a review from mattdurham July 20, 2021 18:09
@rfratto rfratto changed the title (WIP) Add logging support to operator Add logging support to operator Jul 20, 2021
@rfratto

rfratto commented Jul 20, 2021

Copy link
Copy Markdown
Contributor Author

(Because I reverted the logs fix in 57ecfb3 this will work but there will be a lot of errors about not being able to save the positions file)

Comment thread cmd/agent-operator/agent-example-config.yaml Outdated
Comment thread cmd/agent-operator/example-grafana.yaml
Comment thread pkg/operator/clientutil/clientutil.go Outdated
Comment thread pkg/operator/config/templates/ext/marshal.libsonnet Outdated

// intoStages converts the a yaml slice of stages into a Jsonnet array.
func intoStages(i []interface{}) (interface{}, error) {
text, ok := i[0].(string)

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.

Do we need to protect against 0 length arrays?

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.

Nah, jsonnet will prevent that function from being called if you don't have an argument matching for the list of params passed to vm.NativeFunction.

@mattdurham mattdurham 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.

Looks good, mostly nits/opinions, changing the version and the loki naming as the primary items.

@rfratto
rfratto force-pushed the operator-logs-daemonset branch from 42d92d6 to 45364f4 Compare August 19, 2021 18:00
@rfratto
rfratto merged commit ad0b414 into grafana-cold-storage:main Aug 19, 2021
@rfratto
rfratto deleted the operator-logs-daemonset branch August 19, 2021 18:32
mattdurham pushed a commit that referenced this pull request Nov 11, 2021
* deploy daemonsets for logs

* changelog entry

* working example e2e

* undo docs changes until a future PR

* defer logs fix for a future PR

* remove duplicated intoStages code

* undo base rebase change

* undo accidental commit

* review feedback
@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 9, 2024
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 9, 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.

Proposal: logging support in the Grafana Agent Operator

4 participants