Add a new 'workload name' label override #3374
Conversation
howardjohn
added
the
release-notes-none
|
🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test? Courtesy of your friendly test nag. |
istio-testing
added
the
size/S
| @@ -49,6 +49,17 @@ labels: | |||
| resources: | |||
| - Pod | |||
|
|
|||
| - name: service.istio.io/workload-name | |||
There was a problem hiding this comment.
Need to think about this more, but we probably need a warning to advise users to consider label cardinality
There was a problem hiding this comment.
Probably so, same applies to the canonical-{service,revision} which I can add. Interestingly, usage of this label is often in order to reduce cardinality (where we do not correctly pick an abstract-enough name and end up with random suffix in the default derivation)
There was a problem hiding this comment.
@howardjohn any Action Item (AI) from @keithmattix's comment? Wasn't sure if the warning should be added from your comment.
|
On Mon, Dec 2, 2024 at 10:49 AM John Howard ***@***.***> wrote:
Today, we have a "workload name" concept that ends up in metrics.
This is defaulted to deriving from deployment metadata (Deployment name,
etc).
This approach is not perfect. We cannot always derive an appropriate
name (for instance, a Pod can be created directly!). WorkloadEntry,
which often is equvilent to Pod, is also using the WE name -- generally
we would want a higher order name there.
Not sure what is the problem - yes, a pod can be created directly and like
WE it has a name.
Why would a user set a label that is different then the WE name ?
I don't think the canonical-{} stuff is ideal solution either - I don't
know the usage number, but besides the auto-added
stuff it is not as common.
Having a separate resource with a selector - without requiring pods to be
injected or deal with the various ways to
set labels in helm - is a better option.
And if the use case is telemetry - using the OTel mechanisms instead of
inventing our own would likely be better too.
… To fix this, I propose we add a label to explicitly specify the workload
name. This format mirrors the existing canonical-{service,revision}
label.
------------------------------
You can view, comment on, or merge this pull request online at:
#3374
Commit Summary
- 4eb867c
<4eb867c>
Add a new 'workload name' label override
File Changes
(3 files <https://github.com/istio/api/pull/3374/files>)
- *M* label/labels.gen.go
<https://github.com/istio/api/pull/3374/files#diff-812733a955cb04783aa5dcdb0739f0bacf722ecaddb20a1456c3e5983947db4c>
(15)
- *M* label/labels.pb.html
<https://github.com/istio/api/pull/3374/files#diff-7eea90a97a48aff572555d8afa4abaa709028ffc9227f623496eae23b403d021>
(23)
- *M* label/labels.yaml
<https://github.com/istio/api/pull/3374/files#diff-38c3a0b1fb716887f9581a9f0d58bd7d76271ea34585307b7f3f3d47b4475c09>
(11)
Patch Links:
- https://github.com/istio/api/pull/3374.patch
- https://github.com/istio/api/pull/3374.diff
—
Reply to this email directly, view it on GitHub
<#3374>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2U55Y2JQ2AQS4ARXT32DSTR3AVCNFSM6AAAAABS4CL4B2VHI2DSMVQWIX3LMV43ASLTON2WKOZSG4YTEOJYGE3DQMI>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
My pod/workload entry name is We have heuristics to prevent this, but they are only that -- heuristics. And there is not (unless this PR lands) a way to override the heuristics. The intent here is to provide a reliable way to get sane behavior when the heuristic fails.
I disagree:
There is not one that I know of. |
|
Though it may give some facilities, but over relying annotation and labels is not a good UX. Istio may already have hundred of label + annotation. That's a horrible thing. For this particular case, i would think if no deployment or job found, we can fallback to service name. In most cases, they may have same name. |
this is injected into the pod so getting service name is impossible |
|
On Mon, Dec 2, 2024 at 5:43 PM John Howard ***@***.***> wrote:
For this particular case, i would think if no deployment or job found, we
can fallback to service name. In most cases, they may have same name.
this is injected into the pod so getting service name is impossible
Getting the service name is not impossible - pod has labels, service has
label selectors.
Yes, a pod may be selected by multiple services - but it's telemetry, not
authorization or rocket science.
And if it doesn't match a service - it is also fine to just not set that
particular dimension - we are not
breaking anything since the attribute doesn't exist yet.
The proper way to handle that is to work with open telemetry and figure out
an API that is independent
of Istio - ideally using more precise label selectors in a separate CRD, if
having such dimensions in corner cases is
that critical, which I personally doubt. I don't mind an API to
group/organize pods without deployments - my
concerns are with even more annotations on the pod (that is a pretty bad
API - even if we use it extensively)
and is another istio-only telemetry.
We are talking about pods without deployment/job (not so common) - we
already track the service account
for client metrics, for servers we should be able to track the VIP and/or
service name in http case.
… Message ID: ***@***.***>
|
The attribute does exist. I am not proposing adding a new attribute, I would be strongly against that. The problem is users today get a label set to WorkloadEntry/Pod name, or something else that is dynamic, which is extremely unsafe. This is trivial to reproduce, try to use an WorkloadEntry and you will see its name show up in the Prometheus labels.
Even if we hypothetically could, this attribute is something that is derived at startup/injection time so its practically infeasible. Not to mention the value is workload which is intentionally distinct from "service" as a concept in Istio. |
|
Its pretty clear the OTel can't be a solution for this issue since it can only operate on data that is in-band to the pipeline and an alternate semantic name could never be. The proposal here mimics what kubernetes has done for itself with the app.kubernetes.io/name label per https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ Could we use that instead? |
We currently have these labels in our metadata:
So effectively we have 2 concepts:
One may argue we should just have one concept, application. Maybe so, but IMO its pretty much too late to change it. Notably, if we used Using a new label doesn't have this issue since (1) its clearly scoped to just workload ( |
|
The expectation here is that a user will set this to point to the object type of the highest level of workload orchestration they have. This could be a WorkloadGroup, a Deployment, an ArgoCD application, etc. In many of these cases we can auto detect it. In fact, with WorkloadGroup we could (there is a "TODO" in the codebase to do this, which I will also address). But not all -- sometimes Pods and WorkloadEntry are created in a such a way that there is no way to determine their parent automatically. In these cases, it is imperative that we allow users a way to not place an unstable ID into a Prometheus label. |
|
I generally agree with what's said above. We use the ability of Deployment to copy labels down onto pods to handle the "provide a semantic name" thing and we support a plethora of labels via that mechanism to align with common usage patterns. WorkloadGroup plays the same role as Deployment here but we don't have an auto-copying mechanism and WorkloadEntry is allowed to exist without a WorkloadGroup. Assuming we keep using the same label definitions and derivation mechanisms I see no reason not support reading those labels of WorkloadEntry just as we would for a directly scheduled Job or Pod |
|
Can we set it to the workload group instead of workload entry ? I don't
mind changes to Istio APIs, my concern was Pod, and dependency on injection.
Also: are we creating more difficulty on adoption of ambient ? At some
point we may need to reduce the set of labels that only work with
sidecar/injection.
I'm not trying to block this or make it too complicated - just to make sure
we understand the implications.
…On Fri, Dec 13, 2024, 13:49 Louis Ryan ***@***.***> wrote:
I generally agree with what's said above.
We use the ability of Deployment to copy labels down onto pods to handle
the "provide a semantic name" thing and we support a plethora of labels via
that mechanism to align with common usage patterns.
WorkloadGroup plays the same role as Deployment here but we don't have an
auto-copying mechanism and WorkloadEntry is allowed to exist without a
WorkloadGroup. Assuming we keep using the same label definitions and
derivation mechanisms I see no reason not support reading those labels of
WorkloadEntry just as we would for a directly scheduled Job or Pod
—
Reply to this email directly, view it on GitHub
<#3374 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2WC4SV7VRPAXWJHP6D2FNI7JAVCNFSM6AAAAABS4CL4B2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBSGQZTMNJQGE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
And to be clear - WorkloadEntry labels are sent in CDS/EDS and computed by
Istiod, I don't think they depend on injection, so not sure what is the
connection. For dest labels - almost everything is under Istiod control.
…On Fri, Dec 13, 2024, 17:06 Costin Manolache ***@***.***> wrote:
Can we set it to the workload group instead of workload entry ? I don't
mind changes to Istio APIs, my concern was Pod, and dependency on injection.
Also: are we creating more difficulty on adoption of ambient ? At some
point we may need to reduce the set of labels that only work with
sidecar/injection.
I'm not trying to block this or make it too complicated - just to make
sure we understand the implications.
On Fri, Dec 13, 2024, 13:49 Louis Ryan ***@***.***> wrote:
> I generally agree with what's said above.
>
> We use the ability of Deployment to copy labels down onto pods to handle
> the "provide a semantic name" thing and we support a plethora of labels via
> that mechanism to align with common usage patterns.
>
> WorkloadGroup plays the same role as Deployment here but we don't have an
> auto-copying mechanism and WorkloadEntry is allowed to exist without a
> WorkloadGroup. Assuming we keep using the same label definitions and
> derivation mechanisms I see no reason not support reading those labels of
> WorkloadEntry just as we would for a directly scheduled Job or Pod
>
> —
> Reply to this email directly, view it on GitHub
> <#3374 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAUR2WC4SV7VRPAXWJHP6D2FNI7JAVCNFSM6AAAAABS4CL4B2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBSGQZTMNJQGE>
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>
|
|
I think it will answer a lot of questions to just send a draft PR showing
exactly what changes will be made. I'll send one next week.
On Fri, Dec 13, 2024, 5:09 PM Costin Manolache ***@***.***>
wrote:
… And to be clear - WorkloadEntry labels are sent in CDS/EDS and computed by
Istiod, I don't think they depend on injection, so not sure what is the
connection. For dest labels - almost everything is under Istiod control.
On Fri, Dec 13, 2024, 17:06 Costin Manolache ***@***.***> wrote:
> Can we set it to the workload group instead of workload entry ? I don't
> mind changes to Istio APIs, my concern was Pod, and dependency on
injection.
>
> Also: are we creating more difficulty on adoption of ambient ? At some
> point we may need to reduce the set of labels that only work with
> sidecar/injection.
>
> I'm not trying to block this or make it too complicated - just to make
> sure we understand the implications.
>
> On Fri, Dec 13, 2024, 13:49 Louis Ryan ***@***.***> wrote:
>
>> I generally agree with what's said above.
>>
>> We use the ability of Deployment to copy labels down onto pods to
handle
>> the "provide a semantic name" thing and we support a plethora of labels
via
>> that mechanism to align with common usage patterns.
>>
>> WorkloadGroup plays the same role as Deployment here but we don't have
an
>> auto-copying mechanism and WorkloadEntry is allowed to exist without a
>> WorkloadGroup. Assuming we keep using the same label definitions and
>> derivation mechanisms I see no reason not support reading those labels
of
>> WorkloadEntry just as we would for a directly scheduled Job or Pod
>>
>> —
>> Reply to this email directly, view it on GitHub
>> <#3374 (comment)>, or
>> unsubscribe
>> <
https://github.com/notifications/unsubscribe-auth/AAAUR2WC4SV7VRPAXWJHP6D2FNI7JAVCNFSM6AAAAABS4CL4B2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBSGQZTMNJQGE>
>> .
>> You are receiving this because you commented.Message ID:
>> ***@***.***>
>>
>
—
Reply to this email directly, view it on GitHub
<#3374 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYGXP2SRGF4TUVJCKPLIL2FOAKZAVCNFSM6AAAAABS4CL4B2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBSGYYDOOJVGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Implementation for istio/api#3374
|
Here is the implementation: istio/istio#54340. WorkloadName is used in 3 places:
The above PR is adding this. It sort of already exists because |
|
Good, that works, my concern was on depending on injection or Pod
modifications. As long as it's in WDS/EDS - no problem.
Not so much concerned about sidecar behavior, it already depends on
injection and lots of other annotations - just want to make sure a pure
ambient
can work 100% without requiring pod annotation.
…On Mon, Dec 16, 2024 at 8:37 AM John Howard ***@***.***> wrote:
Here is the implementation: istio/istio#54340
<istio/istio#54340>.
WorkloadName is used in 3 places:
1. Bootstrap for envoy, so the proxy knows how to emit source_workload
and send it over metadata exchange
2. WDS for Ztunnel, so it knows how to emit
{source,destination}_workload
3. EDS for envoy proxies, so it knows how to emit destination_workload *as
a fallback if metadata exchange does not have it*
Can we set it to the workload group instead of workload entry
The above PR is adding this. It *sort of* already exists because istioctl
will read the WorkloadGroup and generate a bootstrap config that has
ISTIO_META_WORKLOAD_NAME which hits case (1) and sends over MX. This
doesn't work with HBONE though. The above PR makes sure all 3 cases
fallback to WorkloadGroup name
—
Reply to this email directly, view it on GitHub
<#3374 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2SALNCS6ZLW35OAYKT2F36WJAVCNFSM6AAAAABS4CL4B2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBWGEYDSNJXGE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Added a comment on your PR - 99% good.
…On Mon, Dec 16, 2024 at 9:51 AM Costin Manolache ***@***.***> wrote:
Good, that works, my concern was on depending on injection or Pod
modifications. As long as it's in WDS/EDS - no problem.
Not so much concerned about sidecar behavior, it already depends on
injection and lots of other annotations - just want to make sure a pure
ambient
can work 100% without requiring pod annotation.
On Mon, Dec 16, 2024 at 8:37 AM John Howard ***@***.***>
wrote:
> Here is the implementation: istio/istio#54340
> <istio/istio#54340>.
>
> WorkloadName is used in 3 places:
>
> 1. Bootstrap for envoy, so the proxy knows how to emit
> source_workload and send it over metadata exchange
> 2. WDS for Ztunnel, so it knows how to emit
> {source,destination}_workload
> 3. EDS for envoy proxies, so it knows how to emit
> destination_workload *as a fallback if metadata exchange does not
> have it*
>
> Can we set it to the workload group instead of workload entry
>
> The above PR is adding this. It *sort of* already exists because istioctl
> will read the WorkloadGroup and generate a bootstrap config that has
> ISTIO_META_WORKLOAD_NAME which hits case (1) and sends over MX. This
> doesn't work with HBONE though. The above PR makes sure all 3 cases
> fallback to WorkloadGroup name
>
> —
> Reply to this email directly, view it on GitHub
> <#3374 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAUR2SALNCS6ZLW35OAYKT2F36WJAVCNFSM6AAAAABS4CL4B2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBWGEYDSNJXGE>
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>
|
Today, we have a "workload name" concept that ends up in metrics.
This is defaulted to deriving from deployment metadata (Deployment name,
etc).
This approach is not perfect. We cannot always derive an appropriate
name (for instance, a Pod can be created directly!). WorkloadEntry,
which often is equvilent to Pod, is also using the WE name -- generally
we would want a higher order name there.
To fix this, I propose we add a label to explicitly specify the workload
name. This format mirrors the existing canonical-{service,revision}
label.
howardjohn
force-pushed
the
label/workload-name
branch
from
4eb867c to
2f35e79
Compare
Implementation for istio/api#3374
* Implement new workload-name label Implementation for istio/api#3374 * add release note
Today, we have a "workload name" concept that ends up in metrics.
This is defaulted to deriving from deployment metadata (Deployment name,
etc).
This approach is not perfect. We cannot always derive an appropriate
name (for instance, a Pod can be created directly!). WorkloadEntry,
which often is equvilent to Pod, is also using the WE name -- generally
we would want a higher order name there.
To fix this, I propose we add a label to explicitly specify the workload
name. This format mirrors the existing canonical-{service,revision}
label.