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

External Deletion for clusters #575

Merged
merged 1 commit into from
Oct 2, 2023
Merged

External Deletion for clusters #575

merged 1 commit into from
Oct 2, 2023

Conversation

worryg0d
Copy link
Collaborator

@worryg0d worryg0d commented Sep 22, 2023 •
edited
Loading

Implemented handling of the external deletion of resources. In the previous implementation if there was no resource on the Instaclustr we deleted it in k8s. Now it changes a resource state to DELETED without deleting it in k8s.

@worryg0d worryg0d marked this pull request as draft September 22, 2023 10:38
@worryg0d worryg0d self-assigned this Sep 22, 2023
@worryg0d worryg0d added the enhancement New feature or request label Sep 22, 2023
@worryg0d worryg0d changed the title External deletion External Deletion for clusters Sep 25, 2023
@worryg0d worryg0d marked this pull request as ready for review September 25, 2023 07:57
@worryg0d worryg0d force-pushed the issue-572 branch 3 times, most recently from 15beb9c to 254383b Compare September 25, 2023 08:14
OleksiienkoMykyta
OleksiienkoMykyta requested changes Sep 25, 2023
View reviewed changes
controllers/clusters/helpers.go Outdated
"encoding/json"
"fmt"
"k8s.io/client-go/tools/record"
"sigs.k8s.io/controller-runtime/pkg/log"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, sort imports, alse delete isClusterActive function on row 141

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@worryg0d worryg0d force-pushed the issue-572 branch 2 times, most recently from 7d2b60e to 5597b84 Compare September 25, 2023 09:38
@worryg0d worryg0d force-pushed the issue-572 branch 2 times, most recently from 0acccfb to 33bfd70 Compare September 25, 2023 10:45
@worryg0d worryg0d force-pushed the issue-572 branch 2 times, most recently from ee6d5c8 to ac411f4 Compare September 27, 2023 09:08
@worryg0d worryg0d force-pushed the issue-572 branch 2 times, most recently from 3ff0d92 to 664dde9 Compare September 28, 2023 14:06
testisnullus
testisnullus requested changes Sep 28, 2023
View reviewed changes
controllers/clusters/kafkaconnect_controller_test.go Outdated
Comment on lines 20 to 22
"context"
"github.com/instaclustr/operator/pkg/instaclustr"
"github.com/instaclustr/operator/pkg/models"
"os"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sort imports please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

ribaraka
ribaraka requested changes Sep 28, 2023
View reviewed changes
controllers/clusters/cassandra_controller_test.go Outdated

<-doneCh

By("testing by deleting the cluster by Instaclustr API client")
Copy link
Contributor

Choose a reason for hiding this comment

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

By("using all possible ways other than k8s to delete a resource, the k8s operator scheduler should notice the changes and set the status of the deleted resource accordingly.")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

controllers/clusters/cassandra_controller_test.go Outdated
return false
}

fmt.Println("cassandra2.Status.State", cassandra2.Status.State)
Copy link
Contributor

Choose a reason for hiding this comment

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

pls remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

controllers/clusters/helpers.go Outdated
Comment on lines 24 to 30
"github.com/hashicorp/go-version"
"k8s.io/utils/strings/slices"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/instaclustr/operator/apis/clusters/v1beta1"
"github.com/instaclustr/operator/pkg/models"
"k8s.io/utils/strings/slices"
"sigs.k8s.io/controller-runtime/pkg/client"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

imports

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

controllers/clusters/kafka_controller_test.go Outdated
@@ -58,7 +59,7 @@ var _ = Describe("Kafka Controller", func() {
return false
}

return kafka.Status.ID == openapi.CreatedID
return kafka.Status.ID != ""
Copy link
Contributor

Choose a reason for hiding this comment

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

why have you changed this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I used uuid to generate IDs for entities. Reverted

controllers/clusters/kafkaconnect_controller.go Outdated
Comment on lines 20 to 23
"context"
"errors"

"github.com/go-logr/logr"
Copy link
Contributor

Choose a reason for hiding this comment

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

imports

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's wrong here?

controllers/clusters/kafkaconnect_controller_test.go Outdated
Comment on lines 19 to 24
import (
"context"
"github.com/instaclustr/operator/pkg/instaclustr"
"github.com/instaclustr/operator/pkg/models"
"os"

. "github.com/onsi/ginkgo/v2"
Copy link
Contributor

Choose a reason for hiding this comment

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

😮‍💨

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

w_w

controllers/clusters/kafkaconnect_controller_test.go
Comment on lines +67 to +74
By("creating secret with the default user credentials")
secret := kafkaConnect.NewDefaultUserSecret("", "")
secretNamespacedName := types.NamespacedName{
Namespace: secret.Namespace,
Name: secret.Name,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is that OK to create a defaultUserSecret with empty creds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I use it only for getting a secret with an already configured namespaced name for the specified cluster and nothing else.

pkg/instaclustr/mock/server/go/api_apache_kafka_connect_provisioning_v2_service.go Outdated
Comment on lines 90 to 92
id := uuid.New().String()
kafkaConnectClusterV2.Id = id
s.clusters[id] = &kafkaConnectClusterV2
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to use the ID setting logic that I implemented in the stub server API service for the Cassandra resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

more control, imo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It makes sense. I just forgot to revert these changes.

@worryg0d worryg0d requested a review from taaraora October 2, 2023 08:17
@taaraora taaraora merged commit 3280a1b into main Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tengu-alt tengu-alt tengu-alt approved these changes

@testisnullus testisnullus testisnullus approved these changes

@ribaraka ribaraka ribaraka approved these changes

@taaraora taaraora taaraora approved these changes

@OleksiienkoMykyta OleksiienkoMykyta Awaiting requested review from OleksiienkoMykyta

Assignees

@worryg0d worryg0d

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@worryg0d @taaraora @ribaraka @testisnullus @tengu-alt @OleksiienkoMykyta