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

Allow resource status restore in a more granular way #8204

Open
shubham-pampattiwar opened this issue Sep 10, 2024 · 10 comments · May be fixed by #8464
Open

Allow resource status restore in a more granular way #8204

shubham-pampattiwar opened this issue Sep 10, 2024 · 10 comments · May be fixed by #8464

Comments

@shubham-pampattiwar
Copy link
Collaborator

Describe the problem/challenge you have

Users don't have the knowledge required to understand the proper use and implications restoring the status of a resource, but the controller managing a resource does.

It would be very useful for the controller of a resource to be able to set some flag (annotation, label, etc) to indicate that the status for that resource should be restored.

Describe the solution you'd like

Currently the only way to handle this is to either instruct users to restore particular resource types in the Restore CR. The only alternative from the controller standpoint is to duplicate status information in an annotation and apply it when an object is restored. This is cumbersome for controllers as they now need to keep duplicate information up to date in the actual status and in the annotation.

Anything else you would like to add:

This feature will provide a more granular way to enable resource status restore. Currently, we can only specify the resource status to be restored at a resource type level, this feature will enable at a particular resource types's instance level.

Environment:

  • Velero version (use velero version):
  • Kubernetes version (use kubectl version):
  • Kubernetes installer & version:
  • Cloud provider or hardware configuration:
  • OS (e.g. from /etc/os-release):

Vote on this issue!

This is an invitation to the Velero community to vote on issues, you can see the project's top voted issues listed here.
Use the "reaction smiley face" up to the right of this comment to vote.

  • 👍 for "The project would be better with this feature added"
  • 👎 for "This feature will not enhance the project in a meaningful way"
@weshayutin weshayutin added this to OADP Sep 10, 2024
@shubham-pampattiwar shubham-pampattiwar self-assigned this Sep 10, 2024
@carbonin
Copy link

Currently, we can only specify the resource status to be restored at a resource type level, this feature will enable at a particular resource types's instance level.

I think it's important to note that the place this indication is made is as important as the granularity. For example (not that this would be a good design) it wouldn't be very helpful to have to provide a label selector on the Restore CR as a solution. Rather the indication that an object's status should be restored should be limited to changes on the resource itself.

@sseago
Copy link
Collaborator

sseago commented Sep 10, 2024

@carbonin As far as I understand this, we have no plans to require a label selector on the restore. Rather the idea would be that any resouce with the defined annotation on it would have status restored without the user creating the restore needing to do anything explicit.

@guettli
Copy link

guettli commented Dec 10, 2024

Why not set the label on the CustomResourceDefinition. At least in my case I want all resource objects of my CRD to be restored with Status.

Of course having labels on the resource objects, would also be fine.

@birsanv
Copy link

birsanv commented Dec 10, 2024 via email

@carbonin
Copy link

I would prefer something on the CR as opposed to (or in addition to) the CRD because it's more flexible and also because controllers dealing with resources may not have permissions to edit CRDs at runtime.

CRD owners may not know anything about whether a resource should be restored with status or even if velero is in use.

@guettli
Copy link

guettli commented Dec 11, 2024

BTW: I see a race condition:

  1. Velero restores the resource (without status)
  2. The controller of the resource gets triggered, the Reconcile() function starts and no status is available.
  3. Velero wants to restore the status sub-resource, but the controller already created it.

How can the author of the controller differentiate between a "restore in progress" and a new applied spec?

@guettli
Copy link

guettli commented Dec 11, 2024

BTW: KEP 2527

Status is, effectively, as durable as spec.

I think Velero should change the default. The status should be restored, except there is configuration to not do it.

@sseago
Copy link
Collaborator

sseago commented Dec 13, 2024

@guettli Even if Velero changed the default, that wouldn't resolve the race condition. As I understand it, we would still have to create the object first (since status is stripped on Create calls), then patch it like we do now. So the only change if we swapped the default would be to replace a whitelist with a blacklist -- functionally, everything else would be equivalent.

This would also be a breaking change for Velero, since suddenly restoring status in a new release would change expected restore behavior, likely confusing users.

If we wanted to provide a blanket "restore status by default" feature, it would need to be opt-in -- i.e. a new restore spec field which, if true, would restore status by default, along with new a new "exclude status" field which listed the resource types to not restore status for. We can't just change the default and eliminate the current "include status" list.

@guettli
Copy link

guettli commented Dec 13, 2024

@sseago

If we wanted to provide a blanket "restore status by default" feature, it would need to be opt-in

Yes, this sound like a good idea!

@carbonin
Copy link

I see a race condition

Velero already support restoring status via the Restore CR spec, right? Is this race any different now that we're changing the granularity of what status we're trying to restore?

i.e. a new restore spec field which, if true, would restore status by default, along with new a new "exclude status" field which listed the resource types to not restore status for.

This sounds like a fine enhancement, but it's not what I was asking for in this one which was specifically to allow whether status is restored or not to be controlled by something on the resource itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants