added otel capabilities for descheduler plugin #958

harshanarayana · 2022-09-23T06:34:22Z

Note

Early Draft of the PR to get feedbacks on. This is not yet ready for full review.

Fixes: #951

k8s-ci-robot · 2022-09-23T06:34:24Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

harshanarayana · 2022-09-23T07:17:49Z

/assign @Dentrax

k8s-ci-robot · 2022-09-23T07:17:50Z

@harshanarayana: GitHub didn't allow me to assign the following users: Dentrax.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @Dentrax

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.

harshanarayana · 2022-09-23T09:38:21Z

/cc @Dentrax

k8s-ci-robot · 2022-09-23T09:38:22Z

@harshanarayana: GitHub didn't allow me to request PR reviews from the following users: Dentrax.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Dentrax

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.

damemi · 2022-09-30T15:35:29Z

pkg/descheduler/strategy_migration.go

+type RunFunc func(ctx context.Context, nodes []*v1.Node) *framework.Status
+
+// RunPlugin runs a plugin.
+func RunPlugin(plugin framework.Plugin, runFn RunFunc, operationName, pluginName string) func(ctx context.Context, nodes []*v1.Node) {


rather than wrapping the plugin's function in a span, why not start and close the span within the plugin's Deschedule function?

I would prefer if traces are implemented by the plugins themselves rather than the framework. The framework can provide the mechanisms (like registering a global traceprovider) but I would leave the actual spans up to plugin developers to implement

My initial attempt was to put Span login within each plugin individually. And then I thought it could hard to maintain if we manage tracing stuff separately:

it would be easy to forget to put tracing logic in a function

it would bring code duplication (1 line tracing, 1 line defer: times plugin count)

it would be easy to forget to add span.RecordError(status.Err) function in all error cases

should plugins really know the existence of tracing? (why plugin developers should duplicate this logic?)

These are just my concern, I'm OK to go with your way anyway. Wanted to learn more about your concern. 🤞

should plugins really know the existence of tracing? (why plugin developers should duplicate this logic?)

One benefit of a framework model is detangling the plugin logic from the surrounding framework internals, so in that sense the answer is yes.

On that end, it makes sense for us to start a parent trace in our internals (somewhere around here), and plugins can use the context to add whatever additional spans they want. But the responsibilities of good design you mention in your other bullet points are kept to the individual plugin developers

On that end, it makes sense for us to start a parent trace in our internals (somewhere around here),

+1 from me for this. I think we can start from this for now and see how it goes. Enforcing the tracing related code can be done mostly during the code review of a new plugin getting added anyway. The current code in the repo already lets us create the parent trace and pass the context down to the individual plugin/strategies so that they can trace the required bit of the code in that accordingly.

This will also leave the plugin developer with freedom of being able to trace some of their internal helper/util functions as well if required.

@harshanarayana yup, that's exactly what I'm thinking

damemi · 2022-10-04T14:08:19Z

pkg/apis/componentconfig/v1alpha1/types.go

@@ -22,6 +22,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	componentbaseconfig "k8s.io/component-base/config"
 	registry "k8s.io/component-base/logs/api/v1"
+	"sigs.k8s.io/descheduler/pkg/apis/componentconfig"


the versioned API shouldn't import the unversioned API. it's a hub-and-spoke model for conversion, so you should basically duplicate the types into the versioned api here

/Ack. I am working on getting this changed. Will publish a copy of this by tomorrow. @damemi

Changes done.

harshanarayana · 2022-10-13T02:17:27Z

/cc @damemi

harshanarayana · 2022-10-13T02:21:06Z

/test pull-descheduler-verify-master

harshanarayana · 2022-10-13T05:34:19Z

/test pull-descheduler-verify-master

harshanarayana · 2022-10-13T05:39:52Z

@damemi @damemi

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_descheduler/958/pull-descheduler-verify-master/1580431724137943040

Unable to figure out why this issue on the CICD showing up.

❯ hack/verify-conversions.sh
hack/uGenerated conversions verified.

if I run the verify locally, it works just fine!

damemi · 2022-10-13T16:28:58Z

@harshanarayana I checked out your branch locally and it fails when I run it with the same output as the CI. Could you try running make clean first, or perhaps in a container?

harshanarayana · 2022-10-13T16:35:18Z

@mikedanese That is definitely weird. I did that already and I see not delta in the commits or files! :(

damemi · 2022-10-13T18:23:15Z

@harshanarayana I think it has something to do with #982, but I can't quite figure it out. Posted a more detailed explanation there

harshanarayana · 2022-10-14T04:19:53Z

@harshanarayana I think it has something to do with #982, but I can't quite figure it out. Posted a more detailed explanation there

@damemi Thanks very much. Definitely looks like the same. Let me run the same generation code in a different machine I have and see if actually makes a difference. Do you think it matters where my code resides though? Mine doesn't exists under the $GOPATH/src/<x>/<y> path

harshanarayana · 2023-02-04T15:57:31Z

@a7i @damemi Sorry for the delay and responding back to this. I will get this addressing your comments today and push the changes accordingly.

tracing: pass start span func to all plugins Signed-off-by: Furkan <furkan.turkal@trendyol.com> tracing: pass span context to run func Signed-off-by: Furkan <furkan.turkal@trendyol.com> move tracing workflow to individual plugins handle shutdown handling for context cancel enable hub/spoke mode for the OTEL config conversion add tests for tracing Signed-off-by: Furkan <furkan.turkal@trendyol.com> update generated conversion data model update generated conversion spec fix deepcopy gen behavior gofumpt cleanup for formatting format descheduler.go Signed-off-by: Furkan <furkan.turkal@trendyol.com> use same context instead creating a new one Signed-off-by: Furkan <furkan.turkal@trendyol.com> update vendoring to match the upstream branches regenerated auto generated code for new copyright for verify match Signed-off-by: Harsha Narayana <harshnar@cisco.com> fix klog ErrorS invocation cleanup vendor and mod files reuse existing context against trace spans and fix the structs update conversion

Signed-off-by: Furkan <furkan.turkal@trendyol.com>

k8s-ci-robot · 2023-02-25T14:44:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ingvagabund for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

harshanarayana · 2023-02-25T15:01:18Z

@a7i @damemi Thanks for your patience on this. I have been on again and off again due to some personal commitments. Getting back to this again. Left a few comments. Need help with those so that I can close the pending comments.

harshanarayana · 2023-03-22T15:03:48Z

/ping @damemi @a7i

k8s-ci-robot · 2023-03-24T18:36:47Z

PR needs rebase.

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.

k8s-ci-robot · 2023-04-12T07:10:30Z

@harshanarayana: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-descheduler-test-e2e-k8s-master-1-27	`a281e80`	link	true	`/test pull-descheduler-test-e2e-k8s-master-1-27`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

a7i · 2023-04-12T13:41:24Z

/cc @damemi @ingvagabund @knelasevero

Has the 0.27 🚢 sailed or do you think we can squeeze this in?
I'm certainly open to bringing this in since not enabling it doesn't have any negative side effects.

damemi · 2023-04-12T13:56:30Z

@a7i this needs a rebase and there is still some unaddressed feedback. I would say let's release 0.27 and target this for 0.28

harshanarayana · 2023-04-12T18:14:18Z

@damemi I've addressed most of the comments already. Let me tenses again and push changes.. I'm fine with moving this to 0.28

damemi · 2023-04-12T18:24:34Z

@harshanarayana I bumped the comments that I noticed that still need to be fixed from when we left this off

k8s-triage-robot · 2023-07-11T18:39:48Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

a7i · 2023-07-11T18:51:47Z

/remove-lifecycle stale

@harshanarayana apologies for the delay on this review. would be good to include this as part of next release. would you be able to address the issues above? I can also address if you are ok with that.

harshanarayana · 2023-07-12T05:41:17Z

@a7i I will take care of this. I might just close and re-create a new PR entirely with a clean slate and keep a reference for this to track the history of discussion. I will have a PR ready for you in a day or two.

harshanarayana · 2023-07-16T19:12:37Z

Closing this in favor of #1189

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 23, 2022

k8s-ci-robot requested review from JaneLiuL and seanmalloy September 23, 2022 06:34

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 23, 2022

harshanarayana changed the title ~~added otel capabilities for descheduler plugin~~ WIP: added otel capabilities for descheduler plugin Sep 23, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2022

harshanarayana force-pushed the feature/enable-otel-tracing branch from fdcafea to 60a9747 Compare September 28, 2022 16:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2022

damemi reviewed Sep 30, 2022

View reviewed changes

harshanarayana force-pushed the feature/enable-otel-tracing branch from 6801733 to d37ab93 Compare October 4, 2022 10:49

damemi reviewed Oct 4, 2022

View reviewed changes

harshanarayana marked this pull request as ready for review October 13, 2022 02:16

k8s-ci-robot requested a review from a7i October 13, 2022 02:16

k8s-ci-robot requested a review from damemi October 13, 2022 02:17

harshanarayana force-pushed the feature/enable-otel-tracing branch from 0f1a622 to 115bab4 Compare October 13, 2022 02:24

damemi mentioned this pull request Oct 13, 2022

Update verify scripts to use the generation scripts directly #982

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2023

harshanarayana and others added 3 commits February 25, 2023 20:04

Add missing copyright headers

b1ed3f0

Signed-off-by: Furkan <furkan.turkal@trendyol.com>

regenerate docs using auto gen tool

312d8f8

harshanarayana force-pushed the feature/enable-otel-tracing branch from f39dfc2 to 312d8f8 Compare February 25, 2023 14:44

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2023

otel: cleanup unwatned context and reuse attribute packages

a281e80

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2023

k8s-ci-robot requested review from damemi, ingvagabund and knelasevero April 12, 2023 13:41

knelasevero mentioned this pull request Jul 11, 2023

Descheduling framework wrap up #1187

Open

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 11, 2023

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 11, 2023

harshanarayana mentioned this pull request Jul 16, 2023

feat: Enable open telemetry tracing #1189

Merged

harshanarayana closed this Jul 16, 2023

added otel capabilities for descheduler plugin #958

added otel capabilities for descheduler plugin #958

Conversation

harshanarayana commented Sep 23, 2022

Note

k8s-ci-robot commented Sep 23, 2022

harshanarayana commented Sep 23, 2022

k8s-ci-robot commented Sep 23, 2022

harshanarayana commented Sep 23, 2022

k8s-ci-robot commented Sep 23, 2022

damemi Sep 30, 2022

Choose a reason for hiding this comment

Dentrax Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

damemi Oct 3, 2022

Choose a reason for hiding this comment

harshanarayana Oct 3, 2022

Choose a reason for hiding this comment

damemi Oct 3, 2022

Choose a reason for hiding this comment

harshanarayana Oct 4, 2022

Choose a reason for hiding this comment

damemi Oct 4, 2022

Choose a reason for hiding this comment

harshanarayana Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

harshanarayana Oct 10, 2022

Choose a reason for hiding this comment

harshanarayana commented Oct 13, 2022

harshanarayana commented Oct 13, 2022

harshanarayana commented Oct 13, 2022

harshanarayana commented Oct 13, 2022

damemi commented Oct 13, 2022

harshanarayana commented Oct 13, 2022

damemi commented Oct 13, 2022

harshanarayana commented Oct 14, 2022 • edited Loading

harshanarayana commented Feb 4, 2023

k8s-ci-robot commented Feb 25, 2023

harshanarayana commented Feb 25, 2023

harshanarayana commented Mar 22, 2023

k8s-ci-robot commented Mar 24, 2023

k8s-ci-robot commented Apr 12, 2023

a7i commented Apr 12, 2023

damemi commented Apr 12, 2023

harshanarayana commented Apr 12, 2023 • edited Loading

damemi commented Apr 12, 2023

k8s-triage-robot commented Jul 11, 2023

a7i commented Jul 11, 2023

harshanarayana commented Jul 12, 2023

harshanarayana commented Jul 16, 2023

Dentrax Sep 30, 2022 •

edited

Loading

harshanarayana Oct 4, 2022 •

edited

Loading

harshanarayana commented Oct 14, 2022 •

edited

Loading

harshanarayana commented Apr 12, 2023 •

edited

Loading