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

Use operator-sdk binary as operator binary for helm and ansible #860

Closed
mhrivnak opened this issue Dec 18, 2018 · 11 comments
Closed

Use operator-sdk binary as operator binary for helm and ansible #860

mhrivnak opened this issue Dec 18, 2018 · 11 comments

Comments

@mhrivnak
Copy link
Member

Feature Request

Is your feature request related to a problem? Please describe.
The ansible operator and helm operator each need to build their own binary that will be run in production. Currently each type has to maintain (almost certainly in a separate repo) its own main.go, Gopkg.toml, Gopkg.lock, etc and facilitate a build and test workflow. The code in main.go overlaps with the operator-sdk up local feature.

Even if we use scaffolds inside the operator-sdk repo to generate all of the above files, their rendered artifacts still need to be maintained somewhere for reproducible builds.

Perhaps most importantly, the above workflow necessarily requires that each binary has its own lifecycle. Each would have its own versioned releases (even if we version them in lock-step), including features, bugs, bug fixes, vendored dependencies (hopefully a subset of what's in operator-sdk), etc.

Describe the solution you'd like
Since all the functionality of helm and ansible operators is already built into the operator-sdk binary, I propose that we add a command that simply runs the correct operator. It will be very similar to up local, but without the "local" parts. Some options that come to mind are:

operator-sdk up
operator-sdk run

With this capability, the operator-sdk binary itself can remove the need to build separate binaries. There will be one binary with one lifecycle.

@AlexNPavel
Copy link
Contributor

AlexNPavel commented Dec 18, 2018

That could work. The main disadvantage I see is that the binary would be larger. The ansible operator binary is currently ~38M, helm is ~52M, and the SDK is ~56M. This change might also increase the size of the sdk binary compared to what it is now.

Other than the size though, it would be nice having a single binary handle the sdk, go, ansible, and helm and not require separate code for the base images.

@joelanford
Copy link
Member

I like this idea.

IMO the binary size differences don't outweigh the benefit of having one binary, one lifecycle, and one implementation of each of the operators' entrypoints. And if I understand correctly, the SDK binary shouldn't grow much (if at all) since it already contains the up local implementations.

@mhrivnak Would you envision the CLI to be explicit (e.g. operator-sdk run ansible) or will we auto-detect the operator type in this scenario as well? I personally think it should be explicit.

@shawn-hurley
Copy link
Member

Would you envision the CLI to be explicit (e.g. operator-sdk run ansible) or will we auto-detect the operator type in this scenario as well? I personally think it should be explicit.

👍 on the explicit for me as well

@mhrivnak
Copy link
Member Author

The main disadvantage I see is that the binary would be larger.

Since all the type-specific code is already compiled in the sdk binary for use by "up local", I think there would be very little change in size after adding this command.

@AlexNPavel
Copy link
Contributor

The main disadvantage I see is that the binary would be larger.

Since all the type-specific code is already compiled in the sdk binary for use by "up local", I think there would be very little change in size after adding this command.

Yeah, looking at the up local code, it looks like it's essentially just the main.go files for ansible and helm copied over, so that shouldn't change the main binary size. And using the a larger binary for the ansible operator wouldn't really matter much since the base image is so large and the helm binary is almost the same size as the operator-sdk's binary. So the increase in size of the images would be pretty much negligible.

This seems like a good idea to me. It would make maintaining the code and images simpler.

@hasbro17
Copy link
Contributor

@mhrivnak @joelanford do we expect users to use the operator-sdk run command as opposed to operator-sdk up local?
From #887 and the discussion above it seems like that's more for the base images to use in their entrypoints.

exec ${OPERATOR} run ansible --watches-file=/opt/ansible/watches.yaml $@

If that's the case then we should clarify in the CLI guide and CHANGELOG that users should still use operator-sdk up local.

@joelanford
Copy link
Member

@hasbro17 I still expect operator-sdk up local to continue to be used as it is today for ansible and helm operators. IMO, the distinction is important since up local can make assumptions about the project tree that run commands cannot. The differences between up local and run are minimal right now, but the distinction gives us flexibility in the future.

If that's the case then we should clarify in the CLI guide and CHANGELOG that users should still use operator-sdk up local.

👍

@mhrivnak
Copy link
Member Author

My understanding:

run is for running the process inside a container that's inside a cluster. This is the operator's "natural environment". run cannot detect the operator type, because it does not have access to its source tree.

up local is for a developer to use in order to run the process outside a cluster, where the CWD is the source tree. up local does extra things to make this non-native environment look more like its natural environment, such as set environment variables.

@mhrivnak
Copy link
Member Author

@hasbro17 if there's anything about the naming or the UX you'd like to change, I'm very happy to chat about that, and we can always revise it. Mostly we needed this capability to land quickly so we can nail down the build process for ansible operators.

@hasbro17
Copy link
Contributor

Thank you both for clarifying. That's what i understood from operator-sdk run as well.

@mhrivnak The UX of run seems good to me. I just wanted to ensure that we document the distinction you've made above between run and up local in a follow up to #887

@hasbro17
Copy link
Contributor

Resolved via #887 and #897

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

No branches or pull requests

5 participants