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.

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

Fix metrics port expose issue for triggers eventlistener #1086

Merged
merged 1 commit into from
May 14, 2021

Conversation

savitaashture
Copy link
Contributor

@savitaashture savitaashture commented May 11, 2021

Changes

Metrics related changes introduced as part of #1061 and because of those changes we are facing below issues when we create Knative Service using customeResources

  1. when we apply custom-resource example EL fail with below error
Warning  InternalError    15s (x14 over 56s)  EventListener  admission webhook "validation.webhook.serving.knative.dev" denied the request: validation failed: More than one container port is set: spec.template.spec.containers[0].ports
Only a single port is allowed
  1. Knative Serving adds queue-proxy container along with EL container and Knative itself handles metrics on 9090 port for queue-proxy and because of that we face already bind error
{"severity":"ERROR","timestamp":"2021-05-11T09:28:00.588588485Z","logger":"queueproxy","caller":"queue/main.go:228","message":"Failed to bring up queue-proxy, shutting down.","commit":"813aa65","knative.dev/key":"dtest/el-github-knative-listener-00001","knative.dev/pod":"el-github-knative-listener-00001-deployment-fc8fb5cbc-xbqqv","error":"metrics server failed to listen: listen tcp :9090: bind: address already in use","stacktrace":"main.main\n\tknative.dev/serving/cmd/queue/main.go:228\nruntime.main\n\truntime/proc.go:204"}

The PR solves above issues and exposes metrics on 9000 port

Fixes #1090

/cc @jmcshane @dibyom

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Triggers exposes metrics on 9000 port 

@tekton-robot tekton-robot requested review from dibyom and jmcshane May 11, 2021 12:43
@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 11, 2021
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go 79.0% 79.1% 0.1

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go 79.0% 79.1% 0.1

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go 79.0% 79.1% 0.1

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go 79.0% 79.1% 0.1

@savitaashture savitaashture changed the title [WIP] Fix metrics port expose issue for triggers eventlistener Fix metrics port expose issue for triggers eventlistener May 11, 2021
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2021
cmd/eventlistenersink/main.go Outdated Show resolved Hide resolved
cmd/eventlistenersink/main.go Outdated Show resolved Hide resolved
@dibyom
Copy link
Member

dibyom commented May 11, 2021

I tried to quickly dig down into where we were actually setting up the metrics server port to see if there was an easier way to do this but failed. One idea: Would it be possible to just set the port on the defaultObservabilityConfig()

@dibyom
Copy link
Member

dibyom commented May 11, 2021

How about something like this:

	updateFunc, err := metrics.UpdateExporterFromConfigMapWithOpts(ctx, exp, logger)
	if err != nil {
		logger.Fatal("Failed to create metrics exporter update function", zap.Error(err))
	}
	configMapWatcher.Watch(metrics.ConfigMapName(), updateFunc)

@dibyom dibyom added this to the Triggers v0.14 milestone May 12, 2021
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 12, 2021
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go 79.1% 78.6% -0.4

@savitaashture
Copy link
Contributor Author

I tried to quickly dig down into where we were actually setting up the metrics server port to see if there was an easier way to do this but failed. One idea: Would it be possible to just set the port on the defaultObservabilityConfig()

Used METRICS_PROMETHEUS_PORT env to set the port as part of controller.yaml

Comment on lines -684 to -705
container := getContainer(el, c, nil)
container.VolumeMounts = []corev1.VolumeMount{{
Name: "config-logging",
MountPath: "/etc/config-logging",
}}
container.Env = append(container.Env, corev1.EnvVar{
Name: "SYSTEM_NAMESPACE",
ValueFrom: &corev1.EnvVarSource{
FieldRef: &corev1.ObjectFieldSelector{
FieldPath: "metadata.namespace",
}},
})
container.Env = append(container.Env, corev1.EnvVar{
Name: "CONFIG_OBSERVABILITY_NAME",
Value: metrics.ConfigMapName(),
})
container.Env = append(container.Env, corev1.EnvVar{
Name: "METRICS_DOMAIN",
Value: triggersMetricsDomain,
})

container = addCertsForSecureConnection(container, c)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is not part of issue
But its a duplicate code so just removed

@jmcshane
Copy link
Contributor

outside of the gocritic issue, this looks good to me

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 12, 2021
@savitaashture savitaashture removed this from the Triggers v0.14 milestone May 12, 2021
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 12, 2021
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go 79.1% 78.6% -0.4

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go 79.1% 78.6% -0.4

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2021
@dibyom
Copy link
Member

dibyom commented May 14, 2021

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2021
@tekton-robot tekton-robot merged commit f98152d into tektoncd:main May 14, 2021
@savitaashture savitaashture mentioned this pull request Jul 29, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Knative service creation failed with port validation
4 participants