Skip to content

rg spawn-resources has no guard against a concurrent rg delete, and can leave a definition in a deleted group #196

Description

@kvaps

rg spawn-resources can leave a resource definition parented to a group that no longer exists. rd create has a guard for this race, spawn has none.

If rg d runs while a spawn sits between reading the group and creating the definition, the delete's re-walk (rollbackRGDeleteIfRaced via countChildRDs) reads definitions from the informer cache, which hasn't seen the new one yet, so the delete goes through. Then the spawn creates the definition and answers 201. rd create closes the same window with refuseRDCreateOnRGDeletedRace, a re-read of the group after the write that rolls the definition back. spawnCreate has nothing like it, and neither half of the guard #193 adds for clone and restore.

The placer then drops the group tier of that definition silently, and this is the path linstor-csi takes for every plain CreateVolume.

Repro on main (d2c6112d2). The store double holds the definition create until released and hides the new definition from List, like a cache that hasn't caught up yet:

$ go test ./pkg/rest/ -run 'TestProbeSpawnDuringRGDelete|TestProbeControlRDCreateDuringRGDelete' -count=1 -v
    spawn: rg d while the definition create is in flight -> 200
    spawn: create answered 201
    spawn: definition rd-race exists, parented to "grp-race"; group lookup: resource group "grp-race": object not found
    rd create: rg d while the definition create is in flight -> 200
    rd create: create answered 404
    rd create: definition rd-race absent (resource definition "rd-race": object not found)
probe test
// gatedRDs holds a definition create until released and keeps it out of List,
// the way an informer cache that has not seen the create yet does.
type gatedRDs struct {
	store.ResourceDefinitionStore

	target  string
	reached chan struct{}
	release chan struct{}
	once    *sync.Once
}

func (g gatedRDs) Create(ctx context.Context, rd *apiv1.ResourceDefinition) error {
	if rd.Name == g.target {
		g.once.Do(func() { close(g.reached) })
		<-g.release
	}

	return g.ResourceDefinitionStore.Create(ctx, rd) //nolint:wrapcheck // probe
}

func (g gatedRDs) List(ctx context.Context) ([]apiv1.ResourceDefinition, error) {
	all, err := g.ResourceDefinitionStore.List(ctx)

	out := all[:0]
	for i := range all {
		if all[i].Name != g.target {
			out = append(out, all[i])
		}
	}

	return out, err //nolint:wrapcheck // probe
}

type gatedStore struct {
	store.Store

	rds gatedRDs
}

func (g gatedStore) ResourceDefinitions() store.ResourceDefinitionStore {
	g.rds.ResourceDefinitionStore = g.Store.ResourceDefinitions()

	return g.rds
}

func probeRGDeleteDuring(t *testing.T, door string, post func(base string) int) {
	t.Helper()

	backend := store.NewInMemory()
	ctx := t.Context()

	if err := backend.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{Name: "grp-race"}); err != nil {
		t.Fatalf("seed group: %v", err)
	}

	gate := gatedRDs{target: "rd-race", reached: make(chan struct{}), release: make(chan struct{}), once: &sync.Once{}}

	base, stop := startServerWithStore(t, gatedStore{Store: backend, rds: gate})
	defer stop()

	code := make(chan int, 1)

	go func() { code <- post(base) }()

	<-gate.reached

	resp := httpDelete(t, base+"/v1/resource-groups/grp-race")
	_ = resp.Body.Close()
	t.Logf("%s: rg d while the definition create is in flight -> %d", door, resp.StatusCode)

	close(gate.release)

	t.Logf("%s: create answered %d", door, <-code)

	rd, err := backend.ResourceDefinitions().Get(ctx, "rd-race")
	_, rgErr := backend.ResourceGroups().Get(ctx, "grp-race")

	if err == nil {
		t.Logf("%s: definition rd-race exists, parented to %q; group lookup: %v", door, rd.ResourceGroupName, rgErr)
	} else {
		t.Logf("%s: definition rd-race absent (%v)", door, err)
	}
}

func TestProbeSpawnDuringRGDelete(t *testing.T) {
	probeRGDeleteDuring(t, "spawn", func(base string) int {
		body, _ := json.Marshal(apiv1.ResourceGroupSpawn{
			ResourceDefinitionName: "rd-race", VolumeSizes: []int64{64 * 1024}, DefinitionsOnly: true,
		})
		resp := httpPost(t, base+"/v1/resource-groups/grp-race/spawn", body)
		_ = resp.Body.Close()

		return resp.StatusCode
	})
}

func TestProbeControlRDCreateDuringRGDelete(t *testing.T) {
	probeRGDeleteDuring(t, "rd create", func(base string) int {
		body, _ := json.Marshal(apiv1.ResourceDefinitionCreate{
			ResourceDefinition: apiv1.ResourceDefinition{Name: "rd-race", ResourceGroupName: "grp-race"},
		})
		resp := httpPost(t, base+"/v1/resource-definitions", body)
		_ = resp.Body.Close()

		return resp.StatusCode
	})
}

I think the fix is the same re-check in spawnCreate right after the definition is created, rolling back through rollbackSpawn on NotFound like rd create does. I kept it out of #193 on purpose: that PR is about clone and restore, and spawn compensates differently.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions