metadata: improve deleting a non-empty namespace's error message#5360
metadata: improve deleting a non-empty namespace's error message#5360estesp merged 1 commit intocontainerd:masterfrom
Conversation
|
Build succeeded.
|
|
I'm not sure I understand the value of having all the types listed in the error message. It can have a potential costly memory usage to produce this message and produce a large error message. Would it help just having something on the client which could give some stats on the namespace? All the objects listed can easily be requested by the client already. |
|
Yeah. I do agree that iterating over How about returning information about non-empty buckets (e.g. image and container), without mentioning specific objects? Then the worst case is only 4 buckets (image, blob, container and snapshot), and the operator can query specific types based on the error message. |
|
@kzys that sounds reasonable to me |
|
Build succeeded.
|
|
Build succeeded.
|
|
Build succeeded.
|
|
Build succeeded.
|
There was a problem hiding this comment.
I would rather keep things simple and just use strings.Join here
for same reasons discussed in: #4248 (comment)
There was a problem hiding this comment.
How do you feel about this variation:
func (s *namespaceStore) listNs(namespace string) ([]string, error) {
var out []string
if !isBucketEmpty(getImagesBucket(s.tx, namespace)) {
out = append(out, "images")
}
if !isBucketEmpty(getBlobsBucket(s.tx, namespace)) {
out = append(out, "blobs")
}
...
if snbkt := getSnapshottersBucket(s.tx, namespace); snbkt != nil {
if err := snbkt.ForEach(func(k, v []byte) error {
if v == nil {
if !isBucketEmpty(snbkt.Bucket(k)) {
out = append(out, "snapshots-"+string(k))
}
}
return nil
}); err != nil {
return nil, err
}
}
return out, nil
}- if we want to be informative, let's use
kto clarify which snapshotter. - no need to remap map -> key slice after calling listNamespace
- no need to have
pair
There was a problem hiding this comment.
Looks good. Updated in the last revision.
|
Build succeeded.
|
This comment has been minimized.
This comment has been minimized.
|
@cpuguy83 any reason the last merge to master is generating a comment in this PR about tests during that (merge, not PR) workflow run? 🤔 😂 |
|
@estesp I think it's because how |
This comment has been minimized.
This comment has been minimized.
|
Build succeeded.
|
This comment has been minimized.
This comment has been minimized.
Deleting a non-empty namespace fails with > namespace must be empty: failed precondition This change improves the error message by listing the types of the objects in the namespace that prevent deletion. Signed-off-by: Kazuyoshi Kato <[email protected]>
|
Build succeeded.
|
|
It is green now :) |
5 participants
Deleting a non-empty namespace fails with
This change improves the error message by listing all objects in
the namespace that prevent deletion.
Signed-off-by: Kazuyoshi Kato [email protected]