Add "bad" testcases for policer #656

New issue

Closed

opened 2023-08-28 14:20:47 +00:00 by fyrchik · 2 comments

fyrchik commented

2023-08-28 14:20:47 +00:00

Owner

Container is not found, but so is it's deletion info.
Missing from #640

Container is not found, but so is it's deletion info. Missing from #640

👍 1

fyrchik added the

frostfs-node

good first issue

labels 2023-08-28 14:21:03 +00:00

fyrchik added this to the v0.37.0 milestone 2023-08-28 14:25:14 +00:00

aarifullin commented

2023-08-28 14:37:17 +00:00

Member

The issue appeared from the discussion.

Unit tests for the policer lack a unit-test where the such scenario is checked:

The policer gets an address (cid + oid)
The container has not been found
But the container never existed from the point of the node where the policer works (WasRemoved = false)

So, we need something like this test but with this setup

containerSrc := containerSrc{
		get: func(id cid.ID) (*container.Container, error) {
			return nil, new(apistatus.ContainerNotFound)
		},
		deletionInfo: func(id cid.ID) (*container.DelInfo, error) {
			return nil, new(apistatus.ContainerNotFound) // never existed (has not created yet at the current epoch)
		},
	}

If you look at the control paths

...
            existed, err := containercore.WasRemoved(p.cnrSrc, idCnr)
			if err != nil {
				p.log.Error(logs.PolicerCouldNotConfirmContainerRemoval,
					zap.Stringer("cid", idCnr),
					zap.Stringer("oid", idObj),
					zap.String("error", err.Error()))
			} else if existed {
				err := p.buryFn(ctx, addrWithType.Address)
				if err != nil {
					p.log.Error(logs.PolicerCouldNotInhumeObjectWithMissingContainer,
						zap.Stringer("cid", idCnr),
						zap.Stringer("oid", idObj),
						zap.String("error", err.Error()))
				}
			}
...

then you can see that here is a problem how to notify the unit-test that nothing got to bury channel. So, some idea can be used to write the unit-test.

Simple way - add

if err != nil {
} else if existed {
} else {
   p.log.Debug("I am sorry. Nothing to bury. Bye!")
}

and itercept this message in the unit-test

The issue appeared from the [discussion](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/640#issuecomment-20317). [Unit tests](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/policer/policer_test.go) for the policer lack a unit-test where the such scenario is checked: 1. The policer [gets](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/policer/check.go#L19) an address (cid + oid) 2. The container has not been [found](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/policer/check.go#L30) 3. But the container never [existed](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/policer/check.go#L31) from the point of the node where the policer works (`WasRemoved = false`) So, we need something like this [test](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/policer/policer_test.go#L25) but with this setup ```golang containerSrc := containerSrc{ get: func(id cid.ID) (*container.Container, error) { return nil, new(apistatus.ContainerNotFound) }, deletionInfo: func(id cid.ID) (*container.DelInfo, error) { return nil, new(apistatus.ContainerNotFound) // never existed (has not created yet at the current epoch) }, } ``` If you look at the control paths ```golang ... existed, err := containercore.WasRemoved(p.cnrSrc, idCnr) if err != nil { p.log.Error(logs.PolicerCouldNotConfirmContainerRemoval, zap.Stringer("cid", idCnr), zap.Stringer("oid", idObj), zap.String("error", err.Error())) } else if existed { err := p.buryFn(ctx, addrWithType.Address) if err != nil { p.log.Error(logs.PolicerCouldNotInhumeObjectWithMissingContainer, zap.Stringer("cid", idCnr), zap.Stringer("oid", idObj), zap.String("error", err.Error())) } } ... ``` then you can see that here is a problem how to notify the unit-test that nothing got to bury channel. So, some idea can be used to write the unit-test. Simple way - add ```golang if err != nil { } else if existed { } else { p.log.Debug("I am sorry. Nothing to bury. Bye!") } ``` and itercept this message in the unit-test

fyrchik commented

2023-08-28 15:42:57 +00:00

Author

Owner

We can either wait on channel for some time.
Or we can wait until 2 policer cycles complete (if the channel is empty we can be sure nothing was buried in a single cycle. This approach has a problem of becoming invalid in case policer implementation changes.
Another option is to use testhook. I don't like them, but here it seems suitable. Hell, even gostdlib uses them 7e7dd4dcd9/src/net/tcpsock_posix.go (L58)

1. We can either wait on channel for some time. 2. Or we can wait until 2 policer cycles complete (if the channel is empty we can be sure nothing was buried in a single cycle. This approach has a problem of becoming invalid in case policer implementation changes. 3. Another option is to use testhook. I don't like them, but here it seems suitable. Hell, even gostdlib uses them https://github.com/golang/go/blob/7e7dd4dcd9a73ed9e832cd873b8fad7b87ddf029/src/net/tcpsock_posix.go#L58

👍 1