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.

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

Remove owner checks #555

Merged
merged 2 commits into from
Dec 6, 2022
Merged

Remove owner checks #555

merged 2 commits into from
Dec 6, 2022

Conversation

plkokanov
Copy link
Contributor

@plkokanov plkokanov commented Nov 10, 2022 •
edited
Loading

What this PR does / why we need it:
This PR removes the owner checks from the backup-restore that were used to restart the etcd process that runs in the source Seed cluster during "bad case" control plane migration. As part of the simplification efforts from gardener/gardener#6302 it was decided that "bad case" control plane migration will most likely never be used when the HA gardener topic is ready.

Which issue(s) this PR fixes:
Part of gardener/gardener#6302

Special notes for your reviewer:
This PR depends on gardener/etcd-druid#461 as we do not want to accidentally set owner check flags without the backup-restore actually supporting them.

Release note:

Removed owner checks that were used to restart the `etcd` process that runs in the source `Seed` cluster during "bad case" control plane migration.

@gardener-robot gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Nov 10, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 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 Nov 10, 2022
@plkokanov plkokanov mentioned this pull request Nov 10, 2022
Closed
31 tasks
@plkokanov plkokanov marked this pull request as ready for review November 11, 2022 13:03
@plkokanov plkokanov requested a review from a team as a code owner November 11, 2022 13:03
ishan16696
ishan16696 requested changes Dec 2, 2022
View reviewed changes
Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

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

Overall looks good, just 1 nit

pkg/server/init.go
DefragmentationSchedule: defaultDefragmentationSchedule,
EtcdProcessName: defaultEtcdProcessName,
Copy link
Member

Choose a reason for hiding this comment

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

Please also remove defaultEtcdProcessName whereever it is defined.

Copy link
Member

Choose a reason for hiding this comment

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

It is define here:

etcd-backup-restore/pkg/server/types.go

Line 25 in d57caed

defaultEtcdProcessName = "etcd"

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Dec 2, 2022
@ishan16696 ishan16696 added the merge/squash Should be merged via 'Squash and merge' label Dec 2, 2022
ishan16696
ishan16696 approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

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

As we are about to start working on refactoring of snapshotter logic, our requirement is to decrease the code base, so I'm approving this PR. Whatever the review comments are left, I will address them by myself in separate PR.
LGTM!!

@ishan16696 ishan16696 merged commit d1e8095 into gardener:master Dec 6, 2022
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Dec 6, 2022
@ishan16696 ishan16696 mentioned this pull request Dec 23, 2022
Merged
@aaronfern aaronfern mentioned this pull request Jan 24, 2023
Merged
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

Assignees
No one assigned
Labels
merge/squash Should be merged via 'Squash and merge' needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@plkokanov @ishan16696 @gardener-robot-ci-3 @gardener-robot