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

Do not set default namespace #1246

Conversation

muffl0n
Copy link
Contributor

@muffl0n muffl0n commented Feb 14, 2022

Change Overview

Currently, it is not possible to delete objects that do not have a namespace, like VolumeSnapshotContent or PersistentVolume.

{"ActionSet":"staging-server-delete-nwvg6","File":"pkg/controller/controller.go","Function":"github.com/kanisterio/kanister/pkg/controller.(*Controller).runAction.func1","Line":438,"Phase":"deleteSnapshotContent","cluster_name":"684f75d4-1734-4ec3-84b2-a56c001e71cf","error":"the server could not find the requested resource","hostname":"kanister-kanister-operator-646cd44d8f-zdc77","level":"info","msg":"Failed to execute phase: v1alpha1.Phase{Name:\"deleteSnapshotContent\", State:\"pending\", Output:map[string]interface {}(nil)}:","time":"2022-02-14T14:28:36.055688239Z"}

My blueprint:

apiVersion: cr.kanister.io/v1alpha1
kind: Blueprint
metadata:
  name: staging-server
  namespace: kanister
actions:
  delete:
    phases:
      - func: KubeOps
        name: deleteSnapshotContent
        args:
          operation: delete
          objectReference:
            apiVersion: v1
            group: snapshot.storage.k8s.io
            resource: volumesnapshotcontents
            name: snapshot-content-test

This PR removes the setting of a default namespace, which allows these kind of operations.

Unfortunately, removing this default behaviour feels pretty dangerous, cause other users might already rely on that.

Thread in Slack: https://kanisterio.slack.com/archives/C85C8V22J/p1643919326058139

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • n/a

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

@@ -67,7 +66,7 @@ func (crs *kubeops) Exec(ctx context.Context, tp param.TemplateParams, args map[
if err := Arg(args, KubeOpsOperationArg, &op); err != nil {
return nil, err
}
if err := OptArg(args, KubeOpsNamespaceArg, &namespace, metav1.NamespaceDefault); err != nil {
if err := OptArg(args, KubeOpsNamespaceArg, &namespace, nil); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

@PrasadG193 What do you think? Is there a better way to handle non-namespaced resources?

Copy link
Contributor

Choose a reason for hiding this comment

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

passing default namespace is a bit confusing for cluster scoped resources. I think empty string would also work. I will let Prasad approve.

Copy link
Contributor

Choose a reason for hiding this comment

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

The safest way I can think of is having an additional arg through which we can decide if the resource is cluster scoped.

Another approach I can think of is running discovery and checking if the passed resource is cluster scoped and ignoring namespace in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In k8s cluster scoped resources are created simply without specifying a namespace. There is no explicit argument in the yaml. Handling this different in Kanister feels a bit confusing.

Latter approach should throw an error when specifying an namespace on a cluster scoped resource. Otherwise it would be quite confusing cause of the magic happening in the background.

@ihcsim
Copy link
Contributor

ihcsim commented Mar 7, 2022

@muffl0n Sorry for the delay; we didn't forget about this PR 🙂. The team has been discussing about the scope and purpose of the KubeOps function. At this point, we decided not to extend it to delete cluster-scoped resources, to avoid KubeOps from being misused to delete other cluster-scoped resources not directly related to data protection.

Can you tell us more about what you are trying to accomplish? If it is just to delete volumesnapshotcontents, we are thinking of adding a new function, similar to the existing DeleteVolumeSnapshot function. Will that work for you?

@muffl0n
Copy link
Contributor Author

muffl0n commented Mar 8, 2022

No worries and thank you for your feedback!

In my blueprints, I mainly use VolumeSnapshots and heavily rely on the resources used in GCP like creating mysql backups with "gcloud sql backup".

As I also want to use the backup from one environment to do a restore in another (e.g. when doing a sync from prod to qa), I have to transfer the data to the other cluster/project.
To do this, I copy the actionset generated by the backup on the source cluster to the destination cluster. The actionset includes the handle of the snapshot, like this:

apiVersion: cr.kanister.io/v1alpha1
kind: ActionSet
...
status:
  actions:
  - artifacts:
      ...
      snapshot:
        keyValue:
          handle: projects/foo/global/snapshots/snapshot-c44e1dbc-fdee-498b-abb7-77b05ca00ad4
          name: sophora-server-storage-cluster-03-backup-cu6fuwee
          namespace: sophora

Now my restore just has to create the VolumeSnapshot (and VolumeSnapshotContent from above "handle") so that my StatefulSets can pick those as a datasource for their PVCs.

Long story short: As I have to create the VolumeSnapshotContent (and VolumeSnapshot) in the restore, I need to create (and delete!) resources that are non-namespaced.
Example would look like this:

apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshotContent
metadata:
  name: snapshot-content-test
spec:
  deletionPolicy: Retain
  driver: pd.csi.storage.gke.io
  source:
    snapshotHandle: {{ .ArtifactsIn.snapshot.KeyValue.handle }}
  volumeSnapshotRef:
   name: snapshot-test
   namespace: "{{ .StatefulSet.Namespace }}"
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshot
metadata:
  name: snapshot-test
  namespace: "{{ .StatefulSet.Namespace }}"
spec:
  source:
    volumeSnapshotContentName: snapshot-content-test

I think there would need to be functions like "CreateVolumeSnapshotFromContent" and "DeleteVolumeSnapshotContent". I'm still not sure if this is enough and maybe even a bit too special.

This allows kubeops to delete objects that do not have a namespace, like VolumeSnapshotContent or PersistentVolume
@muffl0n muffl0n force-pushed the allow-kubeops-to-delete-objects-without-namespace branch from 972486d to d5a3aa6 Compare March 17, 2022 09:22
@shuguet shuguet added this to Review in progress in Kanister Mar 23, 2022
@ihcsim
Copy link
Contributor

ihcsim commented Mar 24, 2022

Superseded by #1282.

@ihcsim ihcsim closed this Mar 24, 2022
Kanister automation moved this from Review in progress to Done Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants