Change Flyte CR naming scheme to better support namespace_mapping #5480
Conversation
ddl-ebrown
force-pushed
the
change-flyte-CR-naming-scheme
branch
from
806c40b to
438dd97
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5480 +/- ##
=======================================
Coverage 36.18% 36.18%
=======================================
Files 1302 1302
Lines 109614 109644 +30
=======================================
+ Hits 39660 39680 +20
- Misses 65809 65818 +9
- Partials 4145 4146 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ddl-ebrown
force-pushed
the
change-flyte-CR-naming-scheme
branch
from
438dd97 to
532888b
Compare
| ctx, | ||
| v1.DeleteOptions{PropagationPolicy: &deletePropagationBackground}, | ||
| v1.ListOptions{ | ||
| LabelSelector: v1.FormatLabelSelector(executionLabelSelector(data.ExecutionID)), |
There was a problem hiding this comment.
Even though new executions will have different CR names, this deletion mechanism is fully backwards compatible - thanks for a good solution @ddl-rliu !
ddl-ebrown
force-pushed
the
change-flyte-CR-naming-scheme
branch
2 times, most recently
from
33e6e7d to
f97c77a
Compare
ddl-ebrown
force-pushed
the
change-flyte-CR-naming-scheme
branch
from
f97c77a to
d994304
Compare
| @@ -165,6 +165,10 @@ func TestDynamic(t *testing.T) { | |||
| Name: "name", | |||
| }, | |||
| "namespace") | |||
| // make sure real CR has randomized suffix | |||
There was a problem hiding this comment.
This is probably the simplest way to ensure existing tests continue to pass
| rand.Seed(seed) | ||
| // K8s has a limitation of 63 chars | ||
| name = name[:minInt(63-ExecutionIDSuffixLength, len(name))] | ||
| execName := name + "-" + rand.String(ExecutionIDSuffixLength-1) |
There was a problem hiding this comment.
Without this randomization, use of namespace_mapping in the config
flyte/flyteadmin/pkg/runtime/namespace_config_provider.go
Lines 11 to 18 in 02bf85f
foo causes problems when executions have the same names across projects
| @@ -159,6 +161,17 @@ func generateName(wfID *core.Identifier, execID *core.WorkflowExecutionIdentifie | |||
| } | |||
| } | |||
|
|
|||
| const ExecutionIDSuffixLength = 21 | |||
There was a problem hiding this comment.
Can we make this a configurable value and if set to 0 (default?) then have it disabled (so no random characters are appended). My concern is that if anything relies on the CR name, this will break it.
There was a problem hiding this comment.
I think we found / updated the spots where the name is a "contract" -- but if we want to be extra super careful we could make this configurable.
That said, I think the tradeoff we have to consider is:
- backward compatibility vs
- adding extra config / managing different behaviors
I would probably vote for not introducing an extra config and keeping this behavior not configurable (I'd argue prior behavior was a bug), but admit to not knowing the potential blast radius beyond core Flyte (i.e. plugins and such).
I'm happy to go either way since it's not my project :)
There was a problem hiding this comment.
Yeah, I understand the config bloat all to well :). As you suggest, my main concern is breaking backwards compatibility here. I know there are Flyte users that rely on the FlyteWorkflow CR to be named identical to the execution ID, which this would break. For me to be comfortable merging this, I feel it should be defaulted to the current behavior. cc @eapolinario thoughts?
There was a problem hiding this comment.
Ah thanks!
If you know there are users depending on the existing CR naming scheme somehow, then making this behavior configurable seems like the only thing to do right now. I can update my PR.
Not sure if you track potential breaking change tickets anywhere, but maybe file one away to remove the option to configure this on the next major release boundary?
There was a problem hiding this comment.
Thanks for the feedback! I've updated the PR to address this and explain a bit more here: #5480 (comment)
|
I am not in favor of this, as randomness will lead to leaky workflows and duplicates. We should use the project id itself or generate a consistent hash to increase inter project execution entropy |
Ah thanks @kumare3 for the heads up! We clearly didn't realize there was something internal to Flyte that depends on deterministic naming for CRs -- will make some updates taking that into account as well |
Also, should mention @kumare3 that if by "leaky" you meant "CR might not be deleted from the cluster", the deletion process is robust because this uses the actual key of the workflow in conjunction with CR labels to perform deletes, rather than the CR name. If there are dupe CRs for the same workflow though, that's clearly an issue regardless :) |
|
@ddl-ebrown I agree with not introducing randomization... specially that the name already starts with a random string :-) Instead, I would update this call to use something like I would also make the length of the execution name configurable in flyteadmin. so in your deployment you can make it longer and give you better entropy... |
ddl-rliu
force-pushed
the
change-flyte-CR-naming-scheme
branch
6 times, most recently
from
33a99f0 to
93bb9a5
Compare
| } | ||
| // make sure real CR has randomized suffix | ||
| assert.Regexp(t, regexp.MustCompile("name-[bcdfghjklmnpqrstvwxz2456789]{20}"), flyteWf.Name) | ||
| // then reset for the sake of serialization comparison | ||
| flyteWf.Name = "name" | ||
|
|
There was a problem hiding this comment.
The cr-name-scheme-change is now non-default, and tests are added here, so removing these checks.
| hashedIdentifier := hashIdentifier(core.Identifier{ | ||
| Project: project, | ||
| Domain: domain, | ||
| Name: name, | ||
| }) | ||
| rand.Seed(int64(hashedIdentifier)) |
There was a problem hiding this comment.
By hashing exactly these fields project, domain, name we prevent duplicate CRs from being created for the same execution (they will hash to the same CR name and k8s will prevent those duplicate CRs).
Edit: Regarding the hashIdentifier function, see this for reference.
| if workflowCRNameHashLength := config.GetConfig().WorkflowCRNameHashLength; workflowCRNameHashLength > 0 { | ||
| obj.ObjectMeta.Name = rand.String(workflowCRNameHashLength) | ||
| } else { | ||
| obj.ObjectMeta.Name = name | ||
| } |
There was a problem hiding this comment.
By default WorkflowCRNameHashLength is 0 so the hash CR naming scheme will only be used when the deployment is specially configured to do so. See 93bb9a5#diff-67941e1320bd66fda570ebcf1f1c5c5221d608b136d662705e168a07a5d166ef
| rand.Seed(int64(hashedIdentifier)) | ||
|
|
||
| if workflowCRNameHashLength := config.GetConfig().WorkflowCRNameHashLength; workflowCRNameHashLength > 0 { | ||
| obj.ObjectMeta.Name = rand.String(workflowCRNameHashLength) |
There was a problem hiding this comment.
Couldn't the name of the CR just incorporate the k8s namespace though and still be stable? I think using just a hash for the CR name loses some valuable information when you're glancing at the resource names now.
There was a problem hiding this comment.
Updated #5480 (comment)
Now, for a setting like WorkflowCRNameHashLength: 32, the CR name will look like mywfname-f5ptmmd66pktp59nphfdjkqr9szp8fv5 or mywfname-wnw9zlg22z5wmdjggdcrnzdrpw59m4bc. Note that the suffix is deterministic, and uses a hash of the name+project+domain.
I opted for this approach rather than mywfname-project, this is because of the character limit of the CR name and how this affects executions that have a long execution name and a long project name, where improperly handled truncation will lead to undesired behavior such as executions not starting as a result of name conflicts. For reference.
ddl-rliu
force-pushed
the
change-flyte-CR-naming-scheme
branch
2 times, most recently
from
f46550d to
d4a0e72
Compare
| base := name + "-" | ||
| maxNameLength := allowedExecutionNameLength - workflowCRNameHashLength | ||
| if len(base) > maxNameLength { | ||
| base = base[:maxNameLength] | ||
| } | ||
| obj.ObjectMeta.Name = fmt.Sprintf("%s%s", base, rand.String(workflowCRNameHashLength)) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Not to bikeshed too much on this one, but a couple things...
You can skip some of that slice logic around length if you just use format specifiers to set the "max"
maxNameLength := allowedExecutionNameLength - workflowCRNameHashLength - 1
if s, err := strconv.Atoi(maxNameLength); err == nil {
format := "[%." + s+ "s]-%s"
obj.ObjectMeta.Name = fmt.Sprintf(format, base, rand.String(workflowCRNameHashLength))
}Even better -- does WorkflowCRNameHashLength really need to be configurable or can it just be on / off? If it's sufficiently long enough to prevent collisions (and I think we can pick something), then you could simplify this length calculation logic and just limit the string statically using a format specifier like this (let's assume its set to 24, for example):
maxBase := allowedExecutionNameLength - workflowCRNameHashLength - 1
obj.ObjectMeta.Name = fmt.Sprintf("[%.38s]-%s", base, rand.String(workflowCRNameHashLength))There was a problem hiding this comment.
The k8s rand util you're using has 32 possible characters... I think once you hit a string of length 24 you're in UUID territory (which is 5.31x10^36 and generally considered more than anyone would ever need)
I don't think something that long is remotely necessary for this use case though - the primary concern is that the seeded random number generator doesn't produce a duplicate random string given two different seeds (i.e. for an execution with the same name and domain, but in a different project). For that, even something like 10 characters should be enough I think, which is why I would advocate for picking something sensible. I know maintainers suggested making it configurable, but that feels unnecessary IMHO.
32^10 is 1.13x10^15
Using the collision calculator at https://devina.io/collision-calculator to understand this in more human-friendly terms
~1 year (or 3.43e+7 seconds) needed, in order to have a 1% probability of at least one collision if 500 ID's are generated every hour.
There was a problem hiding this comment.
A fixed suffix length of 10 sounds good to me. Agreed about the sufficient amount of entropy on relatively high values for the suffix length. Just updated the PR: 15b57cb
ddl-rliu
force-pushed
the
change-flyte-CR-naming-scheme
branch
2 times, most recently
from
70accda to
15b57cb
Compare
- Typically Flyte is configured so that each project / domain has its
own Kubernetes namespace.
Certain environments may change this behavior by using the Flyteadmin
namespace_mapping setting to put all executions in fewer (or a singular)
Kubernetes namespace. This is problematic because it can lead to
collisions in the naming of the CR that flyteadmin generates.
- This patch fixes 2 important things to make this work properly inside
of Flyte:
* it adds a random element to the CR name in Flyte so that the CR is
named by the execution + some unique value when created by
flyteadmin
Without this change, an execution Foo in project A will prevent an
execution Foo in project B from launching, because the name of the
CR thats generated in Kubernetes *assumes* that the namespace the
CRs are put into is different for project A and project B
When namespace_mapping is set to a singular value, that assumption
is wrong
* it makes sure that when flytepropeller cleans up the CR resource
that it uses Kubernetes labels to find the correct CR -- so instead
of assuming that it can use the execution name, it instead uses the
project, domain and execution labels
- Use deterministic hash of execution id, name, project, as the FlyteWorkflow
CR name. Add workflow-cr-name-hash-length to flytepropeller config.
Signed-off-by: ddl-ebrown <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
ddl-ebrown
force-pushed
the
change-flyte-CR-naming-scheme
branch
from
15b57cb to
a268b61
Compare
@ddl-rliu did most of the work on this one - making this an upstream PR as it resolved a real issue for us.
Tracking issue
Why are the changes needed?
Typically Flyte is configured so that each project / domain has its
own Kubernetes namespace.
Certain environments may change this behavior by using the Flyteadmin
namespace_mapping setting to put all executions in fewer (or a singular)
Kubernetes namespace. This is problematic because it can lead to
collisions in the naming of the CR that flyteadmin generates.
What changes were proposed in this pull request?
This patch fixes 2 important things to make this work properly inside
of Flyte:
it adds a random element to the CR name in Flyte so that the CR is
named by the execution + some unique value when created by
flyteadmin
Without this change, an execution Foo in project A will prevent an
execution Foo in project B from launching, because the name of the
CR thats generated in Kubernetes assumes that the namespace the
CRs are put into is different for project A and project B
When namespace_mapping is set to a singular value, that assumption
is wrong
it makes sure that when flytepropeller cleans up the CR resource
that it uses Kubernetes labels to find the correct CR -- so instead
of assuming that it can use the execution name, it instead uses the
project, domain and execution labels
How was this patch tested?
This is deployed in a live Flyte setup where we have automated tests. We observed that the CR names were correctly unique after this and the initial collision no longer occurred.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link