Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Delete field for instance and binding #2037

Closed
wants to merge 10 commits into from
Closed

Delete field for instance and binding #2037

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bf96b33
Add Deletion fields to Instance and Binding
MHBauer May 15, 2018
63f3c31
generated Deletion code
MHBauer May 15, 2018
0859979
update svcat testdata
MHBauer May 16, 2018
d3fae37
missed annotation
MHBauer May 16, 2018
a5f126f
update generated
MHBauer May 16, 2018
93dad71
update comments & make deletion field optional
MHBauer May 16, 2018
fd8ca77
switch to a timestamp
MHBauer May 16, 2018
4821928
generated files
MHBauer May 16, 2018
fe2f92a
start of proposal
MHBauer May 21, 2018
283522c
typo and termination condition
MHBauer May 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/svcat/testdata/output/get-binding.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
}
},
"secretName": "ups-binding",
"externalID": "061e1d78-d27e-4958-97b8-e9f5aa2f99d7"
"externalID": "061e1d78-d27e-4958-97b8-e9f5aa2f99d7",
"deletionRequested": null
},
"status": {
"conditions": [
Expand Down
1 change: 1 addition & 0 deletions cmd/svcat/testdata/output/get-binding.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ metadata:
selfLink: /apis/servicecatalog.k8s.io/v1beta1/namespaces/test-ns/servicebindings/ups-binding
uid: 7f2aefa0-f712-11e7-aa44-0242ac110005
spec:
deletionRequested: null
externalID: 061e1d78-d27e-4958-97b8-e9f5aa2f99d7
instanceRef:
name: ups-instance
Expand Down
3 changes: 2 additions & 1 deletion cmd/svcat/testdata/output/get-bindings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
},
"parameters": {},
"secretName": "ups-binding",
"externalID": "061e1d78-d27e-4958-97b8-e9f5aa2f99d7"
"externalID": "061e1d78-d27e-4958-97b8-e9f5aa2f99d7",
"deletionRequested": null
},
"status": {
"conditions": [
Expand Down
1 change: 1 addition & 0 deletions cmd/svcat/testdata/output/get-bindings.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ items:
selfLink: /apis/servicecatalog.k8s.io/v1beta1/namespaces/test-ns/servicebindings/ups-binding
uid: 7f2aefa0-f712-11e7-aa44-0242ac110005
spec:
deletionRequested: null
externalID: 061e1d78-d27e-4958-97b8-e9f5aa2f99d7
instanceRef:
name: ups-instance
Expand Down
3 changes: 2 additions & 1 deletion cmd/svcat/testdata/output/get-instance.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@
}
},
"externalID": "7e2c42f3-6d94-4409-bb15-7610d60af544",
"updateRequests": 0
"updateRequests": 0,
"deletionRequested": null
},
"status": {
"conditions": [
Expand Down
1 change: 1 addition & 0 deletions cmd/svcat/testdata/output/get-instance.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ spec:
clusterServicePlanExternalName: default
clusterServicePlanRef:
name: 86064792-7ea2-467b-af93-ac9694d96d52
deletionRequested: null
externalID: 7e2c42f3-6d94-4409-bb15-7610d60af544
parameters:
param1: value1
Expand Down
3 changes: 2 additions & 1 deletion cmd/svcat/testdata/output/get-instances.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@
},
"parameters": {},
"externalID": "7e2c42f3-6d94-4409-bb15-7610d60af544",
"updateRequests": 0
"updateRequests": 0,
"deletionRequested": null
},
"status": {
"conditions": [
Expand Down
1 change: 1 addition & 0 deletions cmd/svcat/testdata/output/get-instances.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ items:
clusterServicePlanExternalName: default
clusterServicePlanRef:
name: 86064792-7ea2-467b-af93-ac9694d96d52
deletionRequested: null
externalID: 7e2c42f3-6d94-4409-bb15-7610d60af544
parameters: {}
updateRequests: 0
Expand Down
12 changes: 12 additions & 0 deletions pkg/apis/servicecatalog/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,12 @@ type ServiceInstanceSpec struct {
// allows for parameters to be updated with any out-of-band changes that have
// been made to the secrets from which the parameters are sourced.
UpdateRequests int64

// DeletionRequested provides a mechanism for requesting a delete without
// performing a direct delete (which cannot be undone in cases where the
// delete is rejected by the broker). This is the recommended way to
// delete, to avoid resources becoming stuck in the deletion phase.
DeletionRequested metav1.Time
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think this should be deprovisionTimestamp

Copy link
Contributor

Choose a reason for hiding this comment

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

with this being a timestamp, do we expect people to actually set the time? That seems odd to me. And what does its value mean? What if its Jan 1, 1970? What is its Dec 31, 2020? Are we really supporting scheduled deletes? Is that even a thing in kube?

Copy link
Contributor

Choose a reason for hiding this comment

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

people should not set the time, svcat would, and if we can hijack the k8s delete command, that would.

time before now means do a delete. So if Jan 1, 1970 and the resource is not deleted, try to delete it. k8s does not support a future date, see deletionTimestamp

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we agree to hijack delete? And we need it to work with vanilla kubectl, so I do not think asking people to set a time is a good UX, when 99+% of the time I bet they mean "do it now".

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 is bad UX to not have a time tagged to when you attempted that action. How do you know if the status changed before of after a delete was requested? How do you know when service catalog should use a bigger hammer to unbind or deprovision the thing?

the validator could hijack the setting of these field to be set to now. so user sets it to anything they want, including true or now, and then the validator replaces that with time.Now().

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not against there being a timestmap someplace. What I'm against is making the user set a timestamp as their UX for delete(). That's a weird design. The normal path for Kube is the user does an HTTP DELETE and as a side effect a timestamp is set. That's very different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are outside the area of calling DELETE directly on any of our objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I am fine with keeping the name as it, with a single field that's just a timestamp and relying on svcat to handle calling it appropriately.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we stay with an api server, the clear choice is to make a sub-resource for each type to do the deprovision or unbind, and those methods modify this field. then the user flow is kubectl unbind ServiceInstance foobar but in the direction service catalog should go using CRDs that is not an option, so I am saying the existence of the field, even an empty string, allows the admission controller to set it to a correct timestamp.

}

// ServiceInstanceStatus represents the current status of an Instance.
Expand Down Expand Up @@ -1025,6 +1031,12 @@ type ServiceBindingSpec struct {
// settable by the end-user. User-provided values for this field are not saved.
// +optional
UserInfo *UserInfo

// DeletionRequested provides a mechanism for requesting a delete without
// performing a direct delete (which cannot be undone in cases where the
// delete is rejected by the broker). This is the recommended way to
// delete, to avoid resources becoming stuck in the deletion phase.
DeletionRequested metav1.Time
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think this should be unbindTimestamp

}

// ServiceBindingStatus represents the current status of a ServiceBinding.
Expand Down
12 changes: 12 additions & 0 deletions pkg/apis/servicecatalog/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,6 +865,12 @@ type ServiceInstanceSpec struct {
// been made to the secrets from which the parameters are sourced.
// +optional
UpdateRequests int64 `json:"updateRequests"`

// DeletionRequested provides a mechanism for requesting a delete without
// performing a direct delete (which cannot be undone in cases where the
// delete is rejected by the broker). This is the recommended way to
// delete, to avoid resources becoming stuck in the deletion phase.
DeletionRequested metav1.Time `json:"deletionRequested,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still the right name now that it's not a bool?

Copy link
Contributor

Choose a reason for hiding this comment

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

nope

}

// ServiceInstanceStatus represents the current status of an Instance.
Expand Down Expand Up @@ -1127,6 +1133,12 @@ type ServiceBindingSpec struct {
// settable by the end-user. User-provided values for this field are not saved.
// +optional
UserInfo *UserInfo `json:"userInfo,omitempty"`

// DeletionRequested provides a mechanism for requesting a delete without
// performing a direct delete (which cannot be undone in cases where the
// delete is rejected by the broker). This is the recommended way to
// delete, to avoid resources becoming stuck in the deletion phase.
DeletionRequested metav1.Time `json:"deletionRequested,omitempty"`
}

// ServiceBindingStatus represents the current status of a ServiceBinding.
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions pkg/apis/servicecatalog/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions pkg/apis/servicecatalog/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 14 additions & 2 deletions pkg/openapi/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.