Skip to content

Configurable ports #1086

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 19, 2025
Merged

Configurable ports #1086

merged 5 commits into from
May 19, 2025

Conversation

rfranzke
Copy link
Member

@rfranzke rfranzke commented May 6, 2025 •
edited
Loading

How to categorize this PR?

/area usability
/kind enhancement

What this PR does / why we need it:
This PR

  • fixes an issue which was not properly passing the etcd client port to etcd-wrapper (it was not configurable there before Configurable ports etcd-wrapper#52)
  • adds a new WrapperPort field to the Etcd API which allows to configure the server port of etcd-wrapper
  • fixes an issue which was not properly passing the backup port to etcd-wrapper
  • fixes an issue which was not properly configuring the readiness probes with the correct ports
  • fixes an issue which was not properly passing the backup port to etcd-backup-restore

Which issue(s) this PR fixes:
Part of #1071

Special notes for your reviewer:
Requires gardener/etcd-wrapper#52
/cc @shreyas-s-rao @ishan16696

Release note:

The new `.spec.etcd.wrapperPort` field allows to change the server port of `etcd-wrapper`.
An issue has been fixed which caused `etcd` pods not to start when port different from the default values were used.

@rfranzke rfranzke requested a review from a team as a code owner May 6, 2025 10:44
@gardener-robot gardener-robot added the needs/review Needs review label May 6, 2025
@rfranzke rfranzke mentioned this pull request May 6, 2025
Merged
@gardener-robot gardener-robot added area/usability Usability related kind/enhancement Enhancement, improvement, extension size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels May 6, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 6, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 6, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 6, 2025
@anveshreddy18
Copy link
Contributor

/assign

@ishan16696
Copy link
Member

/test pull-etcd-druid-e2e-kind

@ishan16696
Copy link
Member

/hold until the release of etcd-wrapper v0.5.0 which will contain this PR: gardener/etcd-wrapper#52

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label May 6, 2025
anveshreddy18
anveshreddy18 reviewed May 7, 2025
View reviewed changes
Copy link
Contributor

@anveshreddy18 anveshreddy18 left a comment

Choose a reason for hiding this comment

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

Thanks @rfranzke, overall looks good, just have one comment for now. PTAL.

@rfranzke rfranzke requested a review from anveshreddy18 May 7, 2025 13:19
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 7, 2025
@anveshreddy18
Copy link
Contributor

Thanks for addressing @rfranzke, I tried running this version of druid locally with the etcd-wrapper build image europe-docker.pkg.dev/gardener-project/snapshots/gardener/etcd-wrapper:v0.5.0-dev-5369950ff52134dab80725b96845e785927ffa55 by your PR and by changing the .spec.etcd.wrapperPort field to a non-default value and when run, I saw some connection refusal by the wrapper for the readiness probe that hits the .../readyz endpoint, I just observed it very briefly, couldn't dig through, can you PTAL and confirm whether you're seeing that too?

And also the pipeline tests won't pass without the wrapper PR built image, so can you please change the image locally at images.yaml and try running e2e tests with make ci-e2e-kind and see if everything looks good. I can then proceed to review it further, it mostly looks good changes wise. Thanks

@rfranzke rfranzke mentioned this pull request May 9, 2025
Open
@anveshreddy18 anveshreddy18 added this to the v0.30.0 milestone May 13, 2025
@ishan16696
Copy link
Member

@rfranzke can you rebase this PR on master

@anveshreddy18 anveshreddy18 modified the milestones: v0.30.0, v0.31.0 May 15, 2025
@rfranzke
Copy link
Member Author

/unhold

@gardener-robot gardener-robot added the needs/rebase Needs git rebase label May 15, 2025
@gardener-robot
Copy link

@rfranzke You need rebase this pull request with latest master branch. Please check.

@gardener-robot gardener-robot added size/s Size of pull request is small (see gardener-robot robot/bots/size.py) and removed reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies labels May 15, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 15, 2025
@gardener-robot gardener-robot removed the size/l Size of pull request is large (see gardener-robot robot/bots/size.py) label May 15, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 15, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 15, 2025
anveshreddy18
anveshreddy18 approved these changes May 16, 2025
View reviewed changes
Copy link
Contributor

@anveshreddy18 anveshreddy18 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 introducing the change & fixing the issues @rfranzke.
LGTM!!

@anveshreddy18
Copy link
Contributor

anveshreddy18 commented May 16, 2025 •
edited
Loading

@ishan16696 do you want to take a final look before we merge this, as you've shown interest previously.
/hold

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label May 16, 2025
@ishan16696 ishan16696 removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label May 19, 2025
@ishan16696 ishan16696 merged commit b2c9402 into gardener:master May 19, 2025
13 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label May 19, 2025
@rfranzke rfranzke deleted the ports branch May 19, 2025 07:02
@shreyas-s-rao shreyas-s-rao mentioned this pull request May 29, 2025
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishan16696 ishan16696 ishan16696 approved these changes

@anveshreddy18 anveshreddy18 anveshreddy18 approved these changes

Assignees

@anveshreddy18 anveshreddy18

Labels
area/usability Usability related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review needs/second-opinion Needs second review by someone else size/s Size of pull request is small (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
v0.31.0
Development

Successfully merging this pull request may close these issues.
7 participants
@rfranzke @anveshreddy18 @ishan16696 @gardener-robot @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3