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

Remove k8s.io/kubernetes dependency #2316

Closed

Conversation

michaelbeaumont
Copy link
Contributor

@michaelbeaumont michaelbeaumont commented Jun 10, 2020

Description

According to
golang/go#32776 (comment), it's
not supposed to be used as a dependency.

Of course, hardcoding these labels isn't great but they shouldn't change too
often.

Additionally, I think this code might end up being removed if we agree
with #2197 (comment).

Closes #1537

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Added labels for change area (e.g. area/nodegroup), target version (e.g. version/0.12.0) and kind (e.g. kind/improvement)
  • Make sure the title of the PR is a good description that can go into the release notes

According to
golang/go#32776 (comment), it's
not supposed to be used as a dependency.

Additionally, I think this code might end up being removed if we agree
with eksctl-io#2197 (comment).

Closes eksctl-io#1537
Copy link
Collaborator

@cPu1 cPu1 left a comment

Choose a reason for hiding this comment

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

This change can have the potential issue of the code becoming obsolete. We'd have to keep a watch on the upstream kubelet code, especially when adding support for new EKS versions. If the kubelet later disallows setting a previously-supported label, the nodegroup creation would break without surfacing any useful errors to the user. Granted that even when consuming it as a library, we'd still need to keep its version updated, but this is easier to do than manually keeping a check on the upstream repo and figuring out the new changes that need to be copied.

As to the comment #2197 (comment), that cannot be easily supported (at least not as part of the bootstrapping process) as the kubelet itself will fail to start if it's passed any of the disallowed labels.

martina-if
martina-if previously approved these changes Jun 12, 2020
@michaelbeaumont
Copy link
Contributor Author

@cPu1
yeah that's an issue but I can't see any other way. The label checking logic isn't exported in k8s.io/kubelet or anywhere else (as far as I can tell?)
ultimately i think that comment is saying we shouldn't be using kubelet labels to set node labels in the first place.

@cPu1
Copy link
Collaborator

cPu1 commented Jun 12, 2020

ultimately i think that comment is saying we shouldn't be using kubelet labels to set node labels in the first place.

It is, but the labels feature (nodeGroup.labels) relies on support from the kubelet. So if the kubelet doesn't allow setting a label, supporting it in eksctl would be more complex to implement.

@@ -61,7 +61,6 @@ require (
k8s.io/code-generator v0.16.8
k8s.io/kops v1.15.2
k8s.io/kubelet v0.16.8
k8s.io/kubernetes v1.16.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question! when I remove it, it gets pulled into go.sum anyway at v1.15.3, I believe because of kops. Perhaps it's safer to leave it?

Copy link
Contributor

@sayboras sayboras Jun 17, 2020

Choose a reason for hiding this comment

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

Just my experience with go module :)

I just enable verbose mode in go build to actually find out exactly what libraries will be used. I don't think k8s.io/kubernetes package is used anywhere. go.mod sometimes tries to be smart while deriving dependencies transitively.

$ go clean --cache
$  CGO_ENABLED=0 go build -v ./cmd/eksctl > package.txt 2>&1 
$ wc -l package.txt 
775 package.txt
$ rg "k8s.io/kubernetes" package.txt   

@martina-if
Copy link
Contributor

Let's see if there is a better place for this code like the kubelet module and open a PR for it.

@michaelbeaumont michaelbeaumont dismissed martina-if’s stale review June 17, 2020 14:18

Going to see if we can move the code out of k8s.io/k8s into the right package

@michaelbeaumont
Copy link
Contributor Author

Opened a PR to kubernetes/kubernetes

@michaelbeaumont
Copy link
Contributor Author

Waiting on the above PR

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

Successfully merging this pull request may close these issues.

incorrect usage of k8s.io/kubernetes
4 participants