-
Notifications
You must be signed in to change notification settings - Fork 25
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
🌱 Add new management cluster environment option #1163
base: main
Are you sure you want to change the base?
Conversation
61e89e4
to
b2c5f31
Compare
1c46d0e
to
013b3ee
Compare
013b3ee
to
9589eee
Compare
@yiannistri I think the extraMapping are ignored just because they are overwritten by the docker socket code. Would it be possible to fix this (it's a bug in our test suite anyway) and apply your changes to the current One other question I have is what provides |
@anmazzotti yes for sure, we can fix this upstream. I have added a new provider in the meantime only to get things working for both CAPD tests as well as the CAPV tests that will need the extra port mappings. I will create an issue upstream and prepare a PR to address this. Regarding your second question, I have added a new manifest as part of this PR that creates an nginx load balancer. |
One downside of this approach (using extra port mappings) is that if the previous kind cluster is still running on the machine, the creation of a new kind cluster will fail because the 443 port will still be allocated to the previous cluster. We can ensure that no kind clusters are running when we begin a new test run, to mitigate this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks for the PR! One question/suggestion inline:
ShortTestLabel = "short" | ||
FullTestLabel = "full" | ||
DontRunLabel = "dontrun" | ||
LocalTestLabel = "local" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall this was introduced by me in https://github.com/rancher/turtles/pull/187/files#diff-f41624bd6051a35a1694431afebcf5012b89a1cb9ac856464228f797065d34b1R125 while adding CAPV e2e tests but looking at the current code, that label and some more logic looks redundant to me. Do we want to cleanup those as part of the same PR?
Co-authored-by: Furkat Gofurov <[email protected]>
b64e217
to
32af902
Compare
What this PR does / why we need it:
This PR adds a new option (
internal-kind
) for creating a local bootstrap cluster (usingkind
with extra port mappings) that accepts requests from the host. This can be useful in environments where the CAPI provider is running in the local network (eg. vSphere/CAPV). The new setup basically replicates this guide.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 #1162
Special notes for your reviewer:
I have added a new bootstrap cluster provider instead of updating existing code we have in place for creating a
kind
cluster since the port mappings settings were ignored when the docker socket extra mount was specified (useful for CAPD tests).To test the setup of the bootstrap cluster run the following:
Checklist: