validation: Implement ConfigUpdatesWithoutAffect

[zhouhao3]

Signed-off-by: Zhou Hao [email protected]

[liangchenye]

It proves that the process change after the create operation will not have an effect on the container.
It is a part of 'any change', so this PR looks good to me.

But it does not prove other property changes, like 'env', 'mount' and the etc.
I think it is not ready to mark this ConfigUpdatesWithoutAffect spec item to 'DONE'.

To prove 'any change...', how about this:

1. create a config.json (configA) with 'runtimetest' as the process
2. save configA to a different dir for runtimetest to read
3. create a container
4. create another config.json(configB) and make any changes (except process)
5. start the container
   ---- if runtimetest detects any property is different to configA, there will be
   two possibilities:
   5.1 runtime cannot apply properties correctly --- if we verify all the properties, it will not happen.
   5.2 runtime updates to new config after 'create' --- this is what we need

[wking]

create another config.json(configB) and make any changes (except process)

Why not change process too? The spec has no such qualification.

I like @liangchenye's general approach though. Can we adjust our test helper to always clobber config.json with a dummy config (maybe this one?) immediately after a successful create?

[liangchenye]

Why not change process too? The spec has no such qualification.

The process is already verified in this commit :)

[wking]

The process is already verified in this commit :)

Do we want to keep that? poststop.go seems like a strange place for that check. If we always clobber via our test helpers, almost all our tests would enforce this requirement.

[liangchenye]

You are right. When we use https://github.com/opencontainers/runtime-spec/blob/v1.0.1/schema/test/config/good/minimal.json, if it fails to start, it means the 'process' is updated; if it succeeds to start but fails in checking any properties, it means the 'other properties' are updated(assuming those properties are already verified).

[zhouhao3]

It looks like I need to write a new file for this test.

[zhouhao3]

@liangchenye @wking updated, PTAL.

[liangchenye]

Any updates to config.json after this step MUST NOT affect the container.

inside test

For example, if we want to test if hostname updates will affect the container, we need to

1. set the config.json (A) with hostname == good and with process == runtimetest
2. create a container
3. set the config.json (B) with hostname == bad
4. start the container
   the container process (runtimetest) will verify the hostname in validateHostname.
   if os.Hostname() is equal to good, it means the hostname updates DOES NOT affect the container.
   Or we can output a spec error.

Now bundledir/config.json is both config.json for the runtime and the expected properties for runtimetest, we will lose the 'expected properties' if we doing the 3rd step. We can save a copy of (A) to bundledir/test/config.json (C) and tell 'runtimetest' to check with this file.
If runtimetest does not detect a hostname difference, it means the update of 'hostname' DOES not affect the container.

It is the same approach when we check other updates.
A lazy way is we can do it in RuntimeInsideValidate.

[zhouhao3]

@liangchenye @wking I updated this pr, and I added a runtimetest to test the configA in ProcessArgs.

But when I test it, it's going to quit when it's doing r.Create(), I'm not sure why.

TAP version 13
exit status 1

[liangchenye]

@q384566678 I'm debugging it now.

[liangchenye]

Seems you forget to copy 'runtimetest' to the bundle.

[zhouhao3]

I try to put this test in RuntimeInsideValidate, but I found a problem, other tests are executed in call RuntimeInsideValidate, there is no guarantee that the config is correct, so when the start failure, cannot confirm is affected by the later config, was originally the config is wrong.
So I think it's better to put this test alone.
@liangchenye @wking PTAL.

[liangchenye]

The test directory is not created, maybe we can just use

testPath := filepath.Join(bundleDir, "test.json")

[liangchenye]

It is already inside the bundle, so we can just use "--path=/test.json" or "--path=/test/config.json" .

[liangchenye]

The current implementation (commit#4cf14f6) is clearer to figure out the real problem of a runtime.
I think if we can guarantee the original config (configA) has all the testing properties, commit#4cf14f6 for the inside test will be sufficient.

[zhouhao3]

updated, I added properties to config.json.

[zhouhao3]

@liangchenye PTAL

[wking]

This rooted path fails when you use it for g.SaveToFile(…) below:

$ ./validation/config_updates_without_affect.t 
TAP version 13
open /test/config.json: no such file or directory

I agree with @liangchenye that you should set this path to something inside the bundle, at least for the host-mount-namespace SaveToFile call.

[zhouhao3]

updated, thanks.

[liangchenye]

It works in my test and according to previous discussion, I'll merge it.
LGTM