Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

command/task evaluation concurrency issue #245

Closed
grantzvolsky opened this issue Dec 28, 2019 · 8 comments
Closed

command/task evaluation concurrency issue #245

grantzvolsky opened this issue Dec 28, 2019 · 8 comments

Comments

@grantzvolsky
Copy link

grantzvolsky commented Dec 28, 2019

I made a simple tool to evaluate k8s secrets [edit: a working version of the tool is available here], but its output is not deterministic due to a concurrency issue that we discussed earlier with Marcel.

Code:

package argmin

import("tool/cli")
import("tool/exec")
import("strings")
import("encoding/json")

/////////////////////
// Domain definitions
/////////////////////
EvaluableSecret:: {
  name: string
  values: [K=string]: string
}

Secret:: {
  apiVersion: "v1"
  kind: "Secret"
  metadata: name: string
  data: [K=string]: string
}

//////////
// Secrets
//////////
evaluableSecrets: [...EvaluableSecret]
evaluableSecrets: [
  {
    name: "k8s-secret-1"
    values: "sql-server-connection-string": "echo secret-connection-string"
    values: "some-other-secret": "echo another-secret"
  },
  {
    name: "k8s-secret-2"
    values: "cert.pem": "echo cert-and-key"
  }
]

//////////////////////////
// evaluatesecrets command
//////////////////////////
command: evaluatesecrets: {
  for evaluableSecretKey, evaluableSecret in evaluableSecrets {
    for k, v in evaluableSecret.values {
      task: "\(evaluableSecretKey)-\(k)": exec.Run & {cmd: v, secretKey: "\(evaluableSecretKey)", valueKey: "\(k)", stdout: string}
    }
    task: print: cli.Print & {
      evaluedSecrets: "\(evaluableSecretKey)": Secret & {
        metadata: name: "\(evaluableSecrets[evaluableSecretKey].name)"
        for k, v in task if k != "print" { data: "\(v.valueKey)": v.stdout }
      }

      text: strings.Join([json.Marshal(x) for x in evaluedSecrets], "\n")
    }
  }
}

When I run cue evaluatesecrets ./..., the output is either 1):

{"metadata":{"name":"k8s-secret-1"},"apiVersion":"v1","kind":"Secret","data":{"sql-server-connection-string":"secret-connection-string\n","some-other-secret":"another-secret\n","cert.pem":"cert-and-key\n"}}
{"metadata":{"name":"k8s-secret-2"},"apiVersion":"v1","kind":"Secret","data":{"sql-server-connection-string":"secret-connection-string\n","some-other-secret":"another-secret\n","cert.pem":"cert-and-key\n"}}

or 2)

command.evaluatesecrets.task.print.text: cannot convert incomplete value "string" to JSON:
    ./eval_secrets_tool.cue:20:21
    ./eval_secrets_tool.cue:45:125
    tool/exec:9:20

Cue version 0.0.15 yields the error 2) in 100% of cases. latest cue build yields 2) in cca 75% of cases.

@grantzvolsky
Copy link
Author

grantzvolsky commented Jan 22, 2020

I believe the problem is that task dependency resolution fails to recognize the dependencies introduced by the line

        for k, v in task if k != "print" { data: "\(v.valueKey)": v.stdout }

Would it be possible to somehow explicitly mark the print task as dependent on all the other tasks?

@mpvl
Copy link
Contributor

mpvl commented Jan 30, 2020

@grantzvolsky thanks for digging in to that.

Yes, in tip you can set $after in a task to a list of tasks that should complete before it.

I'm curious to see if that would work for you.

@grantzvolsky
Copy link
Author

@mpvl Thanks for the suggestion. I ran a couple of tests and here's what I found:

  1. $after: "t1" or $after: ["t1"] doesn't print an error
  2. $after: t1 works
  3. $after: taskGroup doesn't work
  4. cyclic dependencies result in stack overflow

Would you like me to write the relevant test cases? If I understand it correctly, it suffices to define them in cmd/cue/cmd/testdata/script/.

@mpvl
Copy link
Contributor

mpvl commented Feb 7, 2020

Yeah, that would be great. Is $after: taskGroup documented to work? It would either way indeed be pretty useful.

Thanks!

@mpvl
Copy link
Contributor

mpvl commented Feb 8, 2020

@grantzvolsky See https://cue-review.googlesource.com/c/cue/+/4906. This improves error messages and allows depending on a group of tasks.

@mpvl
Copy link
Contributor

mpvl commented Feb 8, 2020

@jlongtine you may be interested in this as well.

mpvl added a commit that referenced this issue Feb 9, 2020
- report error if not a reference
- reference must be exact (may not point inside task)
- reference may refer to task group

Issue #245

Change-Id: Ib210ab38e0cb6bb750d4c020f9e7138442169ac3
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/4906
Reviewed-by: Grant Zvolský <grant@zvolsky.org>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
mpvl pushed a commit that referenced this issue Feb 17, 2020
The existing implementatin resulted in an infinite loop in two cases:

1. In the v.Equals(after) call when the 'after' dependencies were cyclic
2. In the task.Walk function when task fields cyclically referenced
other tasks

This commit solves both cases by
- stepping out of the Walk lambda when it had already been evaluated
with the given parameter; and
- removing the special case for 'after' dependencies as it is no longer
necessary.

Relevent tests are included.

Updates #245

Change-Id: I914ff9aad96eb43ae89964000dea60ccd44c38a2
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/4910
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
@mpvl
Copy link
Contributor

mpvl commented Nov 21, 2020

This seems to have been fixed with the move to tools/flow (as expected).

@cueckoo
Copy link

cueckoo commented Jul 3, 2021

This issue has been migrated to cue-lang/cue#245.

For more details about CUE's migration to a new home, please see cue-lang/cue#1078.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants