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

Add the used Kubernetes Version to kudo version output #1671

Merged
merged 4 commits into from
Sep 9, 2020

Conversation

ANeumann82
Copy link
Member

Signed-off-by: Andreas Neumann aneumann@mesosphere.com

What this PR does / why we need it:

This PR shows the used Kubernetes API in the version output. Helps users to understand which version of kubernetes is supported, see #1642

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great stuff and a welcomed addition... my issue with it is the naming of things (including the title of the PR). I'm against the use of the word kubernetesapi it is not. It is a kubernetes version which very indirectly supports an api. I see that you are getting the version of the API from k8s.io/api so it seems like it defines the API... but this is a client side dependency. The real API is at the server.

The version of k8s.io/api is selected when we bump the client-go library and we keep those in-sync. I wonder if it would be better to expose our client-go version or this "k8s api version" as the client-go version... then people will have somewhere to go to understand the mapping to their server version.

If we don't change to client-go version... then I do feel like we need to express this NOT as a kubernetes API version... but as a client kubernetes API version (which could be confusing... although accurate and seemingly necessary)

GitVersion: gitVersion,
GitCommit: gitCommit,
BuildDate: buildDate,
GoVersion: runtime.Version(),
Compiler: runtime.Compiler,
Platform: fmt.Sprintf("%s/%s", runtime.GOOS, runtime.GOARCH),
}

if info, ok := debug.ReadBuildInfo(); ok {
Copy link
Member

@kensipe kensipe Sep 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked into it.. but I'm also questioning if this complexity of reading build info at runtime is necessary. It seems clever... but it is a very indirect way to have a dependency. not that this is likely to happen... but what if the dependency is removed or the naming changes... This is fine if there is no other way.. but is there a way to get the version directly from client-go?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a way to get the version directly from client-go. There is a version package inside, but using
version.Get().GitVersion results in a string of "v0.0.0-master+$Format:%h$".

From what I read is the debug.ReadBuildInfo() the correct way to get information about the used modules.

Copy link
Member

@nfnt nfnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with all that @kensipe said: We have to be clear here that this is the version of the Kubernetes client API and this isn't mistaken for the server API. No strong opinion around debug.ReadBuildInfo(), but if there's an easy way to retrieve the version directly from client-go, let's use that instead.

Use version from client-go library

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
@ANeumann82 ANeumann82 changed the title Add the used Kubernetes API to version output Add the used Kubernetes Version to kudo version output Sep 7, 2020
@ANeumann82
Copy link
Member Author

This is great stuff and a welcomed addition... my issue with it is the naming of things (including the title of the PR). I'm against the use of the word kubernetesapi it is not. It is a kubernetes version which very indirectly supports an api. I see that you are getting the version of the API from k8s.io/api so it seems like it defines the API... but this is a client side dependency. The real API is at the server.

The version of k8s.io/api is selected when we bump the client-go library and we keep those in-sync. I wonder if it would be better to expose our client-go version or this "k8s api version" as the client-go version... then people will have somewhere to go to understand the mapping to their server version.

If we don't change to client-go version... then I do feel like we need to express this NOT as a kubernetes API version... but as a client kubernetes API version (which could be confusing... although accurate and seemingly necessary)

I have renamed the field to KubernetesVersion and retrieve it from the client-go library now. You're correct that this is the top-level library that we should use to determine which version we use.

I don't think it makes sense to expose this version as the ClientGo version. It doesn't help the user, and making the jump from "ClientGo" to "KubernetesVersion" might be a bit much.

Thankfully, all Kubernetes components are released together, so the Kubernetes 0.18.6 release contains the ClientGo 0.18.6.

I think KubernetesVersion makes the most sense and helps users to understand, wdyt?

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the information here is low level... git commit, go version... this information could only be for devs or for us. If we are to enable others, the meaningful metadata is client-go. If they know that it is client-go, then they can go to client-go matrix which is maintained to understand consequences. Anything else likely has has us duplicating all the client-go stuff (please no) and requires some guessing or assumptions... which might sound something like: is this the version of the kubectl on my local machine.. I know my server is a different version. let me upgrade my client kubectl... wait that didn't do it... my client is 1.19.2, the server is 1.19.5... but kudo is reporting 1.18.5... what does that mean? how can I upgrade it?

It is better IMO to be clear when we can. why would we hide it? why make the user guess? why add another layer of translation? It isn't clear what it solves.

I expect all operators to have this need: 1. express what version of client-go they use and 2. what version of controller-runtime they use... which defines their constraints.

@kensipe
Copy link
Member

kensipe commented Sep 8, 2020

if you are trying to stay away from "ClientGo"... that makes sense! Could we call it "ClientKubernetesVersion" or "KubernetesClientGoVersion"... leaving the word kubernetes out wouldn't be good. ah... naming is hard and this is better than the previous options. Whatever you think is best.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
@ANeumann82
Copy link
Member Author

Thanks. I see your point. I've renamed it to KubernetesClientVersion, I think that fits pretty good.

@ANeumann82 ANeumann82 merged commit 6fda0fa into main Sep 9, 2020
@ANeumann82 ANeumann82 deleted the an/add-k8s-api-to-version-output branch September 9, 2020 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants