Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

chore: make cluster creation work with kind v0.7.0 #1221

Conversation

makkes
Copy link
Contributor

@makkes makkes commented Apr 30, 2020

What this PR does / why we need it:
The command 'kind get kubeconfig-path' has been deprecated in kind
v0.6.0 and removed in v0.7.0 so the cluster creation script doesn't
work with the latter version. To account for this fact and other
changes (such as kind now respecting KUBECONFIG and falling back to
the same defaults that kubectl uses), I reworked some parts of the
script.

pre-commit now uses the kind imave v1.17.5 for tests.

Note: This change makes the scripts fail to work with
kind version < v0.7.0.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1220

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 30, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @makkes!

It looks like this is your first PR to kubernetes-sigs/kubefed 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubefed has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 30, 2020
@makkes
Copy link
Contributor Author

makkes commented Apr 30, 2020

/assign @irfanurrehman

@makkes makkes force-pushed the makkes/fix-cluster-creation-script branch from 550a381 to eda2aea Compare April 30, 2020 14:55
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Apr 30, 2020
@makkes makkes force-pushed the makkes/fix-cluster-creation-script branch from eda2aea to 3dcf807 Compare April 30, 2020 14:57
@makkes
Copy link
Contributor Author

makkes commented Apr 30, 2020

tests are failing for a reason that's not totally obvious to me. I'll have to have a further look.

@irfanurrehman
Copy link
Contributor

@makkes Absolutely appreciate you doing this. Does the same run fine locally for you ?

@hectorj2f hectorj2f changed the title chore: make cluster creation work with kind >= v0.6.0 chore: make cluster creation work with kind >= v0.7.0 May 1, 2020
@makkes
Copy link
Contributor Author

makkes commented May 2, 2020

@makkes Absolutely appreciate you doing this. Does the same run fine locally for you ?

@irfanurrehman, thanks for the feedback.

To answer your question: Not yet, as I had to figure out which tests are being run by Travis. Now I got everything set up and I can reproduce the failure. Fix incoming. This is slightly larger than I thought initially. 😌

@makkes
Copy link
Contributor Author

makkes commented May 2, 2020

@irfanurrehman, all tests pass locally now. 🤦‍♂️ 🤣

@hectorj2f
Copy link
Contributor

@makkes nice, I will give it a try myself.

scripts/pre-commit.sh Outdated Show resolved Hide resolved
@makkes makkes requested a review from hectorj2f May 3, 2020 11:53
@makkes makkes changed the title chore: make cluster creation work with kind >= v0.7.0 chore: make cluster creation work with kind v0.7.0 May 3, 2020
@makkes
Copy link
Contributor Author

makkes commented May 3, 2020

I created #1223 to track kind v0.8 support.

Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2020
@hectorj2f hectorj2f self-requested a review May 3, 2020 23:17
Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

@makkes please, clean the commit log. The code looks good to me.

The command 'kind get kubeconfig-path' has been deprecated in kind
v0.6.0 and removed in v0.7.0 so the cluster creation script doesn't
work with the latter version. To account for this fact and other
changes (such as kind now respecting `KUBECONFIG` and falling back to
the same defaults that `kubectl` uses), I reworked some parts of the
script.

pre-commit now uses the kind imave v1.17.5 for tests.

Note: This change makes the scripts fail to work with
      kind version < v0.7.0.
@makkes makkes force-pushed the makkes/fix-cluster-creation-script branch from da26ee8 to 27df67b Compare May 4, 2020 06:34
@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 4, 2020
@makkes
Copy link
Contributor Author

makkes commented May 4, 2020

@hectorj2f, I squashed all commits into one.

@makkes makkes requested a review from hectorj2f May 4, 2020 06:35
@hectorj2f
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorj2f, makkes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2020
@hectorj2f
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit ab94a92 into kubernetes-retired:master May 4, 2020
@makkes makkes deleted the makkes/fix-cluster-creation-script branch May 4, 2020 08:13
@irfanurrehman
Copy link
Contributor

@makkes @hectorj2f. Apologies for not getting to this earlier. I had a glance at this over the weekend and was supposed to spend time on this today. Anywhichways, I am curious if you did verify that the scripts work on mac?

@hectorj2f
Copy link
Contributor

@irfanurrehman I tested on Linux where I have my setup. But I am not sure if it ever worked on OSX out of the box.

@irfanurrehman
Copy link
Contributor

@irfanurrehman I tested on Linux where I have my setup. But I am not sure if it ever worked on OSX out of the box.

@hectorj2f you are right, it did not work out of the box for OSX. I was hoping that with the updated kind version, it might.

@hectorj2f
Copy link
Contributor

@irfanurrehman that might be a simple bug fix. I will take a look at it.

@makkes
Copy link
Contributor Author

makkes commented May 5, 2020

@irfanurrehman, @hectorj2f, making create-clusters.sh work with an insecure registry on macOS might be a slightly bigger effort as Docker for Mac runs inside of a VM so you can't easily change the daemon cfg.

@irfanurrehman
Copy link
Contributor

@irfanurrehman, @hectorj2f, making create-clusters.sh work with an insecure registry on macOS might be a slightly bigger effort as Docker for Mac runs inside of a VM so you can't easily change the daemon cfg.

@makkes we use docker exec to update the config and I think it behaves the same on OSX irrespective of where the container is actually running (Am I missing something?).

The tricky part to take care however is that the apiserver endpoint differs depending on where you are accessing the cluster from. If you access one kind cluster from within another, you need to use the container internal ip (which cannot be reached from local machine) whereas when you access it from local machine (for example the scripts and what is stored in the kubeconfig) it is 127.0.0.1:<some unique port>. This is different from linux and workaround(s) becomes a little hacky.

Having said that, I think we can live with what we currently have.. :)

@makkes
Copy link
Contributor Author

makkes commented May 5, 2020

we use docker exec to update the config

@irfanurrehman, this is true for the kind cluster but create-clusters.sh also changes the host's docker daemon config if CONFIGURE_INSECURE_REGISTRY_HOST is set to true:
https://github.com/kubernetes-sigs/kubefed/blob/ab94a922ef079329f86fa80b1ae220cb5c3cb15d/scripts/create-clusters.sh#L70
which is the case in pre-commit.sh:
https://github.com/kubernetes-sigs/kubefed/blob/ab94a922ef079329f86fa80b1ae220cb5c3cb15d/scripts/pre-commit.sh#L153

So create-clusters.sh works on macOS (if you don't provide the CONFIGURE_INSECURE_REGISTRY_HOST flag) while pre-commit.sh doesn't.

@irfanurrehman
Copy link
Contributor

Aah yes! I was missing that part. As I said earlier, I think we can live with what we currently have.. :)

@irfanurrehman
Copy link
Contributor

Meanwhile thanks @makkes for doing this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cluster creation script doesn't work with recent kind version
4 participants