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

Highly sub-optimal UX of make cli-install at HEAD of master #1549

Closed
porridge opened this issue Jun 4, 2020 · 8 comments · Fixed by #1556
Closed

Highly sub-optimal UX of make cli-install at HEAD of master #1549

porridge opened this issue Jun 4, 2020 · 8 comments · Fixed by #1556
Labels

Comments

@porridge
Copy link
Member

porridge commented Jun 4, 2020

What happened:

A user followed dubious KUDO installation instructions on a third-party site (see (3) Get started) and got a non-functional KUDO installation, where the API resources matched HEAD code (and so included a MutatingWebhookConfiguration unconditionally), except for the statefulset's docker image reference, which was kudobuilder/controller:v0.13.0 docker image (which requires an ENABLE_WEBHOOKS=true env variable to be set to actually serve the endpoint).

The result was:

Error: failed to install instance zookeeper-instance: installing Instance: Internal error occurred: failed calling webhook "instance-admission.kudo.dev": Post https://kudo-controller-manager-service.kudo-system.svc:443/admit-kudo-dev-v1beta1-instance?timeout=30s: dial tcp 10.108.53.102:443: connect: connection refused

What you expected to happen:

Several things:

Ideally, no end-user instructions should encourage installation of unreleased code from tip of master. We should ask Mayadata to point to our install instructions instead. Since we cannot find and weed out all such sites, we should also make it more awkward to use this method.

Secondly, when using CLI built from tip of master, then by default:

  • either the docker image used should be something matching the tip of master as closely as possible. Probably something like kudobuilder/controller:latest which should be built and pushed on every commit,
  • or the default should be to fail and require a docker image tag to be specified explicitly

How to reproduce it (as minimally and precisely as possible):

Follow the aforementioned instructions.

Anything else we need to know?:

The reason why v0.13.0 was used is because it gets the version from $LDFLAGS which we set with:

GIT_VERSION_PATH := github.com/kudobuilder/kudo/pkg/version.gitVersion
GIT_VERSION := $(shell git describe --abbrev=0 --tags | cut -b 2-)
[...]
LDFLAGS := -X ${GIT_VERSION_PATH}=${GIT_VERSION} -X ${GIT_COMMIT_PATH}=${GIT_COMMIT} -X ${BUILD_DATE_PATH}=${BUILD_DATE}

and at the point the user ran make cli-install a couple of days ago, the latest tag happened to be v0.13.0. Today it is v0.14.0-rc1.

More details in slack thread.

Environment:

  • Kubernetes version (use kubectl version):
  • Kudo version (use kubectl kudo version):
  • Operator:
  • operatorVersion:
  • Cloud provider or hardware configuration:
  • OS (e.g. from /etc/os-release):
  • Kernel (e.g. uname -a):
  • Install tools:
  • Others:
@porridge
Copy link
Member Author

porridge commented Jun 4, 2020

Note this is not the first time someone had an issue with installing KUDO like this. The first time we removed such instructions from our site: kudobuilder/kudo.dev#236 but apparently this is not enough.

@nfnt
Copy link
Member

nfnt commented Jun 4, 2020

Isn't the issue here that we're referencing the last released controller even if master is already ahead? Once a version of KUDO is released, we should consider every commit to master afterwards as commits towards the next version of KUDO and this should also be reflected in the controller image kubectl kudo init will use. I.e. it should reference the next, still unreleased image that users would have to build on their own.
Another issue is that we currently don't provide any backward-compatibility guarantees. But we should and plan/test for that accordingly.
So, referencing old versions on master plus no backward-compatibility guarantees means that there aren't any guarantees that master will work.

@porridge
Copy link
Member Author

porridge commented Jun 4, 2020

Isn't the issue here that [...]

Yes, I think this is a re-phrasing of what I wrote under:

Secondly [...]

In the initial message.

@nfnt
Copy link
Member

nfnt commented Jun 4, 2020

Yes, you're right.

@kensipe
Copy link
Member

kensipe commented Jun 4, 2020

it was definitely not good that we advertised make cli as an installation method on our website... that is fixed. We can't control what people do on their sites...

we should not pollute the dev workspace with messages that are meaningless to devs... if someone wants to "build" linux and has issues with their OS... yeah.. that is why we have packaged solutions.
the title of this issue seems completely wrong to me... make cli-install is optimal for it's purpose... it has a sub-optimal UX for those that don't know or understand the nuances of dev... for which we have optimal solutions for.

@kensipe
Copy link
Member

kensipe commented Jun 4, 2020

I don't agree with most of this issue as well:

  1. we should NOT spend time on defaulting an image "close" to master... there is an --kudo-image flag for managing the preferred image for devs
  2. and I'm strongly opposed to using the ":latest" flag... for lots of reasons.. but a simple 1 is when we have a breaking change, latest may be more sub-optimal.

The solution to me is clear... if you are not a dev... are not an advanced user... don't use the dev tools.

@mattj-io
Copy link
Contributor

mattj-io commented Jun 4, 2020

With regards to the original user issue, I have asked Murat at Mayadata to update their blog and just point to our install instructions.

@porridge
Copy link
Member Author

porridge commented Jun 4, 2020

Ken, can you please explain what you mean by:

if someone wants to "build" linux and has issues with their OS...

What "issues with OS" do you have in mind? The issue the user faced was not OS-related as far as I can tell. It was the fact that, at times, the CLI built on tip of master points to a controller image which is incompatible with the rest of it.

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

Successfully merging a pull request may close this issue.

4 participants