TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 104 Pull requests 13 Projects Releases 2 Activity Actions

Set IO tags #1608

Merged
fyrchik merged 13 commits from dstepanov-yadro/frostfs-node:feat/tagging into master 2025-02-07 14:05:59 +00:00
Conversation 0 Commits 13 Files changed 31 +625 -30
dstepanov-yadro commented 2025-01-21 11:16:47 +00:00
Member
Copy link
  • IO tag from the gRPC request metadata is set for server requests
  • for client requests IO tag is set in the gRPC request metadata
  • tree service and object service have IO tag adjustment layers, because those services store and retrieve data from shards
  • for server requests for control service IO tag critical always set
  • config file defines a list of public keys for which requests with IO tags critical and internal are allowed
  • IO tag writecache is used for writecache flush
  • IO tag policer is used for policer local requests
  • IO tag background is used for tree sync and gc
- IO tag from the gRPC request metadata is set for server requests - for client requests IO tag is set in the gRPC request metadata - tree service and object service have IO tag adjustment layers, because those services store and retrieve data from shards - for server requests for control service IO tag `critical` always set - config file defines a list of public keys for which requests with IO tags `critical` and `internal` are allowed - IO tag `writecache` is used for writecache flush - IO tag `policer` is used for policer local requests - IO tag `background` is used for tree sync and gc
dstepanov-yadro force-pushed feat/tagging from 9442a1d2ee to a88085f38b 2025-01-29 14:55:29 +00:00 Compare
dstepanov-yadro force-pushed feat/tagging from a88085f38b to ca6e997706 2025-01-31 07:13:55 +00:00 Compare
dstepanov-yadro force-pushed feat/tagging from ca6e997706 to 19bab4bf9a 2025-01-31 07:36:57 +00:00 Compare
dstepanov-yadro force-pushed feat/tagging from 19bab4bf9a to e43c9d73fd 2025-01-31 08:03:21 +00:00 Compare
dstepanov-yadro force-pushed feat/tagging from 4ab08d1efe to a253afef44 2025-01-31 09:21:45 +00:00 Compare
dstepanov-yadro reviewed 2025-01-31 09:30:17 +00:00
cmd/frostfs-cli/modules/tree/client.go Outdated
@ -34,11 +33,9 @@ func _client() (tree.TreeServiceClient, error) {
opts := []grpc.DialOption{
grpc.WithChainUnaryInterceptor(
metrics.NewUnaryClientInterceptor(),
dstepanov-yadro commented 2025-01-31 09:30:17 +00:00
Author
Member
Copy link

No need for metric in frostfs-cli

No need for metric in frostfs-cli
dstepanov-yadro reviewed 2025-01-31 09:32:11 +00:00
cmd/frostfs-node/control.go Outdated
@ -53,1 +56,3 @@
c.cfgControlService.server = grpc.NewServer()
c.cfgControlService.server = grpc.NewServer(
grpc.ChainUnaryInterceptor(
qos.NewSetCriticalIOTagUnaryServerInterceptor(),
dstepanov-yadro commented 2025-01-31 09:32:11 +00:00
Author
Member
Copy link

First interceptor is the outer most, so it will be incoming RPC -> set IO tag critical -> metrics -> tracing

First interceptor is the outer most, so it will be incoming RPC -> set IO tag critical -> metrics -> tracing
dstepanov-yadro force-pushed feat/tagging from a253afef44 to ee8140d50b 2025-01-31 10:54:20 +00:00 Compare
dstepanov-yadro reviewed 2025-01-31 10:55:54 +00:00
cmd/frostfs-node/grpc.go Outdated
@ -130,10 +131,12 @@ func getGrpcServerOpts(ctx context.Context, c *cfg, sc *grpcconfig.Config) ([]gr
serverOpts := []grpc.ServerOption{
grpc.MaxRecvMsgSize(maxRecvMsgSize),
grpc.ChainUnaryInterceptor(
qos.NewUnaryServerInterceptor(),
dstepanov-yadro commented 2025-01-31 10:55:54 +00:00
Author
Member
Copy link

This is the outer most interceptor, so so it will be incoming RPC -> parse and set IO tag -> metrics -> tracing

This is the outer most interceptor, so so it will be incoming RPC -> parse and set IO tag -> metrics -> tracing
requested reviews from storage-core-committers, storage-core-developers 2025-01-31 14:24:31 +00:00
dstepanov-yadro changed title from WIP: Set IO tags to Set IO tags 2025-01-31 14:24:46 +00:00
dstepanov-yadro force-pushed feat/tagging from ee8140d50b to 5899ed74f3 2025-02-03 12:46:11 +00:00 Compare
acid-ant reviewed 2025-02-04 09:12:27 +00:00
internal/logs/logs.go Outdated
@ -510,4 +510,7 @@ const (
BlobovniczatreeFailedToRemoveRebuildTempFile = "failed to remove rebuild temp file"
WritecacheCantGetObject = "can't get an object from fstree"
FailedToUpdateMultinetConfiguration = "failed to update multinet configuration"
FailerToParseIncommingIOTag = "failed to parse incomming IO tag"
acid-ant commented 2025-02-04 09:12:27 +00:00
Member
Copy link

FailerToParseIncommingIOTag -> FailedToParseIncommingIOTag

FailerToParseIncommingIOTag -> FailedToParseIncommingIOTag
dstepanov-yadro commented 2025-02-04 15:30:15 +00:00
Author
Member
Copy link

fixed

fixed
acid-ant approved these changes 2025-02-04 09:28:22 +00:00
Dismissed
dstepanov-yadro force-pushed feat/tagging from 5899ed74f3 to b4a5aabc89 2025-02-04 15:28:29 +00:00 Compare
dstepanov-yadro force-pushed feat/tagging from b4a5aabc89 to af028748ea 2025-02-04 15:30:14 +00:00 Compare
dstepanov-yadro dismissed acid-ant's review 2025-02-04 15:30:17 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dstepanov-yadro force-pushed feat/tagging from af028748ea to 7a7ec844c2 2025-02-05 06:42:40 +00:00 Compare
requested reviews from storage-core-committers, storage-core-developers 2025-02-05 06:43:56 +00:00
dstepanov-yadro added 1 commit 2025-02-05 12:32:46 +00:00
[#1608] shard: Add IO tag for rebuild
All checks were successful
DCO action / DCO (pull_request) Successful in 38s
Details
Vulncheck / Vulncheck (pull_request) Successful in 59s
Details
Build / Build Components (pull_request) Successful in 1m43s
Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 1m42s
Details
Tests and linters / Run gofumpt (pull_request) Successful in 2m47s
Details
Tests and linters / gopls check (pull_request) Successful in 3m6s
Details
Tests and linters / Staticcheck (pull_request) Successful in 3m26s
Details
Tests and linters / Lint (pull_request) Successful in 3m30s
Details
Tests and linters / Tests (pull_request) Successful in 3m40s
Details
Tests and linters / Tests with -race (pull_request) Successful in 5m7s
Details
998f7a18b7 
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
aarifullin reviewed 2025-02-05 14:45:27 +00:00
pkg/services/object/qos.go Outdated
@ -0,0 +13,4 @@
AdjustIncommingTag(ctx context.Context, requestSignPublicKey []byte) context.Context
}
type qoSObjectService struct {
aarifullin commented 2025-02-05 14:45:27 +00:00
Member
Copy link

qoSObjectService -> qosObjectService? :)

`qoSObjectService` -> `qosObjectService`? :)
dstepanov-yadro commented 2025-02-06 08:13:57 +00:00
Author
Member
Copy link

fixed

fixed
aarifullin marked this conversation as resolved
dstepanov-yadro force-pushed feat/tagging from 998f7a18b7 to 2faffe8372 2025-02-06 08:14:06 +00:00 Compare
aarifullin approved these changes 2025-02-06 08:46:20 +00:00
Dismissed
aarifullin left a comment
Member
Copy link

Awesome

Awesome
fyrchik reviewed 2025-02-06 09:04:42 +00:00
cmd/frostfs-node/control.go Outdated
@ -54,0 +59,4 @@
metrics.NewUnaryServerInterceptor(),
tracing.NewUnaryServerInterceptor(),
),
// control service has no stream methods, so no stream interceptors added
fyrchik commented 2025-02-06 08:41:47 +00:00
Owner
Copy link

When it will have stream methods, we won't remember this line.
Is there any problem with adding them?

When it will have stream methods, we won't remember this line. Is there any problem with adding them?
dstepanov-yadro commented 2025-02-06 11:45:46 +00:00
Author
Member
Copy link

Just I don't want to add something I can't test.

Just I don't want to add something I can't test.
cmd/frostfs-node/main.go Outdated
@ -101,6 +101,7 @@ func initApp(ctx context.Context, c *cfg) {
initAndLog(ctx, c, "gRPC", func(c *cfg) { initGRPC(ctx, c) })
initAndLog(ctx, c, "netmap", func(c *cfg) { initNetmapService(ctx, c) })
initAndLog(ctx, c, "qos", func(c *cfg) { initQoSService(c) })
fyrchik commented 2025-02-06 08:45:21 +00:00
Owner
Copy link

Could you explain, why do we have both interceptor and service?

Could you explain, why do we have both interceptor and service?
dstepanov-yadro commented 2025-02-06 09:36:40 +00:00
Author
Member
Copy link

See comment about tree service: #1608 (comment)

See comment about tree service: https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/1608#issuecomment-66636
cmd/frostfs-node/qos.go Outdated
@ -0,0 +62,4 @@
}
nm, err := s.netmapSource.GetNetMap(0)
if err != nil {
s.logger.Error(ctx, logs.FailedToGetNetmapToAdjustIOTag, zap.Error(err))
fyrchik commented 2025-02-06 08:49:54 +00:00
Owner
Copy link

I won't be so sure about Error -- it could be done on every request.
How about debug?
Not insisting.

I won't be so sure about `Error` -- it could be done on every request. How about `debug`? Not insisting.
dstepanov-yadro commented 2025-02-06 09:39:15 +00:00
Author
Member
Copy link

Ok, fixed

Ok, fixed
fyrchik marked this conversation as resolved
cmd/frostfs-node/qos.go Outdated
@ -0,0 +70,4 @@
return ctx
}
}
return qosTagging.ContextWithIOTag(ctx, qos.IOTagClient.String())
fyrchik commented 2025-02-06 08:44:01 +00:00
Owner
Copy link

Does qos.IOTagClient.String() allocate?
It seems constant, so better be it.

Does `qos.IOTagClient.String()` allocate? It seems constant, so better be it.
dstepanov-yadro commented 2025-02-06 09:56:26 +00:00
Author
Member
Copy link
package qos

import (
	"testing"

	"github.com/stretchr/testify/require"
)

var ioTagBackgroundString = "background"

func (t IOTag) String2() string {
	switch t {
	case IOTagBackground:
		return ioTagBackgroundString
	default:
		return ""
	}
}

func BenchmarkTag(b *testing.B) {
	tag := IOTagBackground
	b.Run("cast", func(b *testing.B) {
		b.ReportAllocs()
		b.ResetTimer()
		require.Equal(b, ioTagBackgroundString, string(tag))
	})
	b.Run("String()", func(b *testing.B) {
		b.ReportAllocs()
		b.ResetTimer()
		require.Equal(b, ioTagBackgroundString, tag.String())
	})
	b.Run("String2()", func(b *testing.B) {
		b.ReportAllocs()
		b.ResetTimer()
		require.Equal(b, ioTagBackgroundString, tag.String2())
	})
}


BenchmarkTag/cast-8         	1000000000	         0.0000052 ns/op	       0 B/op	       0 allocs/op
BenchmarkTag/String()-8     	1000000000	         0.0000024 ns/op	       0 B/op	       0 allocs/op
BenchmarkTag/String2()-8    	1000000000	         0.0000028 ns/op	       0 B/op	       0 allocs/op
PASS
``` package qos import ( "testing" "github.com/stretchr/testify/require" ) var ioTagBackgroundString = "background" func (t IOTag) String2() string { switch t { case IOTagBackground: return ioTagBackgroundString default: return "" } } func BenchmarkTag(b *testing.B) { tag := IOTagBackground b.Run("cast", func(b *testing.B) { b.ReportAllocs() b.ResetTimer() require.Equal(b, ioTagBackgroundString, string(tag)) }) b.Run("String()", func(b *testing.B) { b.ReportAllocs() b.ResetTimer() require.Equal(b, ioTagBackgroundString, tag.String()) }) b.Run("String2()", func(b *testing.B) { b.ReportAllocs() b.ResetTimer() require.Equal(b, ioTagBackgroundString, tag.String2()) }) } BenchmarkTag/cast-8 1000000000 0.0000052 ns/op 0 B/op 0 allocs/op BenchmarkTag/String()-8 1000000000 0.0000024 ns/op 0 B/op 0 allocs/op BenchmarkTag/String2()-8 1000000000 0.0000028 ns/op 0 B/op 0 allocs/op PASS ```
fyrchik marked this conversation as resolved
config/example/node.env Outdated
@ -226,2 +226,4 @@
FROSTFS_MULTINET_RESTRICT=false
FROSTFS_MULTINET_FALLBACK_DELAY=350ms
FROSTFS_QOS_CRITICAL_AUTHORIZED_KEYS="035839e45d472a3b7769a2a1bd7d54c4ccd4943c3b40f547870e83a8fcbfb3ce11 028f42cfcb74499d7b15b35d9bff260a1c8d27de4f446a627406a382d8961486d6"
fyrchik commented 2025-02-06 08:46:21 +00:00
Owner
Copy link

Could you provide a different pair of keys for different section?
Testing same keys easily misses copy-paste errors.

Could you provide a different pair of keys for different section? Testing same keys easily misses copy-paste errors.
dstepanov-yadro commented 2025-02-06 11:14:49 +00:00
Author
Member
Copy link

done

done
fyrchik marked this conversation as resolved
config/example/node.json Outdated
@ -140,3 +140,3 @@
},
"get": {
"priority": ["$attribute:ClusterName", "$attribute:UN-LOCODE"]
"priority": [
fyrchik commented 2025-02-06 08:48:06 +00:00
Owner
Copy link

Please, move formatting changes to a separate commit.
Or, better, leave as is, because someone else will change that again if we don't have this automated.

Please, move formatting changes to a separate commit. Or, better, leave as is, because someone else will change that again if we don't have this automated.
dstepanov-yadro commented 2025-02-06 10:32:32 +00:00
Author
Member
Copy link

fixed

fixed
fyrchik marked this conversation as resolved
pkg/services/tree/sync.go Outdated
@ -381,6 +387,7 @@ func (s *Service) SynchronizeAll() error {
}
func (s *Service) syncLoop(ctx context.Context) {
ctx = tagging.ContextWithIOTag(ctx, qos.IOTagBackground.String())
fyrchik commented 2025-02-06 09:04:15 +00:00
Owner
Copy link

Why have you decided to put it inside the function and not at the callsite?

Why have you decided to put it inside the function and not at the callsite?
dstepanov-yadro commented 2025-02-06 09:35:31 +00:00
Author
Member
Copy link

Right, moved

Right, moved
fyrchik marked this conversation as resolved
pkg/util/logger/log.go Outdated
@ -40,0 +24,4 @@
l.z.Error(msg, appendContext(ctx, fields...)...)
}
func appendContext(ctx context.Context, fields ...zap.Field) []zap.Field {
fyrchik commented 2025-02-06 09:01:58 +00:00
Owner
Copy link

@acid-ant please, look
The PR with logging tags will be adapted, depending on the merge order.

@acid-ant please, look The PR with logging tags will be adapted, depending on the merge order.
acid-ant commented 2025-02-06 09:26:22 +00:00
Member
Copy link

Yeah, saw this, merge will be easy.

Yeah, saw this, merge will be easy.
fyrchik requested changes 2025-02-06 09:09:43 +00:00
Dismissed
pkg/services/tree/qos.go Outdated
@ -0,0 +25,4 @@
}
func (i *ioTagAdjust) Add(ctx context.Context, req *AddRequest) (*AddResponse, error) {
ctx = i.a.AdjustIncommingTag(ctx, req.GetSignature().GetKey())
fyrchik commented 2025-02-06 09:09:40 +00:00
Owner
Copy link

I don't understand all these different places where we work with tags.
Consider tree service: we have 2 interceptors and also NewIOTagAdjustServer.
Why are all those different?

I don't understand all these different places where we work with tags. Consider tree service: we have 2 interceptors and also `NewIOTagAdjustServer`. Why are all those different?
dstepanov-yadro commented 2025-02-06 09:25:37 +00:00
Author
Member
Copy link
  1. tagging.NewUnaryServerInterceptor() and tagging.NewStreamServerInterceptor() extract an IO tag from the metadata of incommin grpc request and store it in the context.Context. They don't know anything about frostfs-node allowed tags.
  2. tagging.NewUnaryClientInteceptor() and tagging.NewStreamClientInterceptor() extract an IO tag from context.Context and store it in the metadata of outcomming grpc request. They also don't know anything about frostfs-node allowed tags.
  3. AdjustIncommingTag checks if incomming request's signer has rights to use internal or critical IO tag and also that tag has allowed value.
  4. qos.NewAdjustOutgoingIOTagUnaryClientInterceptor() and qos.NewAdjustOutgoingIOTagStreamClientInterceptor() replace background, policer and writecache tag values with internal IO tag value for outcomming requests.
1. `tagging.NewUnaryServerInterceptor()` and `tagging.NewStreamServerInterceptor()` extract an IO tag from the metadata of incommin grpc request and store it in the context.Context. They don't know anything about frostfs-node allowed tags. 2. `tagging.NewUnaryClientInteceptor()` and `tagging.NewStreamClientInterceptor()` extract an IO tag from context.Context and store it in the metadata of outcomming grpc request. They also don't know anything about frostfs-node allowed tags. 3. `AdjustIncommingTag` checks if incomming request's signer has rights to use `internal` or `critical` IO tag and also that tag has allowed value. 4. `qos.NewAdjustOutgoingIOTagUnaryClientInterceptor()` and `qos.NewAdjustOutgoingIOTagStreamClientInterceptor()` replace `background`, `policer` and `writecache` tag values with `internal` IO tag value for outcomming requests.
fyrchik commented 2025-02-07 10:23:12 +00:00
Owner
Copy link

Regarding (4), I think there is a slight confusion:
Any thread, initiating network request could set its meta explicitly (policer, replicator).
And writecache doesn't create network requests, for example.

Do you think introducing interceptor is a better solution, than explicit internal on all callsites?

Regarding (4), I think there is a slight confusion: Any thread, initiating network request could set its meta explicitly (policer, replicator). And writecache doesn't create network requests, for example. Do you think introducing interceptor is a better solution, than explicit `internal` on all callsites?
fyrchik commented 2025-02-07 10:23:26 +00:00
Owner
Copy link

Thanks, it became clearer know.
Adjust doesn't scream VALIDATION, thats where my confusion came from.

Thanks, it became clearer know. `Adjust` doesn't scream VALIDATION, thats where my confusion came from.
dstepanov-yadro commented 2025-02-07 11:54:03 +00:00
Author
Member
Copy link

Do you think introducing interceptor is a better solution, than explicit internal on all callsites?

I tried, but it looked very scary (especially in policer/replicator).

> Do you think introducing interceptor is a better solution, than explicit internal on all callsites? I tried, but it looked very scary (especially in policer/replicator).
fyrchik marked this conversation as resolved
achuprov reviewed 2025-02-06 09:32:31 +00:00
cmd/frostfs-node/qos.go Outdated
@ -0,0 +40,4 @@
}
}
func (s *cfgQoSService) AdjustIncommingTag(ctx context.Context, requestSignPublicKey []byte) context.Context {
achuprov commented 2025-02-06 09:32:31 +00:00
Member
Copy link

Typo AdjustIncommingTag -> AdjustIncomingTag

Typo `AdjustIncommingTag` -> `AdjustIncomingTag`
dstepanov-yadro commented 2025-02-06 09:47:06 +00:00
Author
Member
Copy link

fixed

fixed
a-savchuk commented 2025-02-06 10:21:51 +00:00
Member
Copy link

This typo still appears in many places, including commit messages. Please make sure to fix it everywhere

This typo still appears in many places, including commit messages. Please make sure to fix it everywhere
dstepanov-yadro commented 2025-02-06 11:53:38 +00:00
Author
Member
Copy link

fixed

fixed
achuprov marked this conversation as resolved
dstepanov-yadro force-pushed feat/tagging from 2faffe8372 to 1d7cbd5b9a 2025-02-06 09:35:28 +00:00 Compare
dstepanov-yadro dismissed aarifullin's review 2025-02-06 09:35:28 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dstepanov-yadro force-pushed feat/tagging from 1d7cbd5b9a to 557678321a 2025-02-06 09:39:11 +00:00 Compare
dstepanov-yadro force-pushed feat/tagging from 557678321a to 12206c7658 2025-02-06 09:46:50 +00:00 Compare
dstepanov-yadro force-pushed feat/tagging from 12206c7658 to a63fc2f2d8 2025-02-06 10:32:12 +00:00 Compare
dstepanov-yadro force-pushed feat/tagging from a63fc2f2d8 to 447d93e9fd 2025-02-06 11:15:06 +00:00 Compare
dstepanov-yadro force-pushed feat/tagging from 447d93e9fd to a3622fb250 2025-02-06 11:53:41 +00:00 Compare
fyrchik approved these changes 2025-02-07 12:25:55 +00:00
Dismissed
dstepanov-yadro force-pushed feat/tagging from a3622fb250 to 3b8ca0d538 2025-02-07 12:41:33 +00:00 Compare
dstepanov-yadro dismissed fyrchik's review 2025-02-07 12:41:33 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dstepanov-yadro force-pushed feat/tagging from 3b8ca0d538 to 5d79abe523 2025-02-07 12:43:36 +00:00 Compare
requested reviews from storage-core-committers, storage-core-developers 2025-02-07 12:43:55 +00:00
aarifullin approved these changes 2025-02-07 13:42:37 +00:00
fyrchik merged commit 5d79abe523 into master 2025-02-07 14:05:59 +00:00
fyrchik referenced this pull request from a commit 2025-02-07 14:06:01 +00:00
[#1608] logger: Add IO tag logging
fyrchik referenced this pull request from a commit 2025-02-07 14:06:02 +00:00
[#1608] shard: Add IO tag to GC
fyrchik referenced this pull request from a commit 2025-02-07 14:06:02 +00:00
[#1608] shard: Add IO tag for rebuild
Sign in to join this conversation.
Reviewers
No reviewers
aarifullin
TrueCloudLab/storage-core-committers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
6 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#1608
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:feat/tagging"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?