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

Check SMI CRDs version before starting the controller #714

Merged
merged 5 commits into from
Aug 27, 2020

Conversation

jspdown
Copy link
Contributor

@jspdown jspdown commented Aug 25, 2020

What does this PR do?

In order to improve the user experience when incompatible SMI CRDs versions are installed (outdated or not yet supported), this PR introduce some changes on the prepare command.

In the past we use to start informers and start collecting the different SMI objects already present on the cluster. If SMI CRDs were not installed or an unsupported version was we would get the following errors many times:

E0824 17:40:01.430644   26974 reflector.go:178] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:125: Failed to list *v1alpha3.TrafficSplit: the server could not find the requested resource (get trafficsplits.split.smi-spec.io)
E0824 17:40:01.430644   26974 reflector.go:178] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:125: Failed to list *v1alpha3.TrafficSplit: the server could not find the requested resource (get trafficsplits.split.smi-spec.io)
E0824 17:40:01.430644   26974 reflector.go:178] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:125: Failed to list *v1alpha3.TrafficSplit: the server could not find the requested resource (get trafficsplits.split.smi-spec.io)
E0824 17:40:01.430644   26974 reflector.go:178] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:125: Failed to list *v1alpha3.TrafficSplit: the server could not find the requested resource (get trafficsplits.split.smi-spec.io)
E0824 17:40:01.430644   26974 reflector.go:178] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:125: Failed to list *v1alpha3.TrafficSplit: the server could not find the requested resource (get trafficsplits.split.smi-spec.io)

This PR introduce a simpler solution based on the Discovery API of the kubernetes sdk to discover which resource groups are installed and what are there versions.

Fixes #711

How to test it

  • Install Maesh with Helm using the --skip-crds flag: The controller should fail and you should see a message in the init container logs telling you that the SMI CRDs are missing.
  • Install Maesh normally: This should work fine
  • Create a cluster, install an old version of the SMI CRDs (e.g. specs v1alpha) and install Maesh using Helm with the --skip-crds you should see an error in the logs too.

@jspdown jspdown requested a review from kevinpollet August 25, 2020 14:27
@jspdown jspdown changed the title Check SMI CRDs version before starting controller Check SMI CRDs version before starting the controller Aug 25, 2020
cmd/prepare/prepare.go Outdated Show resolved Hide resolved
cmd/prepare/prepare.go Outdated Show resolved Hide resolved
pkg/k8s/client.go Outdated Show resolved Hide resolved
pkg/k8s/smi.go Outdated Show resolved Hide resolved
pkg/k8s/smi.go Outdated Show resolved Hide resolved
Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@kevinpollet kevinpollet force-pushed the GH-711-prepare-simplified branch from 72f925d to e1cc26d Compare August 26, 2020 11:53
@jspdown jspdown force-pushed the GH-711-prepare-simplified branch from e1cc26d to 979e73f Compare August 26, 2020 14:21
@kevinpollet kevinpollet force-pushed the GH-711-prepare-simplified branch from b416ecc to 911c2e1 Compare August 26, 2020 20:13
@kevinpollet kevinpollet added this to the v1.4 milestone Aug 26, 2020
@traefiker traefiker merged commit 15b57b3 into traefik:master Aug 27, 2020
@jspdown jspdown deleted the GH-711-prepare-simplified branch September 7, 2020 07:51
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 this pull request may close these issues.

Prepare command shouldn't start informers
3 participants