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.

Sign up for GitHub

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

Jump to bottom

Add logging support to operator #748

Merged
merged 9 commits into from
Aug 19, 2021
Merged

Add logging support to operator #748

merged 9 commits into from
Aug 19, 2021

Conversation

rfratto
Copy link
Member

@rfratto rfratto commented Jul 16, 2021 •
edited
Loading

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

rfratto
rfratto commented Jul 16, 2021
View reviewed changes
pkg/operator/apis/monitoring/v1alpha1/types_logs.go
Comment on lines -156 to -157
// +kubebuilder:resource:path="pod-logs"
// +kubebuilder:resource:singular="pod-logs"
Copy link
Member 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.

rfratto
rfratto commented Jul 16, 2021
View reviewed changes
pkg/operator/clientutil/clientutil.go
@@ -124,6 +124,43 @@ func CreateOrUpdateStatefulSet(ctx context.Context, c client.Client, ss *apps_v1
return nil
}

// CreateOrUpdateDaemonSet applies the given DaemonSet against the client.
func CreateOrUpdateDaemonSet(ctx context.Context, c client.Client, ss *apps_v1.DaemonSet) error {
Copy link
Member 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 :)

rfratto
rfratto commented Jul 16, 2021
View reviewed changes
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
Member 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.

rfratto
rfratto commented Jul 16, 2021
View reviewed changes
pkg/operator/reconciler_metrics.go
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
Member 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.

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

Copy link
Member 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
rfratto
rfratto commented Jul 20, 2021
View reviewed changes
pkg/operator/apis/monitoring/v1alpha1/types_logs.go
//
// 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
Member 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.

rfratto
rfratto commented Jul 20, 2021
View reviewed changes
pkg/operator/reconciler.go
@@ -143,53 +145,6 @@ func (r *reconciler) fillStore(ctx context.Context, refs []config.AssetReference
return nil
}

// createConfigurationSecret creates the Grafana Agent configuration and stores
// it into a secret.
func (r *reconciler) createConfigurationSecret(
Copy link
Member 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.

pkg/operator/reconciler.go

// createGoverningService creates the service that governs the (eventual)
// StatefulSet. It must be created before the StatefulSet.
func (r *reconciler) createGoverningService(
Copy link
Member 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.

pkg/operator/reconciler.go
}

// createStatefulSets creates a set of Grafana Agent StatefulSets, one per shard.
func (r *reconciler) createStatefulSets(
Copy link
Member 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
Copy link
Member Author

rfratto commented Jul 20, 2021

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
Copy link
Member Author

rfratto commented Jul 20, 2021 •
edited
Loading

(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)

This was referenced Jul 20, 2021
Merged
Merged
@rfratto rfratto mentioned this pull request Aug 17, 2021
Merged
3 tasks
mattdurham
mattdurham reviewed Aug 19, 2021
View reviewed changes
pkg/operator/config/utils.go
@@ -60,6 +64,66 @@ func trimSlice(s []interface{}) []interface{} {
return res
}

// 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
Collaborator

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
Member 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 approved these changes Aug 19, 2021
View reviewed changes
Copy link
Collaborator

@mattdurham mattdurham left a comment •
edited
Loading

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 merged commit ad0b414 into grafana: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
@rfratto
Add logging support to operator (#748)
89ad35f 
* 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.
Reviewers

@mattdurham mattdurham mattdurham approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal: logging support in the Grafana Agent Operator
2 participants
@rfratto @mattdurham