-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Integrate Helm operator into SDK #776
Conversation
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.
Out of curiosity, is there a reason why we're copying code verbatim from helm-app-operator-kit
? Are we getting rid of that repo?
# | ||
# See: http://stackoverflow.com/questions/3338030/multiple-bash-traps-for-the-same-signal | ||
#=================================================================== | ||
trap_add() { |
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.
We should throw this into hack/lib/test_lib.sh
since hack/ci/e2e-ansible.sh
uses it as well.
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.
Agreed. There's duplication that could be reduced in other areas as well:
pkg/helm/internal/util/diff.go
andpkg/scaffold/internal/testutil/test_util.go
have identicalDiff
functionspkg/scaffold/ansible
andpkg/scaffold/helm
are very similar, especiallyoperator.go
andwatches.go
.
I'm fine with taking a stab at reducing that in this PR, but maybe it makes more sense to take care of those things in a follow-up PR since this is already huge as it is?
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 prefer to make those consolidations in a follow on, I think it would be easier for reviews and we can then take the time to potentially design a nice API interface to the watches file that we can share. I think there is more to this then just sharing a template IMO. Thoughts?
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.
SGTM
That hasn't been explicitly decided, but my assumption is that at the very least we'll follow up in that repo by removing |
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.
Overall 🎉 :) Some nits, suggestions and questions.
@@ -171,6 +181,55 @@ func upLocalAnsible() { | |||
log.Info("Exiting.") | |||
} | |||
|
|||
func upLocalHelm() { |
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.
Just a suggestion, but maybe it would make sense to split these functions into separate files. For example, it's all already part of the up pkg, so we can just place this in the helm.go
file. Same would be nice to do with upLocalAnsible, to just leave logic in this file that is shared amongst the different types. More of a nit really. :)
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 file contains what are effectively the main
functions for the ansible and helm operators that we would use to build the binaries for the ansible and helm base images.
Maybe we should extract these into a common package that can be used by the ansible and helm main
packages and by operator-sdk up local
. Thoughts?
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.
Yes that makes sense, but doing that in a follow up PR is fine as well. 👍
hack/tests/e2e-helm.sh
Outdated
|
||
# create CR | ||
kubectl create -f deploy/crds/helm_v1alpha1_memcached_cr.yaml | ||
trap_add 'kubectl delete --ignore-not-found -f ${DIR2}/deploy/crds/helm_v1alpha1_memcached_cr.yaml' EXIT |
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.
TIL: --ignore-not-found
Thanks! :)
I do have one question: I tried to test this locally by running
Is there something I was missing while creating the project? |
@lilic Yes, this was something @shawn-hurley @hasbro17 and I discussed. The problem is that the The decision was that we'd make it work for We tossed around some ideas about how we might be able to make this work "magically":
In the end, we decided against any of them for the sake of explicitness and simplicity, at least initially. |
@joelanford: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
…oading memcached chart
Makes sense, is it possible we might want to follow up on this to add some logs or warnings to be printed out when users run |
I think so, and this also applies to the |
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.
One question, otherwise 👍
version = "2.11.0" | ||
|
||
[[override]] | ||
name = "github.com/russross/blackfriday" |
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.
Why do we need to override this? Just curious about the reason behind this. Thanks!
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.
github.com/russross/blackfriday
is depended on by k8s.io/kubernetes
, which doesn't have dep files. I'm not totally familiar with how dep chooses dependencies, but in this case it chooses the latest tag (v2.0.1) which is incompatible with k8s.io/kubernetes
usage of it. I'll update this to specify the latest v1 tag (1.5.2), which does work.
github.com/docker/distribution
is also depended on by k8s.io/kubernetes
. The latest stable tag in that repo is 2.6.2, which is what dep chooses without an override. That version is not compatible. There is a 2.7.0 pre-release tag that would work. I'm not sure if 2.7.0-rc.0
or master
would be better.
github.com/docker/docker
is also depended on by k8s.io/kubernetes
. Without the override, dep chooses 1.6.0-rc5, which is incompatible with k8s.io/kubernetes
usage of it. The main problem with more recent tagged versions is that they use github.com/Sirupsen/logrus
and we use github.com/sirupsen/logrus
, so dep refuses to pull a more recent version. The latest master switches to github.com/sirupsen/logrus
, but for some reason, docker devs don't seem to be tagging that repo anymore.
I also noticed that I need to use "=1.11.2" instead of "1.11.2" for the kubernetes override to force it to pick 1.11.2 exactly, so I'll make that change as well.
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.
Here are some docker GitHub issues which describe the problems with the docker deps:
- How to vendor docker 17.06.0-ce? moby/moby#33989
- semantic versioning, support vendoring via dep moby/moby#37281
Perhaps we should take this approach? moby/moby#33989 (comment)
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's a shame we need to import k8s.io/kubernetes
in the first place just to satisfy tillers need for the different clients it uses.
But that made me look at https://github.com/helm/helm/blob/2e55dbe1fdb5fdb96b75ff144a339489417b146b/glide.lock in case you find some inspiration there.
All makes sense, thanks for the explanation! Maybe we could add that to the commit msg or a short comment in the Gopkg.toml? WDYT?
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.
Yeah I like the approach of taking helm's locked dependencies. I'll update Gopkg.toml
to use the revision hashes directly and add a comment in there about why were doing it that way.
@@ -171,6 +181,55 @@ func upLocalAnsible() { | |||
log.Info("Exiting.") | |||
} | |||
|
|||
func upLocalHelm() { | |||
// Set the kubeconfig that the manager will be able to grab | |||
os.Setenv(k8sutil.KubeConfigEnvVar, kubeConfig) |
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.
Check return error?
} | ||
|
||
func main() { | ||
flag.Parse() |
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.
Do we have any defined flags that we're parsing?
The ansible-operator uses this for setting the reconcile/resync period.
https://github.com/water-hole/ansible-operator/blob/master/cmd/manager/main.go#L21
For helm we have that set to 5 seconds below.
Slight tangent but I forget if we could configure that per CR.
https://github.com/water-hole/ansible-operator/blob/master/vendor/github.com/operator-framework/operator-sdk/pkg/ansible/controller/reconcile.go#L44-L47
Doesn't seem like we do that right now for helm, but I forgot if we're planning to do that.
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.
There are not flags defined by the helm operator itself, but there are flags defined in github.com/golang/glog and sigs.k8s.io/controller-ruintime
We don't have a command-line configurable or watches.yaml configurable reconcile periods, and I don't think we've talked about it for the Helm operator. We could plan to add that in a follow-on PR.
@shawn-hurley also mentioned dynamically adding watches for created objects. Not sure if we're thinking that will replace or augment the periodic reconciliation though.
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.
LGTM after the error check nit.
Description of the change:
Adding support to the SDK for creating and running a Helm-based operator: This PR adds:
pkg/helm
, which contains Go code for running a Helm operatorjackfan.us.kg/operator-framework/helm-app-operator-kit/helm-app-operator/pkg/helm
.operator-sdk new
) and running it locally (operator-sdk up local
).Motivation for the change:
See the Helm operator proposal