-
Notifications
You must be signed in to change notification settings - Fork 103
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
KEP-17: Pipe task implementation #1105
Conversation
Summary: as defined in KEP-0017 we introduce a new Pipe task. Given a task specification like: ``` tasks: - name: genfiles kind: Pipe spec: container: container.yaml pipe: - file: /tmp/foo.txt kind: Secret # or ConfigMap key: foo ``` KUDO will: - create a Pod with the provided `container.yaml` - wait for successful execution of the container - copy out specified pipe files - store them in the API server (in the above example as a Secret) - the secret can be referenced in the subsequent resources as `{{Pipes.foo}}` For more information about the implementation please consult the KEP-0017. Fixes: #774
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.
I did not do a very deep review so far but I liked what I saw 👍 left couple of minor comments :)
pkg/engine/task/task.go
Outdated
} | ||
|
||
var ( | ||
pipeFileKeyRe = regexp.MustCompile(`^[a-zA-Z0-9_\-]+$`) //a-z, A-Z, 0-9, _ and - are allowed |
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.
This is basically a param-name, right? Do we have a specification/pattern for params somewhere else already? Might be good to reuse that, if we have it
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.
As far as I can see we're only testing for invalid characters. Am I being too strict here? 🤔
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.
Hmmmm. We should allow all parameter names that K8s allows. Otherwise we might run into a problem where some application expects a parameter name of "option=rw,size=2300"
. We just had that case in Marathon, where some application abused the parameter name to pass multiple things :-/
In any case, the validation should be the same as is used in the verify that you linked.
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.
This looks great! Found a few nits though, all unrelated to the actual implementation.
additionally, instance controller doesn't not reconcile on pipe-pod deletion events
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.
I would really like to see more tests around the mapping to a template file...
however this LGTM!
@@ -107,21 +110,19 @@ spec: | |||
volumes: | |||
- name: cert | |||
secret: | |||
secretName: {{.Pipes.Certificate}} | |||
secretName: {{.Pipes.Mycertificate}} |
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.
it looks like pipe keys are generated to guarantee uniques based on phase and step... but then are flatten in the template... is that true?
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.
I'm not sure I understand the question but:
- pipe keys are not generated but chosen by the user e.g.
{{.Pipes.Mycertificate}}
in line 56 above - the actual name of the secret (where the file is stored) is generated by KUDO and is an implementation detail. I mentioned it here for completeness
test/integration/upgrade-command/first-operator/templates/configmap.yaml
Show resolved
Hide resolved
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.
I guess nothing is really a blocker for me - there's definitely a lot of possible improvements, but we can iterate on that. The only part I am really concerned is the tricky code around streams and goroutines - it's not really tested that much and I personally would be really scared to touch it in any way. I wonder if we can still do better there :)
test/integration/upgrade-command/first-operator/templates/configmap.yaml
Show resolved
Hide resolved
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.
This looks in a great shape already, my comments are mostly minor and around Goroutine use. Need to do another pass though tomorrow at pkg/engine/task/*
because that's a lot of code.
and with them goroutines and a lot of complexity.
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.
Finally could look at task_pipe.go
. It looks great, though I'm concerned with leaking resources in some of the code.
additionally PodExec caller can now differentiate between command execution failures (exit code > 0) and other errors
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!
Summary:
as defined in KEP-0017 we introduce a new Pipe task. Given a task specification like:
KUDO will:
pipe-pod.yaml
{{Pipes.foo}}
For more information about the implementation please consult the KEP-0017.
Fixes: #774