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

Update cluster discovery #778

Merged
merged 1 commit into from
Feb 10, 2021
Merged

Update cluster discovery #778

merged 1 commit into from
Feb 10, 2021

Conversation

dturn
Copy link
Contributor

@dturn dturn commented Dec 18, 2020

What are you trying to accomplish with this PR?
Remove the need for a hard coded GVK overide list: #699
Fix v.19 & 1.20 compatiblity

How is this accomplished?
Query the raw api to determine group/versin membership per kind

What could go wrong?
The code is wrong and everything blows up because apply to told to prune the wrong things or things that doesn't exist.

** Notes **

How to generate new fixtures:
kubectl get --raw / > test/fixtures/for_unit_tests/api_raw.txt

require 'json'
raw_response = `kubectl get --raw /`
paths = JSON.parse(raw_response)["paths"].select { |p| %r{^\/api.*\/v.*$}.match(p) }
paths.each { |p| `kubectl get --raw #{p} >  test/fixtures/for_unit_tests/apis/#{p.gsub('/','_')}.txt` }

How kubeclient does this https://github.com/abonas/kubeclient/blob/master/lib/kubeclient/common.rb#L600

@dturn dturn force-pushed the discover-fix branch 3 times, most recently from 4fba280 to 8efba14 Compare December 19, 2020 00:50
@dturn dturn force-pushed the discover-fix branch 2 times, most recently from 875c6b0 to ec9c323 Compare January 5, 2021 00:25
@karanthukral karanthukral force-pushed the ci-v120 branch 2 times, most recently from fd3997e to 6d1bd36 Compare January 5, 2021 16:46
Base automatically changed from ci-v120 to master January 6, 2021 19:24
@dturn dturn force-pushed the discover-fix branch 4 times, most recently from 89e9ea4 to 04fa7dc Compare January 22, 2021 21:56
@dturn dturn force-pushed the discover-fix branch 2 times, most recently from 97a1db8 to 39ec876 Compare January 26, 2021 23:58
Copy link
Contributor

@timothysmith0609 timothysmith0609 left a comment

Choose a reason for hiding this comment

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

I think it's worth considering a bit more the threadsafetiness of fetching api paths

@dturn dturn marked this pull request as ready for review February 5, 2021 00:21
@dturn dturn requested a review from a team as a code owner February 5, 2021 00:21
"TCPIngress" => "v1beta1" }
def fetch_api_path(path)
@api_path_cache[path] ||= begin
raw_json, err, st = kubectl.run("get", "--raw", path, attempts: 1, use_namespace: false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flipping this from attempts: 2 to attempts: 1 didn't speed up the tests.

@dturn dturn requested a review from karanthukral February 9, 2021 23:48
Copy link
Contributor

@karanthukral karanthukral left a comment

Choose a reason for hiding this comment

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

The approach makes a lot of sense! 👏

Use a cache and concurency to be faster
@dturn
Copy link
Contributor Author

dturn commented Feb 10, 2021

I've squashed and rebased this PR on master.

For 🎩 :

I manually deployed the hello-cloud fixtures on v1.15.10 minikube and then removed resources to test pruning.

I did the same thing with minikube start --network-plugin=cni --cni=calico which gives you a v1.20.0 cluster.

@sjagoe
Copy link

sjagoe commented Feb 12, 2021

This (potentially) requires extra permissions on the cluster in order to be able to deploy with krane. Could this requirement be documented?

kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: krane-deploy
rules:
- nonResourceURLs: ["/"]
  verbs: ["get"]

@airhorns
Copy link
Contributor

WOOOOO!!!!!!

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.

5 participants