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

Local Garden Dev on Windows #2578

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

guydc
Copy link
Contributor

@guydc guydc commented Jul 11, 2020

How to categorize this PR?
/area dev-productivity
/kind enhancement
/priority normal

What this PR does / why we need it:

Fixes local-garden containers' host machine address and api-server binding, such that the nodeless gardener dev env will work on WSL2 with WSL2-integrated docker for windows.

Release note:

The Nodeless Development Environment also works on windows, using WSL2 and docker for windows

@guydc guydc requested a review from a team as a code owner July 11, 2020 06:59
@gardener-robot gardener-robot added area/dev-productivity Developer productivity related (how to improve development) kind/enhancement Enhancement, improvement, extension priority/normal labels Jul 11, 2020
@vpnachev
Copy link
Member

Thanks for the contribution of support for local setup running on windows.

Could you add some info about it in the local setup. Basically a disclaimer that windows is supported for local development (we have not declared so far that it was working only on MacOS and Linux distros, this will have to be mentioned, too)
Also, list the limitations and prerequisites, if any.

@guydc guydc force-pushed the feature/dev-nodeless-wsl2 branch from 760b5f9 to d14627b Compare July 12, 2020 06:33
timebertt
timebertt previously approved these changes Jul 13, 2020
Copy link
Member

@timebertt timebertt left a comment

Choose a reason for hiding this comment

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

Thanks for enabling the local setup on WSL!
/lgtm from a code perspective, but I can't test it on windows...

/ok-to-test

@vpnachev
Copy link
Member

/ok-to-test

Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

Just a minor improvement of the documentation.
Otherwise looks good to me as I am also not able to validate it.

docs/development/local_setup.md Outdated Show resolved Hide resolved
@timebertt
Copy link
Member

/status author-action

@gardener-robot
Copy link

@guydaichs The pull request was assigned to you under author-action. Please unassign yourself when you are done. Thank you.

@danielfoehrKn
Copy link
Contributor

@mandelsoft are you able to test this as to my knowledge you at least have a windows machine?

@guydc
Copy link
Contributor Author

guydc commented Jul 16, 2020

There is one issue that I didn't manage to solve: gardenlet fails to establish the VPN tunnel to the shoot. I'm not sure if it's a general limitation, or something related to the firewall setup on my machine. It's not critical, as you can still develop most g/g features locally without this. Would you like me to add some disclaimer? Maybe label this option as best-effort/experimental ?

@timebertt timebertt requested a review from vpnachev July 16, 2020 05:47
@timebertt
Copy link
Member

timebertt commented Jul 16, 2020

There is one issue that I didn't manage to solve: gardenlet fails to establish the VPN tunnel to the shoot. I'm not sure if it's a general limitation, or something related to the firewall setup on my machine. It's not critical, as you can still develop most g/g features locally without this. Would you like me to add some disclaimer? Maybe label this option as best-effort/experimental ?

What do you mean by that? The gardenlet doesn't establish a VPN tunnel to the shoots, only the kube-apiserver does (if the KonnectivityTunnel feature gate is disabled). Do you mean the step in the Shoot reconciliation where gardenlet tries to check, if the VPN/tunnel connection works?
I'm not sure, but I think this shouldn't be related to your firewall setup, as this is just a plain REST call to the /portforward subresource on the VPN pod with SPDY transport (I think).

vpnachev
vpnachev previously approved these changes Jul 16, 2020
Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

/lgtm
/reviewed ok-to-test

Copy link
Member

@timebertt timebertt left a comment

Choose a reason for hiding this comment

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

@guydaichs and I checked offline and the problem was not due to the WSL setup and rather misconfiguration in the Shoot's networking section.
So, not related this PR :)
/lgtm
/approve

@gardener gardener deleted a comment from gardener-robot Jul 16, 2020
Copy link
Member

@timebertt timebertt left a comment

Choose a reason for hiding this comment

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

Can you run git commit --amend --reset-author to kick out Gardener CI/CD as commiter?

@guydc
Copy link
Contributor Author

guydc commented Jul 16, 2020

Thanks @tim-ebert. Indeed, the entire local dev env works after fixing my misconfiguration. I fixed the commit author as well.

Copy link
Member

@timebertt timebertt left a comment

Choose a reason for hiding this comment

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

Thanks!
/lgtm

Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

/lgtm

@vpnachev vpnachev merged commit 65050f2 into gardener:master Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dev-productivity Developer productivity related (how to improve development) kind/enhancement Enhancement, improvement, extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants