From da83807d6bcef1c1f0bb78479c5ec17f150f5eda Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Thu, 26 Oct 2023 16:15:16 -0400 Subject: [PATCH] 21126: Add AllowTrashWhenReadOnly flag. Split KeepMount's ReadOnly flag into AllowWrite and AllowTrash flags. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/config/config.default.yml | 1 + sdk/go/arvados/config.go | 13 +++++++------ sdk/go/arvados/keep_service.go | 3 ++- services/keep-balance/balance.go | 18 ++++++++++-------- services/keep-balance/balance_run_test.go | 8 ++++++++ services/keep-balance/balance_test.go | 15 +++++++++++++-- services/keepstore/azure_blob_volume.go | 3 +-- services/keepstore/command.go | 2 +- services/keepstore/handlers.go | 6 ++++-- services/keepstore/keepstore.go | 4 +++- services/keepstore/s3aws_volume.go | 2 +- services/keepstore/trash_worker.go | 6 ++++-- services/keepstore/unix_volume.go | 3 +-- services/keepstore/volume.go | 10 +++++----- 14 files changed, 61 insertions(+), 33 deletions(-) diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml index 32727b1bce..593abad854 100644 --- a/lib/config/config.default.yml +++ b/lib/config/config.default.yml @@ -1684,6 +1684,7 @@ Clusters: ReadOnly: false "http://host1.example:25107": {} ReadOnly: false + AllowTrashWhenReadOnly: false Replication: 1 StorageClasses: # If you have configured storage classes (see StorageClasses diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go index a3e54952da..5db0222e27 100644 --- a/sdk/go/arvados/config.go +++ b/sdk/go/arvados/config.go @@ -316,12 +316,13 @@ type StorageClassConfig struct { } type Volume struct { - AccessViaHosts map[URL]VolumeAccess - ReadOnly bool - Replication int - StorageClasses map[string]bool - Driver string - DriverParameters json.RawMessage + AccessViaHosts map[URL]VolumeAccess + ReadOnly bool + AllowTrashWhenReadOnly bool + Replication int + StorageClasses map[string]bool + Driver string + DriverParameters json.RawMessage } type S3VolumeDriverParameters struct { diff --git a/sdk/go/arvados/keep_service.go b/sdk/go/arvados/keep_service.go index 5b6d71a4fb..85750d8cfc 100644 --- a/sdk/go/arvados/keep_service.go +++ b/sdk/go/arvados/keep_service.go @@ -33,7 +33,8 @@ type KeepService struct { type KeepMount struct { UUID string `json:"uuid"` DeviceID string `json:"device_id"` - ReadOnly bool `json:"read_only"` + AllowWrite bool `json:"allow_write"` + AllowTrash bool `json:"allow_trash"` Replication int `json:"replication"` StorageClasses map[string]bool `json:"storage_classes"` } diff --git a/services/keep-balance/balance.go b/services/keep-balance/balance.go index 215c5e1b5b..e44dfeda87 100644 --- a/services/keep-balance/balance.go +++ b/services/keep-balance/balance.go @@ -227,7 +227,7 @@ func (bal *Balancer) cleanupMounts() { rwdev := map[string]*KeepService{} for _, srv := range bal.KeepServices { for _, mnt := range srv.mounts { - if !mnt.ReadOnly { + if mnt.AllowWrite { rwdev[mnt.UUID] = srv } } @@ -237,7 +237,7 @@ func (bal *Balancer) cleanupMounts() { for _, srv := range bal.KeepServices { var dedup []*KeepMount for _, mnt := range srv.mounts { - if mnt.ReadOnly && rwdev[mnt.UUID] != nil { + if !mnt.AllowWrite && rwdev[mnt.UUID] != nil { bal.logf("skipping srv %s readonly mount %q because same volume is mounted read-write on srv %s", srv, mnt.UUID, rwdev[mnt.UUID]) } else { dedup = append(dedup, mnt) @@ -587,9 +587,11 @@ func (bal *Balancer) setupLookupTables() { for _, mnt := range srv.mounts { bal.mounts++ - // All mounts on a read-only service are - // effectively read-only. - mnt.ReadOnly = mnt.ReadOnly || srv.ReadOnly + if srv.ReadOnly { + // All mounts on a read-only service + // are effectively read-only. + mnt.AllowWrite = false + } for class := range mnt.StorageClasses { if mbc := bal.mountsByClass[class]; mbc == nil { @@ -667,7 +669,7 @@ func (bal *Balancer) balanceBlock(blkid arvados.SizedDigest, blk *BlockState) ba slots = append(slots, slot{ mnt: mnt, repl: repl, - want: repl != nil && mnt.ReadOnly, + want: repl != nil && !mnt.AllowTrash, }) } } @@ -756,7 +758,7 @@ func (bal *Balancer) balanceBlock(blkid arvados.SizedDigest, blk *BlockState) ba protMnt[slot.mnt] = true replProt += slot.mnt.Replication } - if replWant < desired && (slot.repl != nil || !slot.mnt.ReadOnly) { + if replWant < desired && (slot.repl != nil || slot.mnt.AllowWrite) { slots[i].want = true wantSrv[slot.mnt.KeepService] = true wantMnt[slot.mnt] = true @@ -875,7 +877,7 @@ func (bal *Balancer) balanceBlock(blkid arvados.SizedDigest, blk *BlockState) ba case slot.repl == nil && slot.want && len(blk.Replicas) == 0: lost = true change = changeNone - case slot.repl == nil && slot.want && !slot.mnt.ReadOnly: + case slot.repl == nil && slot.want && slot.mnt.AllowWrite: slot.mnt.KeepService.AddPull(Pull{ SizedDigest: blkid, From: blk.Replicas[0].KeepMount.KeepService, diff --git a/services/keep-balance/balance_run_test.go b/services/keep-balance/balance_run_test.go index db9a668481..962bd40ade 100644 --- a/services/keep-balance/balance_run_test.go +++ b/services/keep-balance/balance_run_test.go @@ -89,21 +89,29 @@ var stubMounts = map[string][]arvados.KeepMount{ UUID: "zzzzz-ivpuk-000000000000000", DeviceID: "keep0-vol0", StorageClasses: map[string]bool{"default": true}, + AllowWrite: true, + AllowTrash: true, }}, "keep1.zzzzz.arvadosapi.com:25107": {{ UUID: "zzzzz-ivpuk-100000000000000", DeviceID: "keep1-vol0", StorageClasses: map[string]bool{"default": true}, + AllowWrite: true, + AllowTrash: true, }}, "keep2.zzzzz.arvadosapi.com:25107": {{ UUID: "zzzzz-ivpuk-200000000000000", DeviceID: "keep2-vol0", StorageClasses: map[string]bool{"default": true}, + AllowWrite: true, + AllowTrash: true, }}, "keep3.zzzzz.arvadosapi.com:25107": {{ UUID: "zzzzz-ivpuk-300000000000000", DeviceID: "keep3-vol0", StorageClasses: map[string]bool{"default": true}, + AllowWrite: true, + AllowTrash: true, }}, } diff --git a/services/keep-balance/balance_test.go b/services/keep-balance/balance_test.go index f9fca1431b..cb61ea58cc 100644 --- a/services/keep-balance/balance_test.go +++ b/services/keep-balance/balance_test.go @@ -87,6 +87,8 @@ func (bal *balancerSuite) SetUpTest(c *check.C) { KeepMount: arvados.KeepMount{ UUID: fmt.Sprintf("zzzzz-mount-%015x", i), StorageClasses: map[string]bool{"default": true}, + AllowWrite: true, + AllowTrash: true, }, KeepService: srv, }} @@ -169,7 +171,8 @@ func (bal *balancerSuite) testMultipleViews(c *check.C, readonly bool) { KeepMount: arvados.KeepMount{ DeviceID: bal.srvs[(i+1)%len(bal.srvs)].mounts[0].KeepMount.DeviceID, UUID: bal.srvs[(i+1)%len(bal.srvs)].mounts[0].KeepMount.UUID, - ReadOnly: readonly, + AllowWrite: !readonly, + AllowTrash: !readonly, Replication: 1, StorageClasses: map[string]bool{"default": true}, }, @@ -374,7 +377,7 @@ func (bal *balancerSuite) TestDecreaseReplBlockTooNew(c *check.C) { } func (bal *balancerSuite) TestCleanupMounts(c *check.C) { - bal.srvs[3].mounts[0].KeepMount.ReadOnly = true + bal.srvs[3].mounts[0].KeepMount.AllowWrite = false bal.srvs[3].mounts[0].KeepMount.DeviceID = "abcdef" bal.srvs[14].mounts[0].KeepMount.UUID = bal.srvs[3].mounts[0].KeepMount.UUID bal.srvs[14].mounts[0].KeepMount.DeviceID = "abcdef" @@ -583,6 +586,8 @@ func (bal *balancerSuite) TestChangeStorageClasses(c *check.C) { // classes=[special,special2]. bal.srvs[9].mounts = []*KeepMount{{ KeepMount: arvados.KeepMount{ + AllowWrite: true, + AllowTrash: true, Replication: 1, StorageClasses: map[string]bool{"special": true}, UUID: "zzzzz-mount-special00000009", @@ -591,6 +596,8 @@ func (bal *balancerSuite) TestChangeStorageClasses(c *check.C) { KeepService: bal.srvs[9], }, { KeepMount: arvados.KeepMount{ + AllowWrite: true, + AllowTrash: true, Replication: 1, StorageClasses: map[string]bool{"special": true, "special2": true}, UUID: "zzzzz-mount-special20000009", @@ -603,6 +610,8 @@ func (bal *balancerSuite) TestChangeStorageClasses(c *check.C) { // classes=[special3], one with classes=[default]. bal.srvs[13].mounts = []*KeepMount{{ KeepMount: arvados.KeepMount{ + AllowWrite: true, + AllowTrash: true, Replication: 1, StorageClasses: map[string]bool{"special2": true}, UUID: "zzzzz-mount-special2000000d", @@ -611,6 +620,8 @@ func (bal *balancerSuite) TestChangeStorageClasses(c *check.C) { KeepService: bal.srvs[13], }, { KeepMount: arvados.KeepMount{ + AllowWrite: true, + AllowTrash: true, Replication: 1, StorageClasses: map[string]bool{"default": true}, UUID: "zzzzz-mount-00000000000000d", diff --git a/services/keepstore/azure_blob_volume.go b/services/keepstore/azure_blob_volume.go index 4fb469bdcb..56a52c913a 100644 --- a/services/keepstore/azure_blob_volume.go +++ b/services/keepstore/azure_blob_volume.go @@ -480,10 +480,9 @@ func (v *AzureBlobVolume) listBlobs(page int, params storage.ListBlobsParameters // Trash a Keep block. func (v *AzureBlobVolume) Trash(loc string) error { - if v.volume.ReadOnly { + if v.volume.ReadOnly && !v.volume.AllowTrashWhenReadOnly { return MethodDisabledError } - // Ideally we would use If-Unmodified-Since, but that // particular condition seems to be ignored by Azure. Instead, // we get the Etag before checking Mtime, and use If-Match to diff --git a/services/keepstore/command.go b/services/keepstore/command.go index 9761680fb5..db387f494b 100644 --- a/services/keepstore/command.go +++ b/services/keepstore/command.go @@ -211,7 +211,7 @@ func (h *handler) setup(ctx context.Context, cluster *arvados.Cluster, token str if d := h.Cluster.Collections.BlobTrashCheckInterval.Duration(); d > 0 && h.Cluster.Collections.BlobTrash && h.Cluster.Collections.BlobDeleteConcurrency > 0 { - go emptyTrash(h.volmgr.writables, d) + go emptyTrash(h.volmgr.mounts, d) } return nil diff --git a/services/keepstore/handlers.go b/services/keepstore/handlers.go index 60fdde89c7..abeb20fe86 100644 --- a/services/keepstore/handlers.go +++ b/services/keepstore/handlers.go @@ -475,8 +475,10 @@ func (rtr *router) handleDELETE(resp http.ResponseWriter, req *http.Request) { Deleted int `json:"copies_deleted"` Failed int `json:"copies_failed"` } - for _, vol := range rtr.volmgr.AllWritable() { - if err := vol.Trash(hash); err == nil { + for _, vol := range rtr.volmgr.Mounts() { + if !vol.KeepMount.AllowTrash { + continue + } else if err := vol.Trash(hash); err == nil { result.Deleted++ } else if os.IsNotExist(err) { continue diff --git a/services/keepstore/keepstore.go b/services/keepstore/keepstore.go index f83ba603d2..953aa047cb 100644 --- a/services/keepstore/keepstore.go +++ b/services/keepstore/keepstore.go @@ -49,7 +49,9 @@ func (e *KeepError) Error() string { func emptyTrash(mounts []*VolumeMount, interval time.Duration) { for range time.NewTicker(interval).C { for _, v := range mounts { - v.EmptyTrash() + if v.KeepMount.AllowTrash { + v.EmptyTrash() + } } } } diff --git a/services/keepstore/s3aws_volume.go b/services/keepstore/s3aws_volume.go index aaec02721b..18b30f4638 100644 --- a/services/keepstore/s3aws_volume.go +++ b/services/keepstore/s3aws_volume.go @@ -846,7 +846,7 @@ func (b *s3AWSbucket) Del(path string) error { // Trash a Keep block. func (v *S3AWSVolume) Trash(loc string) error { - if v.volume.ReadOnly { + if v.volume.ReadOnly && !v.volume.AllowTrashWhenReadOnly { return MethodDisabledError } if t, err := v.Mtime(loc); err != nil { diff --git a/services/keepstore/trash_worker.go b/services/keepstore/trash_worker.go index d7c73127eb..5e8a5a963c 100644 --- a/services/keepstore/trash_worker.go +++ b/services/keepstore/trash_worker.go @@ -36,10 +36,12 @@ func TrashItem(volmgr *RRVolumeManager, logger logrus.FieldLogger, cluster *arva var volumes []*VolumeMount if uuid := trashRequest.MountUUID; uuid == "" { - volumes = volmgr.AllWritable() - } else if mnt := volmgr.Lookup(uuid, true); mnt == nil { + volumes = volmgr.Mounts() + } else if mnt := volmgr.Lookup(uuid, false); mnt == nil { logger.Warnf("trash request for nonexistent mount: %v", trashRequest) return + } else if !mnt.KeepMount.AllowTrash { + logger.Warnf("trash request for mount with ReadOnly=true, AllowTrashWhenReadOnly=false: %v", trashRequest) } else { volumes = []*VolumeMount{mnt} } diff --git a/services/keepstore/unix_volume.go b/services/keepstore/unix_volume.go index 8c935dcddb..dee4bdc1c1 100644 --- a/services/keepstore/unix_volume.go +++ b/services/keepstore/unix_volume.go @@ -442,8 +442,7 @@ func (v *UnixVolume) Trash(loc string) error { // be re-written), or (b) Touch() will update the file's timestamp and // Trash() will read the correct up-to-date timestamp and choose not to // trash the file. - - if v.volume.ReadOnly || !v.cluster.Collections.BlobTrash { + if v.volume.ReadOnly && !v.volume.AllowTrashWhenReadOnly { return MethodDisabledError } if err := v.lock(context.TODO()); err != nil { diff --git a/services/keepstore/volume.go b/services/keepstore/volume.go index c3b8cd6283..f597ff5781 100644 --- a/services/keepstore/volume.go +++ b/services/keepstore/volume.go @@ -316,8 +316,6 @@ func makeRRVolumeManager(logger logrus.FieldLogger, cluster *arvados.Cluster, my if err != nil { return nil, fmt.Errorf("error initializing volume %s: %s", uuid, err) } - logger.Printf("started volume %s (%s), ReadOnly=%v", uuid, vol, cfgvol.ReadOnly || va.ReadOnly) - sc := cfgvol.StorageClasses if len(sc) == 0 { sc = map[string]bool{"default": true} @@ -330,7 +328,8 @@ func makeRRVolumeManager(logger logrus.FieldLogger, cluster *arvados.Cluster, my KeepMount: arvados.KeepMount{ UUID: uuid, DeviceID: vol.GetDeviceID(), - ReadOnly: cfgvol.ReadOnly || va.ReadOnly, + AllowWrite: !va.ReadOnly && !cfgvol.ReadOnly, + AllowTrash: !va.ReadOnly && (!cfgvol.ReadOnly || cfgvol.AllowTrashWhenReadOnly), Replication: repl, StorageClasses: sc, }, @@ -340,9 +339,10 @@ func makeRRVolumeManager(logger logrus.FieldLogger, cluster *arvados.Cluster, my vm.mounts = append(vm.mounts, mnt) vm.mountMap[uuid] = mnt vm.readables = append(vm.readables, mnt) - if !mnt.KeepMount.ReadOnly { + if mnt.KeepMount.AllowWrite { vm.writables = append(vm.writables, mnt) } + logger.Printf("started volume %s (%s), AllowWrite=%v, AllowTrash=%v", uuid, vol, mnt.AllowWrite, mnt.AllowTrash) } // pri(mnt): return highest priority of any storage class // offered by mnt @@ -382,7 +382,7 @@ func (vm *RRVolumeManager) Mounts() []*VolumeMount { } func (vm *RRVolumeManager) Lookup(uuid string, needWrite bool) *VolumeMount { - if mnt, ok := vm.mountMap[uuid]; ok && (!needWrite || !mnt.ReadOnly) { + if mnt, ok := vm.mountMap[uuid]; ok && (!needWrite || mnt.AllowWrite) { return mnt } return nil -- 2.30.2