docs: Improve top-level README #3558

jodh-intel · 2022-01-26T10:46:16Z

Various improvements to the top-level README file:

Moved the following sections from the runtime's README to the
top-level README:
- License
- Platform support / Hardware requirements
Added the following sections to the top-level README:
- Configuration
- Hypervisors
Improved formatting of the Documentation section in the top-level
README.
Removed some unused named links from the top-level README.

Fixes: #3557.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

README.md

jodh-intel · 2022-01-26T10:49:17Z

README.md

+## Configuration
+
+See the [runtime configuration details](src/runtime/README.md#configuration) and
+the [stateless systems](src/runtime/README.md#stateless-systems)
+documentation.


I'm undecided whether this is the best approach (reference the config details in the runtime's README) or whether we should just move those details to the top-level README since the config file doesn't just apply to the runtime. Thoughts folks?

The config file doesn't just apply to the runtime, but doesn't everything that we describe in that README only apply to the runtime?

@Jakob-Naucke - I'm not totally clear on what you're saying here. The license and platform support applies to the whole project so that's why I've moved it to the top-level. The config is a bit more debatable since although it's technically for the runtime (it's read by the runtime), it also affects other parts of the system (such as the agent). Are you saying we should move that config detail into the top-level too? That would make the information more visible but I left it in the runtime README since if we moved the full details to the top-level README, it felt a bit odd for the runtime's README to refer to another file for it's configuration. It's debatable though ;)

Sorry, I'm too brief lately :) I meant: While the Kata Containers config does not only affect the runtime, AFAICT, everything we describe in https://github.com/kata-containers/kata-containers/tree/main/src/runtime#configuration does only affect the runtime. Thus, right now, I don't see a reason to move it up anyway.

Ack. I've updated the Configuration for both README's to give a bit of extra information to users.

jodh-intel · 2022-01-26T11:42:28Z

Ugh. The static checker is failing to validate a correct URL:

[static-checks.sh:584] ERROR: found HTTP error status codes for URL https://www.amd.com (403)
ERROR: Invalid URL 'https://www.amd.com' found in the following files:

README.md

The problem appears to be that the site in question is filtering on the user agent. Our CI checker doesn't set one, hence the HTTP 403 ("forbidden") error message.

For now, I've just removed that link since currently there is no good way to fix the static checker's URL check in a generic way.

fidencio · 2022-01-26T12:36:07Z

A head's up that may affect your PR.

In the Architecture Committee meeting from January 25th, 2022, the Architecture Committee has agreed on using the "Dismiss stale pull request approvals when new commits are pushed" configuration from GitHub. It basically means that if your PR has been rebased or updated, the approvals given will be erased.

In order to minimize traumas due to the new approach, please, consider adding a note on the changes done before the rebase / force-push, and also pinging the reviewers for a subsequent round of reviews.

Thanks for your understanding!

Related issue: kata-containers/community#249

README.md

jodh-intel · 2022-02-01T08:52:32Z

Ping for reviews y'all!

Jakob-Naucke

Thank you @jodh-intel, also for the spelling updates. I have two other minor comments...

README.md

Jakob-Naucke · 2022-02-01T10:14:48Z

src/runtime/README.md

+> - The runtime will read the configuration file from one of [two
+>   possible locations](#stateless-systems).


What about the /opt/kata tree?

Yeah, there's a bit more detail here, indeed. it'd be more accurate to say what it will do by default.

If we want to provide more detail, we can find a nice way to describe what actually happens, hopefully more succinctly than I do in my blather that follows:

It'll first check if a config path is specified in annotations associated with the pause container (yuck - I hope no one ever does this), and then it'll check for a path specified within the CreateTaskRequest (this is what is done in the case of kata-deploy install, since we look at /opt/kata...), and then finally check the two default locations on the host filesystem, with /etc/kata-containers taking precedence over /usr/share/defaults/kata-containers....).

See:

kata-containers/src/runtime/pkg/containerd-shim-v2/create.go

Lines 212 to 256 in 88a70d3

func loadRuntimeConfig(s *service, r *taskAPI.CreateTaskRequest, anno map[string]string) (*oci.RuntimeConfig, error) {

if s.config != nil {

return s.config, nil

}

configPath := oci.GetSandboxConfigPath(anno)

if configPath == "" && r.Options != nil {

v, err := typeurl.UnmarshalAny(r.Options)

if err != nil {

return nil, err

}

option, ok := v.(*crioption.Options)

// cri default runtime handler will pass a linux runc options,

// and we'll ignore it.

if ok {

configPath = option.ConfigPath

} else {

// Some versions of containerd, such as 1.4.3, and 1.4.4

// still rely on the runtime options coming from

// github.com/containerd/cri-containerd/pkg/api/runtimeoptions/v1

// Knowing that, instead of breaking compatibility with such

// versions, let's work this around on our side

oldOption, ok := v.(*oldcrioption.Options)

if ok {

configPath = oldOption.ConfigPath

}

}

}

// Try to get the config file from the env KATA_CONF_FILE

if configPath == "" {

configPath = os.Getenv("KATA_CONF_FILE")

}

_, runtimeConfig, err := katautils.LoadConfiguration(configPath, false)

if err != nil {

return nil, err

}

// For the unit test, the config will be predefined

if s.config == nil {

s.config = &runtimeConfig

}

return &runtimeConfig, nil

}

fidencio

lgtm, thanks @jodh-intel!

fidencio · 2022-02-01T10:21:12Z

README.md

+### Hardware requirements
+
+The [Kata Containers runtime](src/runtime) provides a command to
+determine if your host system is capable of running and creating a


I, personally, think that kata-runtime check is not the best way to check whether the host system is capable of running and creating a Kata Container.

As we're 100% Kubernetes focused nowadays, I'd rather see us working on and recommending NFD instead.

But hey, I guess this is the best we have now.

I find the tool useful as a starting point, however -- most people will come here to test in their VM environment, and this helps. I do wonder if we need /want this here versus user/developer guide?

fidencio · 2022-02-02T12:39:46Z

/test

fidencio · 2022-02-02T14:05:55Z

/retest

egernst

Nice improvements! I left a bit of feedback, mostly just agreeing with Jakob :)

README.md

egernst · 2022-02-04T00:01:58Z

README.md

+### Hardware requirements
+
+The [Kata Containers runtime](src/runtime) provides a command to
+determine if your host system is capable of running and creating a


I find the tool useful as a starting point, however -- most people will come here to test in their VM environment, and this helps. I do wonder if we need /want this here versus user/developer guide?

egernst · 2022-02-04T00:14:49Z

src/runtime/README.md

+> - The runtime will read the configuration file from one of [two
+>   possible locations](#stateless-systems).


Yeah, there's a bit more detail here, indeed. it'd be more accurate to say what it will do by default.

If we want to provide more detail, we can find a nice way to describe what actually happens, hopefully more succinctly than I do in my blather that follows:

It'll first check if a config path is specified in annotations associated with the pause container (yuck - I hope no one ever does this), and then it'll check for a path specified within the CreateTaskRequest (this is what is done in the case of kata-deploy install, since we look at /opt/kata...), and then finally check the two default locations on the host filesystem, with /etc/kata-containers taking precedence over /usr/share/defaults/kata-containers....).

See:

kata-containers/src/runtime/pkg/containerd-shim-v2/create.go

Lines 212 to 256 in 88a70d3

func loadRuntimeConfig(s *service, r *taskAPI.CreateTaskRequest, anno map[string]string) (*oci.RuntimeConfig, error) {

if s.config != nil {

return s.config, nil

}

configPath := oci.GetSandboxConfigPath(anno)

if configPath == "" && r.Options != nil {

v, err := typeurl.UnmarshalAny(r.Options)

if err != nil {

return nil, err

}

option, ok := v.(*crioption.Options)

// cri default runtime handler will pass a linux runc options,

// and we'll ignore it.

if ok {

configPath = option.ConfigPath

} else {

// Some versions of containerd, such as 1.4.3, and 1.4.4

// still rely on the runtime options coming from

// github.com/containerd/cri-containerd/pkg/api/runtimeoptions/v1

// Knowing that, instead of breaking compatibility with such

// versions, let's work this around on our side

oldOption, ok := v.(*oldcrioption.Options)

if ok {

configPath = oldOption.ConfigPath

}

}

}

// Try to get the config file from the env KATA_CONF_FILE

if configPath == "" {

configPath = os.Getenv("KATA_CONF_FILE")

}

_, runtimeConfig, err := katautils.LoadConfiguration(configPath, false)

if err != nil {

return nil, err

}

// For the unit test, the config will be predefined

if s.config == nil {

s.config = &runtimeConfig

}

return &runtimeConfig, nil

}

egernst

Good improvements, just some additional feedback.

because I didn't mean to approve :D

jodh-intel · 2022-02-14T15:33:02Z

@egernst, @Jakob-Naucke - ptal. The configuration is more confusing then even Eric listed. Hopefully, the verbiage on this PR will make things clearer 😄

Jakob-Naucke

Thanks @jodh-intel -- I have some comments again

src/runtime/README.md

jodh-intel · 2022-02-15T09:58:33Z

/test

jodh-intel · 2022-02-15T10:38:34Z

Ping for re-review folks.

src/runtime/README.md

Various improvements to the top-level README file: - Moved the following sections from the runtime's README to the top-level README: - License - Platform support / Hardware requirements - Added the following sections to the top-level README: - Configuration - Hypervisors - Improved formatting of the Documentation section in the top-level README. - Removed some unused named links from the top-level README. Also improvements to the runtime README: - Removed confusing mention of the old 1.x runtime name. - Clarify the binary name for the 2.x runtime and the utility program. > **Note:** > > We cannot currently link to the AMD website as that site's > configuration causes the CI static checks to fail. See > kata-containers/tests#4401 Fixes: kata-containers#3557. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel · 2022-02-16T09:54:02Z

@fidencio, @egernst - ptal so we can try to get this landed...

jodh-intel · 2022-02-16T09:54:06Z

/test

fidencio

lgtm, thanks @jodh-intel!

jodh-intel requested a review from a team as a code owner January 26, 2022 10:46

jodh-intel commented Jan 26, 2022

View reviewed changes

jodh-intel added no-backport-needed labels Jan 26, 2022

jodh-intel mentioned this pull request Jan 26, 2022

Set user agent in static checker's URL check kata-containers/tests#4401

Closed

jodh-intel force-pushed the docs-rework-readme branch from 1618cf1 to 1a29550 Compare January 26, 2022 15:04

Jakob-Naucke reviewed Jan 26, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

jodh-intel force-pushed the docs-rework-readme branch from 1a29550 to f0ad203 Compare January 27, 2022 12:19

jodh-intel commented Jan 27, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

jodh-intel commented Jan 27, 2022

View reviewed changes

README.md Show resolved Hide resolved

jodh-intel force-pushed the docs-rework-readme branch 3 times, most recently from 9160dc9 to e779aff Compare February 1, 2022 08:51

Jakob-Naucke reviewed Feb 1, 2022

View reviewed changes

fidencio previously approved these changes Feb 1, 2022

View reviewed changes

egernst previously approved these changes Feb 4, 2022

View reviewed changes

egernst reviewed Feb 4, 2022

View reviewed changes

jodh-intel dismissed fidencio’s stale review via a196c0f February 14, 2022 15:31

jodh-intel force-pushed the docs-rework-readme branch from e779aff to a196c0f Compare February 14, 2022 15:31

Jakob-Naucke reviewed Feb 14, 2022

View reviewed changes

src/runtime/README.md Outdated Show resolved Hide resolved

src/runtime/README.md Outdated Show resolved Hide resolved

src/runtime/README.md Outdated Show resolved Hide resolved

jodh-intel force-pushed the docs-rework-readme branch 2 times, most recently from aa7a1db to 22180a9 Compare February 14, 2022 17:09

Jakob-Naucke reviewed Feb 15, 2022

View reviewed changes

src/runtime/README.md Show resolved Hide resolved

jodh-intel force-pushed the docs-rework-readme branch from 22180a9 to 56461e5 Compare February 16, 2022 09:51

jodh-intel force-pushed the docs-rework-readme branch from 56461e5 to 9818cf7 Compare February 16, 2022 09:52

Jakob-Naucke approved these changes Feb 16, 2022

View reviewed changes

fidencio approved these changes Feb 16, 2022

View reviewed changes

GabyCT approved these changes Feb 16, 2022

View reviewed changes

GabyCT merged commit ced5e91 into kata-containers:main Feb 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Improve top-level README #3558

docs: Improve top-level README #3558

jodh-intel commented Jan 26, 2022

jodh-intel Jan 26, 2022

Jakob-Naucke Jan 26, 2022

jodh-intel Jan 27, 2022

Jakob-Naucke Jan 27, 2022

jodh-intel Jan 27, 2022

jodh-intel commented Jan 26, 2022

fidencio commented Jan 26, 2022

jodh-intel commented Feb 1, 2022

Jakob-Naucke left a comment

Jakob-Naucke Feb 1, 2022

egernst Feb 4, 2022

fidencio left a comment

fidencio Feb 1, 2022

egernst Feb 4, 2022

fidencio commented Feb 2, 2022

fidencio commented Feb 2, 2022

egernst left a comment

egernst Feb 4, 2022

egernst Feb 4, 2022

egernst left a comment

jodh-intel commented Feb 14, 2022

Jakob-Naucke left a comment

jodh-intel commented Feb 15, 2022

jodh-intel commented Feb 15, 2022

jodh-intel commented Feb 16, 2022

jodh-intel commented Feb 16, 2022

fidencio left a comment

		> - The runtime will read the configuration file from one of [two
		> possible locations](#stateless-systems).

	func loadRuntimeConfig(s service, r taskAPI.CreateTaskRequest, anno map[string]string) (*oci.RuntimeConfig, error) {
	if s.config != nil {
	return s.config, nil
	}
	configPath := oci.GetSandboxConfigPath(anno)
	if configPath == "" && r.Options != nil {
	v, err := typeurl.UnmarshalAny(r.Options)
	if err != nil {
	return nil, err
	}
	option, ok := v.(*crioption.Options)
	// cri default runtime handler will pass a linux runc options,
	// and we'll ignore it.
	if ok {
	configPath = option.ConfigPath
	} else {
	// Some versions of containerd, such as 1.4.3, and 1.4.4
	// still rely on the runtime options coming from
	// github.com/containerd/cri-containerd/pkg/api/runtimeoptions/v1
	// Knowing that, instead of breaking compatibility with such
	// versions, let's work this around on our side
	oldOption, ok := v.(*oldcrioption.Options)
	if ok {
	configPath = oldOption.ConfigPath
	}
	}
	}

	// Try to get the config file from the env KATA_CONF_FILE
	if configPath == "" {
	configPath = os.Getenv("KATA_CONF_FILE")
	}

	_, runtimeConfig, err := katautils.LoadConfiguration(configPath, false)
	if err != nil {
	return nil, err
	}

	// For the unit test, the config will be predefined
	if s.config == nil {
	s.config = &runtimeConfig
	}

	return &runtimeConfig, nil
	}

docs: Improve top-level README #3558

docs: Improve top-level README #3558

Conversation

jodh-intel commented Jan 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Jan 26, 2022

fidencio commented Jan 26, 2022

jodh-intel commented Feb 1, 2022

Jakob-Naucke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fidencio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fidencio commented Feb 2, 2022

fidencio commented Feb 2, 2022

egernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egernst left a comment

Choose a reason for hiding this comment

jodh-intel commented Feb 14, 2022

Jakob-Naucke left a comment

Choose a reason for hiding this comment

jodh-intel commented Feb 15, 2022

jodh-intel commented Feb 15, 2022

jodh-intel commented Feb 16, 2022

jodh-intel commented Feb 16, 2022

fidencio left a comment

Choose a reason for hiding this comment