descheduler
descheduler copied to clipboard
kubernetes-sigs
WIP: added otel capabilities for descheduler plugin
Note
Early Draft of the PR to get feedbacks on. This is not yet ready for full review.
Fixes: #951
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: 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: 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 @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!
@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?
@mikedanese That is definitely weird. I did that already and I see not delta in the commits or files! :(
@harshanarayana I think it has something to do with https://github.com/kubernetes-sigs/descheduler/pull/982, but I can't quite figure it out. Posted a more detailed explanation there
@harshanarayana I think it has something to do with https://github.com/kubernetes-sigs/descheduler/pull/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 could you rebase and try running the scripts again to see if there's any changes now? /retest
@harshanarayana could you rebase and try running the scripts again to see if there's any changes now? /retest
Doing that right aways. Thanks @mikedanese
[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 by writing /assign @ingvagabund in a comment. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@harshanarayana could you rebase and try running the scripts again to see if there's any changes now? /retest
Thanks a lot @mikedanese Seem to have done the right thing now. I was not able to get it to work when I rebased and ran on the machine directly. had to build the update command inside the container for it to work. #sigh
❯ make verify
./hack/verify-govet.sh
# sigs.k8s.io/descheduler/pkg/apis/componentconfig
pkg/apis/componentconfig/zz_generated.deepcopy.go:75:10: cannot use c (variable of type *TracingConfiguration) as type runtime.Object in return statement:
*TracingConfiguration does not implement runtime.Object (missing GetObjectKind method)
@mikedanese This is where it got stuck now with new changes. Looks like the deep copy thing is not fully generated.
@harshanarayana is there a reason for the deepcopy gen tag on TracingConfiguration? (also in the unversioned type) that might be the problem, can you try without it?
(btw, please be careful with autocompleting my github name, it's @damemi, but sometimes github picks up a different name -- sorry @mikedanese!)
@harshanarayana is there a reason for the deepcopy gen tag on
TracingConfiguration? (also in the unversioned type) that might be the problem, can you try without it?
Done. Let me see how that goes. Don't think the deep copy bit was really required for that
@harshanarayana: 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.
Rebased. I think we are ready to review! 😊
I'm not sure if we correctly used the context in the following line:
parentCtx, _, parentCloser := tracing.StartSpan(context.Background(), "descheduler_parent", "deschedule")
We initiated new context and passed to plugins; or should we use the ctx here?
We initiated new context and passed to plugins; or should we use the
ctxhere?
We should use the same context that's already available
We should use the same context that's already available
I did think so, makes sense! Updated:
- parentCtx, _, parentCloser := tracing.StartSpan(context.Background(), "descheduler_parent", "deschedule")
+ parentCtx, _, parentCloser := tracing.StartSpan(ctx, "descheduler_parent", "deschedule")
FYI @harshanarayana
1 chart(s) linted, 0 chart(s) failed
./hack/verify-toc.sh
./hack/verify-conversions.sh
Generated output differs:
diff -Naupr pkg/apis/componentconfig/v1alpha1/zz_generated.conversion.go ./hack/../_tmp/kube-verify.Z6K2v6/descheduler/pkg/apis/componentconfig/v1alpha1/zz_generated.conversion.go
--- pkg/apis/componentconfig/v1alpha1/zz_generated.conversion.go 2023-01-03 06:54:10.555176506 +0000
+++ ./hack/../_tmp/kube-verify.Z6K2v6/descheduler/pkg/apis/componentconfig/v1alpha1/zz_generated.conversion.go 2023-01-03 06:56:33.674897486 +0000
@@ -2,7 +2,7 @@
// +build !ignore_autogenerated
/*
-Copyright 2022 The Kubernetes Authors.
+Copyright 2023 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Generated conversions verify failed. Please run ./hack/update-generated-conversions.sh
make: *** [Makefile:133: verify-gen] Error 1
@damemi Looks like one more odd behavior with verify. I am not even sure what the diff is from that error. And if I run the update hack script, it generates no diff. Any idea what I am doing wrong ?
@harshanarayana the diff is in the copyright year for the generated headers (maybe from merging the 1.26 GA libraries last week).
Since it's happening in CI but not your local, try rebasing on master and running the generator locally again.
@mikedanese thought the same an re-ran the hack/update-generated-conversions.sh after rebasing, but there is no diff generated from it.
❯ make clean gen
rm -rf _output
rm -rf _tmp
./hack/update-generated-conversions.sh
# k8s.io/code-generator/cmd/conversion-gen
./hack/update-generated-deep-copies.sh
# k8s.io/code-generator/cmd/deepcopy-gen
./hack/update-generated-defaulters.sh
# k8s.io/code-generator/cmd/defaulter-gen
./hack/update-toc.sh
❯ git status
On branch feature/enable-otel-tracing
Your branch is up to date with 'origin/feature/enable-otel-tracing'.
nothing to commit, working tree clean
@harshanarayana I checked out your branch and ran hack/update-generated-conversions.sh and got the expected diff:
$ git fetch [email protected]:harshanarayana/descheduler.git feature/enable-otel-tracing
remote: Enumerating objects: 276, done.
remote: Counting objects: 100% (276/276), done.
remote: Compressing objects: 100% (107/107), done.
remote: Total 276 (delta 155), reused 253 (delta 150), pack-reused 0
Receiving objects: 100% (276/276), 169.01 KiB | 856.00 KiB/s, done.
Resolving deltas: 100% (155/155), completed with 87 local objects.
From github.com:harshanarayana/descheduler
* branch feature/enable-otel-tracing -> FETCH_HEAD
*
$ git checkout FETCH_HEAD
HEAD is now at 222fc09ef update vendoring to match the upstream branches
mikedame[(HEAD detached at 222fc09ef)] ~/go/src/sigs.k8s.io/descheduler$ hack/update-generated-conversions.sh
mikedame[(HEAD detached at 222fc09ef) !] ~/go/src/sigs.k8s.io/descheduler$ git diff
diff --git a/pkg/apis/componentconfig/v1alpha1/zz_generated.conversion.go b/pkg/apis/componentconfig/v1alpha1/zz_generated.conversion.go
index 924a81590..5087a6369 100644
--- a/pkg/apis/componentconfig/v1alpha1/zz_generated.conversion.go
+++ b/pkg/apis/componentconfig/v1alpha1/zz_generated.conversion.go
@@ -2,7 +2,7 @@
// +build !ignore_autogenerated
/*
-Copyright 2022 The Kubernetes Authors.
+Copyright 2023 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Maybe it is a Go version issue? Or try running in a container?