feat: support for central trait `preserved` #1564

parametalol · 2024-01-15T11:41:46Z

Make use of #1549: do not deprovision centrals which have "preserved" trait.

E2E tests for the traits.

parametalol · 2024-01-15T11:41:47Z

Current dependencies on/for this PR:

main
- PR feat: support for central trait preserved #1564 👈

parametalol · 2024-02-09T16:07:21Z

I think we need an e2e test here as well that actually executes the query.

I added e2e tests for generic traits and this preserved trait specifically.

rukletsov · 2024-02-13T10:13:17Z

internal/dinosaur/pkg/services/dinosaur.go

@@ -60,6 +62,10 @@ type CNameRecordStatus struct {
 	Status *string
 }

+var errPreservedCentral = func(id string) *errors.ServiceError {
+	return errors.BadRequest("central %q has %s trait", id, constants.CentralTraitPreserved)


Should be 409 I think. The error message (probably in the caller given there are two) should also include the remediation hint, I guess it will be removing the trait or using force.

rukletsov · 2024-02-13T10:17:54Z

internal/dinosaur/pkg/services/dinosaur.go

@@ -492,6 +498,10 @@ func (k *dinosaurService) RegisterDinosaurDeprovisionJob(ctx context.Context, id
 	}
 	metrics.IncreaseCentralTotalOperationsCountMetric(dinosaurConstants.CentralOperationDeprovision)

+	if arrays.Contains(dinosaurRequest.Traits, constants.CentralTraitPreserved) {


Why do we need a check here in addition to the one during deletion?

This is for the case when the function is called by the HTTP handler: the check allows for returning the error immediately, instead of marking the instance for deletion and fail to delete it later.

stehessel · 2024-02-13T22:07:00Z

internal/dinosaur/pkg/services/dinosaur.go

@@ -60,6 +63,11 @@ type CNameRecordStatus struct {
 	Status *string
 }

+var errPreservedCentral = func(id string) *errors.ServiceError {
+	return errors.NewErrorFromHTTPStatusCode(http.StatusConflict,
+		"central %q has %s trait, remove the trait to enable deletion", id, constants.CentralTraitPreserved)


Could you please update the openapi spec to include 409 for delete?

stehessel · 2024-02-13T22:16:42Z

internal/dinosaur/pkg/services/dinosaur_test.go

+	})
+	svcErr := k.Delete(preserved, false)
+	assert.Equal(t, http.StatusConflict, svcErr.HTTPCode)
+	{


This block feels like it should be extracted into a separate function.

stehessel · 2024-02-13T22:22:57Z

e2e/e2e_central_traits_test.go

+		Expect(err).ToNot(HaveOccurred(), "no error on checking for existing trait")
+
+		_, err = adminAPI.GetCentralTrait(ctx, id, "test-trait-2")
+		Expect(err).To(HaveOccurred(), "error on checking for non-existing trait")


We could assert 404 here.

Made it in #1661.

stehessel · 2024-02-13T22:34:39Z

internal/dinosaur/pkg/services/dinosaur.go

@@ -575,6 +589,11 @@ func (k *dinosaurService) DeprovisionExpiredDinosaurs() *errors.ServiceError {
 // If the force flag is true, then any errors prior to the final deletion of the CentralRequest will be logged as warnings
 // but do not interrupt the deletion flow.
 func (k *dinosaurService) Delete(centralRequest *dbapi.CentralRequest, force bool) *errors.ServiceError {


Is the force option exposed by the console UI? It feels like we are mixing up different concepts a bit. What deletion scenarios are we trying to prevent?

User deletes their own instance. Do we want to stop anyone from doing this? They will be confused what's going on because the traits are managed via the admin API. Traits will be generally unknown to users, so why expose them to users?

(Accidental) deletion by admins? Admins control traits, but admin API ignores traits.

Unwanted/early expiry of instances. Wouldn't it make more sense to prevent expiry in the first place, rather than stop the deprovision of an already expired instance?

No, force is not exposed. In fact, force=true if only an admin deletes the central via /centrals/db/{id} endpoint. I was thinking that this would be the only way to delete a preserved instance, including users not being able to do so.

Your 3rd point makes sense. The trait should be called perpetual then. We could actually have both traits, but I think perpetual is more interesting.

No, force is not exposed. In fact, force=true if only an admin deletes the central via /centrals/db/{id} endpoint. I was thinking that this would be the only way to delete a preserved instance, including users not being able to do so.

I think users could interpret this as being forced to pay in case they use on-demand billing. So I don't think this is a good idea. If we want to expose it to users, it would be better to also mark their instance as preserved in the UI and let them be able to change the state / force delete it.

IMHO both admin and user API calls to delete instances should be respected despite preserved trait. If user wants to delete an instance, they should be able to do that without additional steps (they already have to type in instance name in the UI to confirm). If an admin wants to delete a user instance, they most likely have a good reason for it.

ludydoo · 2024-03-25T12:57:34Z

internal/dinosaur/pkg/services/dinosaur.go

@@ -512,6 +524,7 @@ func (k *dinosaurService) DeprovisionDinosaurForUsers(users []string) *errors.Se
 		Model(&dbapi.CentralRequest{}).
 		Where("owner IN (?)", users).
 		Where("status NOT IN (?)", dinosaurDeletionStatuses).
+		Where("traits IS NULL OR ? != ALL (traits)", dinosaurConstants.CentralTraitPreserved).


By curiosity, what does ? != ALL (traits) mean ? And why not use ("? NOT IN traits", CentralTraitPreserved)

This is a specific syntax for column of type array: https://www.postgresql.org/docs/current/arrays.html#ARRAYS-SEARCHING

openshift-merge-robot · 2024-05-28T10:08:49Z

PR needs rebase.

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-sigs/prow repository.

openshift-ci · 2024-05-28T12:11:52Z

@0x656b694d: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e	`4420c77`	link	true	`/test e2e`
ci/prow/images	`4420c77`	link	true	`/test images`

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-sigs/prow repository. I understand the commands that are listed here.

parametalol temporarily deployed to development January 15, 2024 11:41 — with GitHub Actions Inactive

parametalol mentioned this pull request Jan 15, 2024

feat: central traits #1549

Closed

9 tasks

parametalol force-pushed the michael/central-trait-preserved branch from c4633a8 to 419d224 Compare January 15, 2024 13:37

parametalol temporarily deployed to development January 15, 2024 13:37 — with GitHub Actions Inactive

parametalol temporarily deployed to development January 15, 2024 13:38 — with GitHub Actions Inactive

parametalol force-pushed the michael/central-labels branch from fe1a163 to 6b46642 Compare January 15, 2024 13:53

parametalol force-pushed the michael/central-trait-preserved branch from 419d224 to 651078b Compare January 15, 2024 14:16

parametalol temporarily deployed to development January 15, 2024 14:16 — with GitHub Actions Inactive

parametalol force-pushed the michael/central-trait-preserved branch from 651078b to a2fe03c Compare January 16, 2024 13:58

parametalol temporarily deployed to development January 16, 2024 13:58 — with GitHub Actions Inactive

parametalol changed the title ~~michael/central-trait-preserved~~ feat: support for central trait preserved Jan 16, 2024

parametalol force-pushed the michael/central-labels branch from 069c6f1 to 3ee143c Compare January 16, 2024 15:46

kovayur force-pushed the michael/central-trait-preserved branch from a2fe03c to f4b76ca Compare January 17, 2024 10:09

kovayur force-pushed the michael/central-labels branch from 3ee143c to e6da879 Compare January 17, 2024 10:09

openshift-merge-robot added the needs-rebase label Jan 17, 2024

parametalol force-pushed the michael/central-labels branch from e6da879 to 6474c30 Compare January 18, 2024 09:20

parametalol changed the base branch from michael/central-labels to michael/central-traits January 18, 2024 09:24

parametalol force-pushed the michael/central-trait-preserved branch from f4b76ca to 1aa70bc Compare January 18, 2024 09:25

parametalol temporarily deployed to development January 18, 2024 09:25 — with GitHub Actions Inactive

openshift-merge-robot removed the needs-rebase label Jan 18, 2024

parametalol temporarily deployed to development February 9, 2024 08:41 — with GitHub Actions Inactive

tests

78bc675

parametalol temporarily deployed to development February 9, 2024 09:44 — with GitHub Actions Inactive

tests

4c309d9

parametalol temporarily deployed to development February 9, 2024 10:48 — with GitHub Actions Inactive

return error quicker, test fixes

433b9b9

parametalol temporarily deployed to development February 9, 2024 15:11 — with GitHub Actions Inactive

parametalol requested a review from stehessel February 9, 2024 16:06

rukletsov reviewed Feb 13, 2024

View reviewed changes

errors refactoring

e8ddc1c

parametalol temporarily deployed to development February 13, 2024 15:53 — with GitHub Actions Inactive

parametalol requested a review from rukletsov February 13, 2024 15:54

fix e2e

4420c77

parametalol temporarily deployed to development February 13, 2024 17:31 — with GitHub Actions Inactive

stehessel reviewed Feb 13, 2024

View reviewed changes

parametalol mentioned this pull request Feb 15, 2024

ROX-21867: e2e tests #1661

Open

9 tasks

ludydoo reviewed Mar 25, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for central trait `preserved` #1564

feat: support for central trait `preserved` #1564

parametalol commented Jan 15, 2024 •

edited

Loading

parametalol commented Jan 15, 2024 •

edited

Loading

parametalol commented Feb 9, 2024

rukletsov Feb 13, 2024

rukletsov Feb 13, 2024

parametalol Feb 13, 2024

stehessel Feb 13, 2024 •

edited

Loading

stehessel Feb 13, 2024

stehessel Feb 13, 2024 •

edited

Loading

parametalol Feb 16, 2024

stehessel Feb 13, 2024 •

edited

Loading

parametalol Feb 15, 2024

stehessel Feb 15, 2024

ivan-degtiarenko Feb 15, 2024

ludydoo Mar 25, 2024 •

edited

Loading

parametalol Mar 29, 2024

openshift-merge-robot commented May 28, 2024

openshift-ci bot commented May 28, 2024

feat: support for central trait preserved #1564

Are you sure you want to change the base?

feat: support for central trait preserved #1564

Conversation

parametalol commented Jan 15, 2024 • edited Loading

parametalol commented Jan 15, 2024 • edited Loading

parametalol commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stehessel Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stehessel Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stehessel Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludydoo Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented May 28, 2024

openshift-ci bot commented May 28, 2024

feat: support for central trait `preserved` #1564

feat: support for central trait `preserved` #1564

parametalol commented Jan 15, 2024 •

edited

Loading

parametalol commented Jan 15, 2024 •

edited

Loading

stehessel Feb 13, 2024 •

edited

Loading

stehessel Feb 13, 2024 •

edited

Loading

stehessel Feb 13, 2024 •

edited

Loading

ludydoo Mar 25, 2024 •

edited

Loading