Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set containerd config on nodeup.Config instead of clusterspec #11750

Merged
merged 1 commit into from
Jun 15, 2021
Merged

Set containerd config on nodeup.Config instead of clusterspec #11750

merged 1 commit into from
Jun 15, 2021

Conversation

olemarkus
Copy link
Member

This allows us to set a default containerd config per IG (e.g add a different config for GPU IGs)

Can also be considered a cleanup as we no longer use containerd.overrideConfig as a mechanism for bringing the default containerd config from cloudup to nodeup.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 13, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2021
@olemarkus olemarkus changed the title Set containerd config on nodeup.Config instead of clusterspec Set containerd config on nodeup.AuxConfig instead of clusterspec Jun 13, 2021
@johngmyers
Copy link
Member

Are these so big they can't fit in userdata? I'm trying to keep non-empty AuxConfig to the uncommon case if possible.

@olemarkus
Copy link
Member Author

It's a good question. toml format is quite verbose. The default config is 500bytes, with GPU config is twice as big. Then people can add x number of registry mirror configurations, that is around 300 bytes each.

Since it to some extent can be arbitrary bytes long, I thought it was a good candidate.

@hakman
Copy link
Member

hakman commented Jun 13, 2021

We still have compression of user data. Maybe wait a little more with this?

@olemarkus
Copy link
Member Author

If we believe the config to be small enough, changing to nodeup.Config (which the former version of this PR used) is trivial. I am happy with either.

@johngmyers
Copy link
Member

Let's go with nodeup.Config for now. I'd like to keep as few things in AuxConfig as possible as a likely future refactor would merge AuxConfig into Config and spill Config to VFS when it gets too big.

@johngmyers
Copy link
Member

FYI, I'd like to move the docker config into nodeup.Config as well. Doesn't have to be this PR.

@olemarkus olemarkus force-pushed the containerd-per-ig branch 2 times, most recently from eda157f to 1eafd8b Compare June 13, 2021 20:14
@olemarkus olemarkus changed the title Set containerd config on nodeup.AuxConfig instead of clusterspec Set containerd config on nodeup.Config instead of clusterspec Jun 13, 2021
hakman
hakman requested changes Jun 14, 2021
View reviewed changes
pkg/apis/nodeup/config.go Outdated
@@ -73,6 +73,9 @@ type Config struct {
ConfigServer *ConfigServerOptions `json:"configServer,omitempty"`
// AuxConfigHash holds a secure hash of the nodeup.AuxConfig.
AuxConfigHash string

// Containerd config holds the configuration for containerd
Containerd *ContainerdConfig `json:"containerd,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be extended later if there's any need for it.

Suggested change
Containerd *ContainerdConfig `json:"containerd,omitempty"`
ContainerdConfig string `json:",omitempty"`

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config is unversioned. I don't think we can extend it in the future as that would break nodeup/cloudup marshaling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's just part of the launch template, which is updated together with new version of nodeup?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand @johngmyers correctly, there is an idea about writing this to VFS at some point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an alternative for large user data cases, if I understand correctly.
For containerd there are no flags to control various aspects, everything can be done through the config file, which will be generated in cloudup. So, I don't see any need for extending this short term.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that it's unversioned isn't a problem because the reader (nodeup) always matches the version of the writer (kops). So we can make incompatible changes with impunity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking you'd run into the autoscaling edge case, but you are right. This doesn't apply here.

I'll amend then.

@olemarkus olemarkus force-pushed the containerd-per-ig branch 3 times, most recently from f724687 to 3f8020a Compare June 15, 2021 07:52
hakman
hakman requested changes Jun 15, 2021
View reviewed changes
upup/pkg/fi/cloudup/apply_cluster.go Outdated Show resolved Hide resolved
upup/pkg/fi/cloudup/apply_cluster.go Outdated Show resolved Hide resolved
upup/pkg/fi/cloudup/apply_cluster.go Show resolved Hide resolved
@olemarkus olemarkus force-pushed the containerd-per-ig branch 3 times, most recently from 65820fb to 23ba363 Compare June 15, 2021 08:54
Set containerd config on nodeup.Config instead of clusterspec
e7fa3fa 
This allows us to set a default containerd config per IG (e.g add a different config for GPU IGs)

Can also be considered a cleanup as we no longer use containerd.overrideConfig as a mechanism for bringing the default containerd config from cloudup to nodeup.
hakman
hakman reviewed Jun 15, 2021
View reviewed changes
upup/pkg/fi/cloudup/apply_cluster.go
Comment on lines +1388 to +1391
if cluster.Spec.Containerd == nil {
cluster.Spec.Containerd = &kops.ContainerdConfig{}
}
config.ContainerdConfig = buildContainerdConfig(cluster)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last suggestion. This only makes sense to be done if container runtime is containerd.
/lgtm

Suggested change
if cluster.Spec.Containerd == nil {
cluster.Spec.Containerd = &kops.ContainerdConfig{}
}
config.ContainerdConfig = buildContainerdConfig(cluster)
if cluster.ContainerRuntime == "containerd" {
if cluster.Spec.Containerd == nil {
cluster.Spec.Containerd = &kops.ContainerdConfig{}
}
config.ContainerdConfig = buildContainerdConfig(cluster)
}

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2021
@k8s-ci-robot k8s-ci-robot merged commit 847040d into kubernetes:master Jun 15, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hakman hakman hakman requested changes

@johngmyers johngmyers johngmyers approved these changes

@joshbranham joshbranham Awaiting requested review from joshbranham

@mikesplain mikesplain Awaiting requested review from mikesplain

Assignees

@johngmyers johngmyers

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/nodeup cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@olemarkus @johngmyers @hakman @k8s-ci-robot