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

Fetch control plane IP at runtime #376

Merged
merged 8 commits into from
Feb 10, 2022
Merged

Conversation

anusha94
Copy link
Contributor

@anusha94 anusha94 commented Feb 8, 2022

What this PR does / why we need it:
Set the CONTROL_PLANE_ENDPOINT_IP programmatically at runtime during E2E tests based on docker network settings.

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 #206

@anusha94 anusha94 marked this pull request as draft February 8, 2022 09:38
@anusha94 anusha94 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress and removed area/test-release labels Feb 8, 2022
@NilanjanDaw NilanjanDaw changed the title Temp fix for CP Endpoint Fetch control plane IP at runtime Feb 8, 2022
@NilanjanDaw NilanjanDaw removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress label Feb 8, 2022
@NilanjanDaw NilanjanDaw marked this pull request as ready for review February 8, 2022 16:46
Copy link
Contributor Author

@anusha94 anusha94 left a comment

Choose a reason for hiding this comment

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

minor nits. Otherwise lgtm

test/e2e/byohost_reuse_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_docker_helper.go Outdated Show resolved Hide resolved
test/e2e/e2e_docker_helper.go Show resolved Hide resolved
@pshail
Copy link
Contributor

pshail commented Feb 9, 2022

small nit, rest is LGTM

@NilanjanDaw
Copy link
Contributor

small nit, rest is LGTM

Hey, @pshail please Iet me know the edits required.

@pshail
Copy link
Contributor

pshail commented Feb 9, 2022

@anusha94 just suggested the other way round 🙃 ... sorry for this @NilanjanDaw i just saw the resolved comments
lets go ahead with this - its anyway a nit 😉

test/e2e/e2e_docker_helper.go Show resolved Hide resolved
Copy link
Contributor

@jamiemonserrate jamiemonserrate left a comment

Choose a reason for hiding this comment

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

LGTM

@NilanjanDaw NilanjanDaw merged commit 0fdb123 into vmware-tanzu:main Feb 10, 2022
@huchen2021
Copy link
Contributor

huchen2021 commented Feb 10, 2022

I don't have strong opinion about this. But I feel a little concern about this hard code way, it looks like a potential bug may occurs one day in the future. It will be better if we can do this more smart. What I can think of is as followed, just for refer:

  1. Fetch subNet by some method, for example, "docker network inspect kind | jq -r 'map(.IPAM.Config[].Subnet) []'"
  2. Calculate the minIP and MaxIP.
  3. Fetch all occupied ip list by some method, for example, "docker network inspect kind | jq -r 'map(.Containers[].IPv4Address) []".
  4. Loop all IPs between minIP and MaxIP one by one, check if it is occpuied, and return it as long as it is an available one.

Anyway, if you are all fine by current fix, I am also ok with it.

@anusha94
Copy link
Contributor Author

I feel a little concern about this hard code way, it looks like a potential bug may occurs one day in the future.

I think that’s a very valid concern. We did think about it - but we know we won’t have that many containers now, so this felt a little complex to achieve something so small. Before this PR - we were hardcoding the full IP, see this removed line from Makefile, so compared to that, this PR added some smartness in fetching the subnet and hardcoding only the last octet.
We can revisit this in future if the 151 hardcoding is turning out to be problematic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fetch the control plane endpoint ip programmatically for e2e tests
6 participants