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

[main] Add new method to track originating cluster of Backup #613

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

mallardduck
Copy link
Member

@mallardduck mallardduck commented Oct 31, 2024

This partial solves for #612

However it's important to note that while I wrote an RFE for this too - the root here is seeking to fix a bug. The bug is that "BRO itself doesn't do anything to give indications when In-Place Restore is acceptable to use".

The short version of the fix, is we add a few new status details and expose them via kubectl. And later make a change to rancher/dashboard to ensure UI can also benefit from this new context by adding a column.


To be discussed:

  • This change is very Backwards Compatible, can we ship a CRD change for the new status field in a 2.10.x release?
    • Feels like it's a grey area - but, probably OK.
    • k8s seems to be OK with full on schema additions (of spec too) as long as they're optional/nullalble. This is hopefully less impactful since users cannot control status.
    • The current result on a cluster w/o CRD change is:
      • is a new warning about originCluster not existing on status,
      • Status conditions are correctly set to represent unknown in-place viability.
  • Do we want to explore supporting an even more Backwards friendly method of detecting origin ID's with Filename?
    • For the best UX, we will account for existing backups with UID in filename. In other words, once the CRD is updated with new status field we'll add the UID we find from the filename (or do nothing if not matching expected filename format).

@mallardduck mallardduck requested a review from a team as a code owner October 31, 2024 23:27
@mallardduck mallardduck force-pushed the bro-metadata-context branch 2 times, most recently from 4967e19 to f0b6b30 Compare November 1, 2024 01:26
@mallardduck
Copy link
Member Author

mallardduck commented Nov 1, 2024

@jbiers - So i'm not moving it to in review yet, since this was a very rough first pass try.

However if you can take some time to get familiar with the changes and review the list of concerns I thought of (as well as consider/add your own) - that'd be great help to keep the ball rolling on this. In other words, while I could work on this more I'd love your feedback first before I spend more time on it.

@mallardduck
Copy link
Member Author

@alexandreLamarre - I've added you as a reviewer her as I'd like to get your take on this. When you have time LMK if you'd like to sync up and chat about in a huddle. Or feel free to review in your own time and give me feedback here.

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.

1 participant