Skip to content
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

namespace applied out of order #3421

Closed
nicks opened this issue Jun 8, 2020 · 5 comments · Fixed by #3567
Closed

namespace applied out of order #3421

nicks opened this issue Jun 8, 2020 · 5 comments · Fixed by #3567
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@nicks
Copy link
Member

nicks commented Jun 8, 2020

Create a Tiltfile with two bits of YAML

  1. A Namespace
  2. A Deployment that deploys to that namespace, but doesn't contain any locally-built images

Expected behavior:
Tilt should deploy the namespace before deploying the Deployment

Actual behavior:
Tilt tries to deploy them in parallel, and there's a race to see which one gets applied first

It's easier to repro if the uncategorized set of resources contains a bunch of unrelated resources that take time to apply (like CRDs). Seems like a regression of #2348, among others

@landism @maiamcc do either of you have ideas on how this is expected to work? I guess an easy way to fix it is to not parallelize the uncategorized yaml. A better way might be to try to infer resource deps

@nicks nicks added the bug Something isn't working label Jun 8, 2020
@maiamcc
Copy link
Contributor

maiamcc commented Jun 8, 2020

Yeah that's the expected behavior :( we're good at sorting YAMLs in a reasonable order within a single resource but bad at doing it between resources.

Spitballing some ways to address this:

  1. don't parallelize uncategorized YAML (this would def take care of this race condition, but could slow down the Tilt experience a lot if you don't actually have any resources that depend on uncategorized)
  2. infer dependencies automatically: resourceX depends on uncategorized IF it deploys to a namespace that's created in uncategorized (potentially still really slow if uncategorized contains a lot of junk)
  3. we could also provide users the means to declare uncategorized as a resource_dep in their Tiltfile (doesn't work out of the box/more config for user, but less work for us)
  4. infer dependencies --> reorganize resources: if we figure out that resourceX depends on namespaceA, we could add that entity to the resource much the same way that we add service entities to the resource that deploys pods matching the service. OR, assuming multiple resources depend on namespaceA, we make it its own resource, such that multiple resources can depend on it without waiting for ALL the uncategorized YAML (potentially the most sidebar explosion/magic, but the least waiting)
  5. encourage users to do resource organization themselves--the above approach should be doable manually with the new k8s_resource categorization code that @jazzdan recently wrote (everything will work, but not out of the box/will require finicky configuration)

The easiest seems to be #1

@landism
Copy link
Member

landism commented Jun 8, 2020

do either of you have ideas on how this is expected to work?

My understanding of how this used to work:

  1. we sort entities in kustomize order when we create the k8s target
  2. we ensure the uncategorized k8s target builds before other k8s targets

With parallel builds, these no longer suffice to ensure we apply namespaces first.

Both of the solutions you describe seem fine.

Relatedly, the new objects k8s_resource param allows users to stick namespaces into k8s resources, which could cause more of these sorts of problems, but at least in that case it's a bit more reasonable since the user is configuring it to do that.

@maiamcc maiamcc added the good first issue Good for newcomers label Jun 9, 2020
@maiamcc
Copy link
Contributor

maiamcc commented Jun 9, 2020

Tagging this "good first issue" IF the solution is "uncategorized yaml shouldn't be parallelizable". (the "infer dependencies" solution is quite a bit gnarlier)

@bobjackman
Copy link

Is this a possible solution for the comment on this line? https://github.com/tilt-dev/tilt-extensions/blob/master/helm_remote/Tiltfile#L41

@nicks
Copy link
Member Author

nicks commented Jun 12, 2020

Nope :(

Because that still registers the namespace as yaml, to be deployed in parallel with other yaml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
4 participants