[#574] policer: Check the container has been really removed #640

Merged
fyrchik merged 2 commits from aarifullin/frostfs-node:fix/574-cnt_ever_existed into master 2023-08-28 14:21:40 +00:00
Member
  • Introduce SpecInfoProvider interface
  • Introduce common method EverExisted
  • Refactor tree service
  • Use the check in the policer

Signed-off-by: Airat Arifullin a.arifullin@yadro.com

* Introduce SpecInfoProvider interface * Introduce common method EverExisted * Refactor tree service * Use the check in the policer Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
aarifullin added the
bug
frostfs-node
labels 2023-08-23 10:14:38 +00:00
Owner

Please, rebase on master, tests failures should be fixed there.

Please, rebase on master, tests failures should be fixed there.
fyrchik reviewed 2023-08-23 10:31:14 +00:00
@ -46,0 +46,4 @@
// SpecInfoProvider is the interface that provides methods to
// get container specific info or metadata.
type SpecInfoProvider interface {
DeletionInfo(cid.ID) (DelInfo, error)
Owner

To me it is a part of a Source interface, why have you decided to introduce a separate one?

To me it is a part of a `Source` interface, why have you decided to introduce a separate one?
Author
Member

Because this affects many implementations of Source interface (some of them wrapped in wrapped wrappers) and for me

  1. It is absolutely not obvious that such implementations should be extended with DeletionInfo
  2. I need to put stubs for DeletionInfo impl like for caches or morph readers

So, I tried to avoid harmful refactoring

Because this affects many implementations of Source interface (some of them wrapped in wrapped wrappers) and for me 1. It is absolutely not obvious that such implementations should be extended with DeletionInfo 2. I need to put stubs for DeletionInfo impl like for caches or morph readers So, I tried to avoid harmful refactoring
Owner

Can we have 2 interface, one included in another? This should make it easier to accept just what you need.
Another option is to define the interface where we use it (so, tree and policer).

Can we have 2 interface, one included in another? This should make it easier to accept just what you need. Another option is to define the interface where we use it (so, tree and policer).
Author
Member

Alright. I will try then

Alright. I will try then
fyrchik marked this conversation as resolved
@ -15,0 +31,4 @@
return c.DeletionInfo(binCnr)
}
func (c *Client) DeletionInfo(cid []byte) (containercore.DelInfo, error) {
Owner

Previously it returned a pointer, why this change?

Previously it returned a pointer, why this change?
fyrchik marked this conversation as resolved
@ -119,2 +121,4 @@
}
// WithContainerSpecInfo returns option to set container specific info provider for Policer.
func WithContainerSpecInfo(v container.SpecInfoProvider) Option {
Owner

We refactor and introduce new policer functionality in the same commit, can we split it?

We refactor and introduce new policer functionality in the same commit, can we split it?
fyrchik marked this conversation as resolved
aarifullin changed title from [#574] policer: Check the container has been really removed to WIP: [#574] policer: Check the container has been really removed 2023-08-23 10:55:08 +00:00
aarifullin changed title from WIP: [#574] policer: Check the container has been really removed to [#574] policer: Check the container has been really removed 2023-08-23 11:00:20 +00:00
requested reviews from storage-core-committers, storage-core-developers 2023-08-23 16:26:25 +00:00
dstepanov-yadro approved these changes 2023-08-24 08:27:55 +00:00
aarifullin changed title from [#574] policer: Check the container has been really removed to WIP: [#574] policer: Check the container has been really removed 2023-08-24 09:54:33 +00:00
aarifullin force-pushed fix/574-cnt_ever_existed from 76405209f4 to 9dcf70033e 2023-08-24 09:58:41 +00:00 Compare
aarifullin force-pushed fix/574-cnt_ever_existed from 9dcf70033e to fe9b617e8f 2023-08-24 12:36:53 +00:00 Compare
aarifullin changed title from WIP: [#574] policer: Check the container has been really removed to [#574] policer: Check the container has been really removed 2023-08-24 12:41:41 +00:00
aarifullin force-pushed fix/574-cnt_ever_existed from fe9b617e8f to 7aa67b383b 2023-08-24 12:42:58 +00:00 Compare
aarifullin force-pushed fix/574-cnt_ever_existed from 7aa67b383b to 1c0e9a1f09 2023-08-24 12:49:25 +00:00 Compare
fyrchik approved these changes 2023-08-24 16:31:40 +00:00
@ -159,1 +166,3 @@
s.set(cnr, nil, new(apistatus.ContainerNotFound))
s.containerCache.set(cnr, nil, new(apistatus.ContainerNotFound))
// The removal causes the cache miss and thus deletion info (that contains
Owner

The removal invalidates possibly stored error response. sounds better to me.

`The removal invalidates possibly stored error response.` sounds better to me.
fyrchik marked this conversation as resolved
@ -42,6 +42,7 @@ const (
NotificatorNotificatorStartProcessingObjectNotifications = "notificator: start processing object notifications"
NotificatorNotificatorProcessingObjectNotification = "notificator: processing object notification"
PolicerCouldNotGetContainer = "could not get container"
PolicerCouldNotCheckIfContainerEverExisted = "could not check if container ever existed"
Owner

What about could not confirm container removal? "ever existed" sounds vague to me -- it definitely existed if there is an object in our storage.

What about `could not confirm container removal`? "ever existed" sounds vague to me -- it _definitely_ existed if there is an object in our storage.
Author
Member

Nice!

Nice!
fyrchik marked this conversation as resolved
@ -0,0 +9,4 @@
// EverExisted checks whether the container ever existed or
// it just has not been created yet at the current epoch.
func EverExisted(s Source, cid cid.ID) (bool, error) {
Owner

Again, why not "WasRemoved"?

Again, why not "WasRemoved"?
Author
Member

For me EverExisted makes he motivation for rechecking very understandable.

If I name the method WasRemoved then is it really obvious that I need to deduce removed containers and then call WasRemoved when it deduced they are removed? I prefer to keep EverExisted.

My point can seem confusing but, briefly, I want to make the rechecking more obvious and logical

For me `EverExisted` makes he motivation for rechecking very understandable. If I name the method `WasRemoved` then is it really obvious that I need to deduce removed containers and then call `WasRemoved` when it deduced they are removed? I prefer to keep `EverExisted`. My point can seem confusing but, briefly, I want to make the rechecking more obvious and logical
Owner

But this is what you actually check: the method in contract is called DeletionInfo and, ironically, EverExisted will return false if container exists but was not removed. I find this confusing.

But this is what you _actually_ check: the method in contract is called `DeletionInfo` and, ironically, `EverExisted` will return false if container exists but was not removed. I find this confusing.
Author
Member

Alright :) Fixed to WasRemoved. I don't mind

Alright :) Fixed to `WasRemoved`. I don't mind
Author
Member

Alright :) Fixed to WasRemoved. I don't mind

Alright :) Fixed to `WasRemoved`. I don't mind
fyrchik marked this conversation as resolved
aarifullin force-pushed fix/574-cnt_ever_existed from 1c0e9a1f09 to 06558507c6 2023-08-25 08:54:45 +00:00 Compare
aarifullin force-pushed fix/574-cnt_ever_existed from 06558507c6 to c166fa007c 2023-08-25 11:16:31 +00:00 Compare
fyrchik reviewed 2023-08-25 11:22:39 +00:00
@ -133,2 +134,3 @@
cachedContainerLister.update(cnr.Value.Owner(), ev.ID, true)
cachedContainerStorage.set(ev.ID, cnr, nil)
cachedContainerStorage.containerCache.set(ev.ID, cnr, nil)
cachedContainerStorage.delInfoCache.set(ev.ID, nil, new(apistatus.ContainerNotFound))
Owner

Containers are long-living entities, do we need to PUT sth here (vs just deleting or not touching at all)?

Containers are long-living entities, do we need to PUT sth here (vs just deleting or not touching at all)?
Author
Member

Alright, agree!

Alright, agree!
fyrchik marked this conversation as resolved
@ -15,0 +20,4 @@
DeletionInfo(cid []byte) (*containercore.DelInfo, error)
}
func AsContainerSpecInfoProvider(w *Client) containercore.Source {
Owner

The title should be adjusted (do we already have AsContainerSource?

The title should be adjusted (do we already have `AsContainerSource`?
Author
Member

Sorry, my fault, bad refactoring

Sorry, my fault, bad refactoring
fyrchik marked this conversation as resolved
@ -39,0 +39,4 @@
return nil, new(apistatus.ContainerNotFound)
},
deletionInfo: func(id cid.ID) (*container.DelInfo, error) {
return &container.DelInfo{}, nil
Owner

Do we check the behaviour we introduce in this task, namely ContainerNotFound in both methods?

Do we check the behaviour we introduce in this task, namely ContainerNotFound in both methods?
Author
Member

We don't but actually no good idea comes to my mind how I would check this case

This scenario when container is really removed can be tested by checking bury channel. In the case when container never existed in chain - nothing happens. Probably, to check the scenario I need to make some refactoring for processObject and put messages like (object won't be deleted) and check them from the unit-test side. WDYT?

We don't but actually no good idea comes to my mind how I would check this case This scenario when container is _really_ removed can be tested by checking bury channel. In the case when container never existed in chain - nothing happens. Probably, to check the scenario I need to make some refactoring for `processObject` and put messages like (`object won't be deleted`) and check them from the unit-test side. WDYT?
Owner

Just check that bury channel is empty (maybe wait for 2 seconds)? I mean if there is no definite info, we do not expect anything there.

Just check that bury channel is empty (maybe wait for 2 seconds)? I mean if there is no definite info, we do not expect anything there.
requested reviews from dstepanov-yadro, fyrchik 2023-08-25 11:51:07 +00:00
aarifullin force-pushed fix/574-cnt_ever_existed from c166fa007c to 1c0540c529 2023-08-25 12:39:43 +00:00 Compare
dstepanov-yadro reviewed 2023-08-28 13:59:43 +00:00
@ -42,6 +42,7 @@ const (
NotificatorNotificatorStartProcessingObjectNotifications = "notificator: start processing object notifications"
NotificatorNotificatorProcessingObjectNotification = "notificator: processing object notification"
PolicerCouldNotGetContainer = "could not get container"
PolicerCouldNotConfirmContainerRemoval = "could not confirm container remova"

remova - typo

```remova``` - typo
Author
Member

fixed 👍

fixed 👍
dstepanov-yadro approved these changes 2023-08-28 14:02:24 +00:00
aarifullin force-pushed fix/574-cnt_ever_existed from 1c0540c529 to 57dcd5a53c 2023-08-28 14:07:02 +00:00 Compare
fyrchik approved these changes 2023-08-28 14:18:01 +00:00
fyrchik approved these changes 2023-08-28 14:21:33 +00:00
fyrchik merged commit 4ea0df77d0 into master 2023-08-28 14:21:40 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#640
No description provided.