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

HOSTEDCP-1416: Hosted Control Planes ETCD Backup API #1553

Closed
wants to merge 1 commit into from

Conversation

jparrill
Copy link

@jparrill jparrill commented Feb 2, 2024

This PR contains the definition of a new API under the hostedcluster.spec.etcd group which it's called backup and it will help to perform automated etcd backups using the Hypershift API.

We are reusing the ETCD backup API, which will help on the homogenisation in the backups for Openshift and Hypershift.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 2, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 2, 2024

@jparrill: This pull request references HOSTEDCP-1416 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

This PR contains the definition of a new API under the hostedcluster.spec.etcd group which it's called backup and it will help to perform automated etcd backups using the Hypershift API.

We are reusing the ETCD backup API, which will help on the homogenisation in the backups for Openshift and Hypershift.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2024
Copy link
Contributor

openshift-ci bot commented Feb 2, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

openshift-ci bot commented Feb 2, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sjenning for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dhellmann
Copy link
Contributor

#1555 is changing the enhancement template in a way that will cause the header check in the linter job to fail for existing PRs. If this PR is merged within the development period for 4.16 you may override the linter if the only failures are caused by issues with the headers (please make sure the markdown formatting is correct). If this PR is not merged before 4.16 development closes, please update the enhancement to conform to the new template.

@jparrill
Copy link
Author

Hey @dhellmann thanks for the heads up, I will take that in mind before the final submission :).

@jparrill jparrill force-pushed the HOSTEDCP-1416 branch 4 times, most recently from b6a1715 to 0eac566 Compare February 16, 2024 10:19
@jparrill jparrill marked this pull request as ready for review February 16, 2024 10:19
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 16, 2024
@jparrill
Copy link
Author

Mentioning the Hypershift team in order to review and add their thoughts (sorry in advance for the spam):

/cc @csrwng @sjenning @muraee @enxebre @derekwaynecarr @imain @bryan-cox @Patryk-Stefanski @celebdor

Copy link

@Patryk-Stefanski Patryk-Stefanski left a comment

Choose a reason for hiding this comment

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

Just a few typos and nitpicks. Otherwise lgtm

@jparrill jparrill force-pushed the HOSTEDCP-1416 branch 2 times, most recently from 2d625b7 to 2f661e2 Compare February 16, 2024 13:54
@jparrill jparrill force-pushed the HOSTEDCP-1416 branch 2 times, most recently from 3024126 to 2c442cc Compare February 20, 2024 11:18
Copy link
Contributor

@csrwng csrwng left a comment

Choose a reason for hiding this comment

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

some comments

@jparrill
Copy link
Author

jparrill commented Feb 22, 2024

I would double-check with the field, but S3 is not very common on-prem. Traditional on-prem is more likely to have NFS.

Yep, me too but I prefer to have an MVP with smaller footprint and then add new storage providers.

Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
Copy link
Contributor

openshift-ci bot commented Feb 22, 2024

@jparrill: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

Overall the direction here seems reasonable, but I would like to see some more detail on the responsibilities of the controllers and how and where we re-use OCP core functionality versus where we're adding on top or re-implementing.

@jparrill
Copy link
Author

jparrill commented Mar 1, 2024

Overall the direction here seems reasonable, but I would like to see some more detail on the responsibilities of the controllers and how and where we re-use OCP core functionality versus where we're adding on top or re-implementing.

Agree, in this case we are adapting the controllers of the ETCD Operator in our code because we don't want to get the whole ETCD Operator as part of the ControlPlane (for scalability reasons) (apart that in their case, this is using a separated CRD). So the responsibility relies on us and we can follow the direction they followed.

@jparrill
Copy link
Author

After a team call discussion we decided to go through a couple of spikes regarding OADP and vol-synk from ACM/MCE. I will update the design afterwards.

@openshift-bot
Copy link

Inactive enhancement proposals go stale after 28d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 9, 2024
@openshift-bot
Copy link

Stale enhancement proposals rot after 7d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Rotten proposals close after an additional 7d of inactivity.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 16, 2024
@jparrill
Copy link
Author

The decision over this enhancement is basically cover the functionality with OADP. We will develop the internals in a document instead of en enhancement, which IMHO makes more sense.

@jparrill jparrill closed this Apr 17, 2024
@dhellmann
Copy link
Contributor

(automated message) This pull request is closed with lifecycle/rotten. The associated Jira ticket, HOSTEDCP-1370, has status "Closed, Obsolete". Should the PR be reopened, updated, and merged? If not, removing the lifecycle/rotten label will tell this bot to ignore it in the future.

1 similar comment
@dhellmann
Copy link
Contributor

(automated message) This pull request is closed with lifecycle/rotten. The associated Jira ticket, HOSTEDCP-1370, has status "Closed, Obsolete". Should the PR be reopened, updated, and merged? If not, removing the lifecycle/rotten label will tell this bot to ignore it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.