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

Generalize createKindClusters.sh to create multiple clusters #131

Closed
wants to merge 3 commits into from

Conversation

imjasonh
Copy link
Contributor

@imjasonh imjasonh commented Jul 8, 2021

When given no args, it creates a us-east1 and us-west1 cluster as
before. When given args, it creates clusters with those names. KinD
configs, kubeconfigs and Cluster CRD YAMLs are generated and gitignored
to avoid churn and potentially leaking sensitive information.

When given no args, it creates a us-east1 and us-west1 cluster as
before. When given args, it creates clusters with those names. KinD
configs, kubeconfigs and Cluster CRD YAMLs are generated and gitignored
to avoid churn and potentially leaking sensitive information.
@imjasonh imjasonh requested a review from davidfestal July 8, 2021 18:25
@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #131 (0259385) into main (cb67159) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #131   +/-   ##
=======================================
  Coverage   20.75%   20.75%           
=======================================
  Files           5        5           
  Lines         318      318           
=======================================
  Hits           66       66           
  Misses        237      237           
  Partials       15       15           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb67159...0259385. Read the comment docs.

apiVersion: kind.x-k8s.io/v1alpha4
name: ${name}
networking:
apiServerAddress: "127.0.0.1"

Choose a reason for hiding this comment

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

just curious, what's the reason for apiServerAddress 127.0.0.1 ?

Copy link

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

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

One question (unrelated to this patch), but looks good to me.

@mangelajo
Copy link

@imjasonh @davidfestal can we merge this? I wanted to build a PR on top of it and it looks good to me.

@davidfestal
Copy link
Member

I'm OK with it, apart from the fact that, in the default case for the west cluster this PR removes the overriden k8s version (1.15.x) which is necessary for the apiNegociation test.
@imjasonh was it on purpose ?

Copy link
Member

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

In the default case for the west cluster this PR removes the overriden k8s version (1.15.x) which is necessary for the apiNegociation test.
@imjasonh was it on purpose ?

@mangelajo
Copy link

In the default case for the west cluster this PR removes the overriden k8s version (1.15.x) which is necessary for the apiNegociation test.
@imjasonh was it on purpose ?

Oh right, I guess it's accidental image: kindest/node:v1.15.12would be missing for the west cluster.

I guess we could have an array with the versions ?

@davidfestal
Copy link
Member

Oh right, I guess it's accidental image: kindest/node:v1.15.12would be missing for the west cluster.

I guess we could have an array with the versions ?

@imjasonh Do you agree this would be a good simple way to make this PR fully compatible with the previous behavior (especially for the apiNegociation demo) ?

@imjasonh
Copy link
Contributor Author

Oh right, I guess it's accidental image: kindest/node:v1.15.12would be missing for the west cluster.
I guess we could have an array with the versions ?

@imjasonh Do you agree this would be a good simple way to make this PR fully compatible with the previous behavior (especially for the apiNegociation demo) ?

How about createKindClusters.sh us-east us-west@1.15.2, and the script just checks for @ in the name?

The default would still be to create two clusters with the latest k8s version, but apiNegotiation would still be able to request an older version for one cluster.

@davidfestal
Copy link
Member

@imjasonh I assume I'm OK with this approach as well.

@mangelajo
Copy link

ping @imjasonh @davidfestal

@mangelajo
Copy link

Hey sorry, I realized I can make my work independent of this, please ignore my comments, no urge in merging.

kind create cluster --config ${DEMO_ROOT}/clusters/kind/us-east1.config --kubeconfig ${DEMO_ROOT}/clusters/kind/us-east1.kubeconfig
sed -e 's/^/ /' ${DEMO_ROOT}/clusters/kind/us-east1.kubeconfig | cat ${DEMO_ROOT}/clusters/us-east1.yaml - > ${DEMO_ROOT}/clusters/kind/us-east1.yaml
kind delete cluster --name=${name} || true
kind create cluster --config ${TMPDIR}/${name}.config --kubeconfig ${TMPDIR}/${name}.kubeconfig

Choose a reason for hiding this comment

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

Suggested change
kind create cluster --config ${TMPDIR}/${name}.config --kubeconfig ${TMPDIR}/${name}.kubeconfig
kind create cluster --config ${TMPDIR}/${name}.config --kubeconfig ${DEMO_ROOT}/clusters/kind/${name}.kubeconfig

Could we keep it like this?

And then add those .kubeconfigs to .gitignore ?

Otherwise for some work I'm doing later it's not possible to fund the kubeconfigs and use them

You can always rely on kind get kubeconfig --name $cluster but, if you have other kind cluster in the system we can't know if they belong to this demo or something else.

Copy link

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

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

check the kubeconfig comments.

kubeconfig: |
EOF
sed -e 's/^/ /' ${TMPDIR}/${name}.kubeconfig >> ${DEMO_ROOT}/clusters/${name}.yaml
done
Copy link

@mangelajo mangelajo Aug 18, 2021

Choose a reason for hiding this comment

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

I say this, because this kubeconfig .yaml "Cluster" kind is something I can't consume later down the road (I could strip it out again dynamically though).

Copy link
Member

Choose a reason for hiding this comment

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

I completely agree

@ncdc
Copy link
Member

ncdc commented Dec 9, 2021

Going through old-ish PRs - @imjasonh do you want to revive this & get it in? Want someone to take it over? Close it? Thanks!

@stevekuznetsov
Copy link
Contributor

I wrote Go code to do the Cluster creation with the kubeconfig for a cluster, would rather us have more generic Go than Bash TBH

@ncdc
Copy link
Member

ncdc commented Dec 10, 2021

SGTM. Would like to hear from Jason before closing, though.

@ncdc
Copy link
Member

ncdc commented Feb 9, 2022

@imjasonh based on yesterday's community meeting discussion about not wanting to maintain the demo framework, is it ok to close this?

@imjasonh imjasonh closed this Feb 23, 2022
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

5 participants