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

Update to member lease annotation for peer URL TLS is now done by making a member API call #716

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

unmarshall
Copy link
Contributor

What this PR does / why we need it:
Update to member lease annotation for peer URL TLS is now done by making a member API call which accurately depicts the status of the enablement of change of peer URL TLS as seen by the etcd process. Previously only mounted configmap was looked at which is not sufficient as an additional restart of the etcd process is required for this to be reflected in the etcd member.

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

Special notes for your reviewer:

Release note:

Update to member lease annotation for peer URL TLS is now done by making a member API call

@unmarshall unmarshall requested a review from a team as a code owner February 29, 2024 07:52
@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Feb 29, 2024
@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 Feb 29, 2024
@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Mar 11, 2024
@gardener-robot
Copy link

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

@gardener-robot gardener-robot added size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else and removed size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Mar 11, 2024
@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 Mar 11, 2024
@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 Mar 11, 2024
@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 Mar 11, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 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 Mar 11, 2024
@shreyas-s-rao shreyas-s-rao added this to the v0.29.0 milestone Mar 15, 2024
@shreyas-s-rao shreyas-s-rao added the needs/documentation Needs (more) documentation label Mar 21, 2024
@ishan16696
Copy link
Member

/assign

@renormalize
Copy link
Member

In an out of bands discussion it was mentioned that this PR would not be necessary for gardener/etcd-druid#777 to be merged, could this be elaborated further here for documenting why so? Please correct me if my understanding is wrong.

@unmarshall
Copy link
Contributor Author

unmarshall commented Apr 4, 2024

In an out of bands discussion it was mentioned that this PR would not be necessary for gardener/etcd-druid#777 to be merged, could this be elaborated further here for documenting why so? Please correct me if my understanding is wrong.

As per Update advertise peer URLs it was understood that one needs to call member API to update the peer URL and then restart the member for it come into effect. In the current code in master STS component first updates the STS mounting the peer secret volume (this causes a rolling update thus resulting in backup-restore taking the latest config map which contains the changed peer url). Once the backup-restore container restarts as part of initialization it makes the update member API call. Then druid deletes the pods to force a second restart. When working on gardener/etcd-druid#777 it was observed that in the case where we move from replicas 1->3 (with scheme of peer URL changes from http to https) then the second restart was not really required. Druid continues to patch the STS mounting the secret vol (preserving the original replica count of 1). This causes the single member to restart and while it restarts the embedded etcd already picks up the latest peer URL (with TLS enabled) and also correctly gets the tls resources that it then uses to start etcd. This effectively makes the explicit call to update the member URL (currently done in backup-restore) and force a second restart (done in druid) quite useless.

In another case where there is a 3 member cluster and now replicas are increased to 5 and TLS is enabled then one restart would no longer be sufficient. However in this case this PR will still not be useful.

@ishan16696 ishan16696 removed this from the v0.29.0 milestone Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/documentation Needs (more) documentation 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/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants