-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Adding statefulsets to inject #910
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by convention, add Fixes #907
to the end of your commit message, that way Github will resolve the original issue when this merges.
cli/cmd/inject.go
Outdated
return err | ||
} | ||
obj = &statefulset | ||
k8sLabels[k8s.ProxyDeploymentLabel] = statefulset.Name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll need to create a new k8s.ProxyStatefulSetLabel
.
taking this a bit farther, this implies some subsequent tasks (best done in future PRs, not this one):
- cli
stat
(part of Fully-implemented, resource-awareconduit stat
#627) - cli
tap
- web
- grafana
it's possible that with this particular PR, we'll at least get stateful-set
labels and metrics wired all the way through from inject -> pod -> destination service -> proxy -> prometheus, but i'm not totally sure. have you tried deploying a StatefulSet and seeing if metrics show up in Prometheus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just what I needed. I'll go look at those now and update this.
@siggy what do you think would be the best way to move forward. I see a couple options:
For this PR, purely from a scoping/focus perspective, it feels okay to leave it at injection and open up issues to track the others. It looks like we've missed implementing things like stat/tap for the other resource types as well (job, daemonset). |
c2c315b
to
ac52d56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for all the updates.
i agree most of this work should be done in subsequent PRs, and we just focus on injection for this PR.
for this PR, there are a couple more places that i think we need to add StatefulSet support:
ac52d56
to
fe1e828
Compare
Ah, the joys of learning a new codebase. Got that all updated, thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! i realized there are two more doc updates needed. please shipit after that:
I grep'd for those, damnit! The underscore got me. |
fe1e828
to
9ead4a6
Compare
9ead4a6
to
6dbc1bd
Compare
No description provided.