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

Complete cri grpc plugin config migration #11061

Merged
merged 2 commits into from
Dec 11, 2024
Merged

Complete cri grpc plugin config migration #11061

merged 2 commits into from
Dec 11, 2024

Conversation

djdongjin
Copy link
Member

It seems CRI GRPC plugin config cannot be migrated (other than disable_tcp_service). This PR make sure all fields in v3 plugins."io.containerd.grpc.v1.cri" are migrated from v2 plugins."io.containerd.grpc.v1.cri".

Running sudo ./bin/containerd -c test.toml config migrate on this v2 cri grpc config:

version = 2

[plugins]
[plugins."io.containerd.grpc.v1.cri"]
disable_tcp_service = true
stream_server_address = '127.1.1.1'
stream_server_port = '15000'
stream_idle_timeout = '5h0m0s'
enable_tls_streaming = true

[plugins.'io.containerd.grpc.v1.cri'.x509_key_pair_streaming]
tls_cert_file = '/path/to/cert.pem'
tls_key_file = '/path/to/key.pem'

The migrated plugins."io.containerd.grpc.v1.cri" in main branch (only default values):

  [plugins.'io.containerd.grpc.v1.cri']
    disable_tcp_service = true
    stream_server_address = '127.0.0.1'
    stream_server_port = '0'
    stream_idle_timeout = '4h0m0s'
    enable_tls_streaming = false

    [plugins.'io.containerd.grpc.v1.cri'.x509_key_pair_streaming]
      tls_cert_file = ''
      tls_key_file = ''

After this change:

  [plugins.'io.containerd.grpc.v1.cri']
    disable_tcp_service = true
    stream_server_address = '127.1.1.1'
    stream_server_port = '15000'
    stream_idle_timeout = '5h0m0s'
    enable_tls_streaming = true

    [plugins.'io.containerd.grpc.v1.cri'.x509_key_pair_streaming]
      tls_cert_file = '/path/to/cert.pem'
      tls_key_file = '/path/to/key.pem'

@dosubot dosubot bot added the area/cri Container Runtime Interface (CRI) label Nov 26, 2024
djdongjin
djdongjin commented Nov 26, 2024
View reviewed changes
plugins/cri/cri.go

// Currently only a single key migrated
Copy link
Member Author

Choose a reason for hiding this comment

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

// Currently only a single key migrated

@dmcgowan I wonder if this is intended or not. Appreciate any pointer if there are other changes required to migrate other keys.

Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to follow the history on this one, it was moved before the migration so it might have just been missed 65b3922

I'm wondering if this copy approach is the best, it seems just to be stripping out the other values but that shouldn't be necessary. Ideally when values are migrated the migration cleans up the value in the original struct, so if these values aren't moving, they shouldn't need to be copied.

Copy link
Member Author

@djdongjin djdongjin Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally when values are migrated the migration cleans up the value in the original struct

That means a field can only be used by 1 plugin at a time. If a field (say config.containerd.runtime) is used by 2 independent plugins (e.g., cri-runtime, cri-image) during migration, neither of them can clean up the value in the original struct (cri-grpc).

Then when we map the original struct to the new cri-grpc ServerConfig struct, it will print some confusing warnings (unknown key in TOML) #10959, due to no corresponding fields in golang struct.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you are right, the cri config is pretty messy and migration isn't that straight forward. Always overwriting like you are doing here is fine

@djdongjin
Copy link
Member Author

/test pull-containerd-k8s-e2e-ec2

@dmcgowan dmcgowan added the cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch label Nov 27, 2024
@dmcgowan
Copy link
Member

We should get this into the first patch release for 2.0

It is worth looking into whether we should have a non default config migration test, seems this is missed is the migration test since we only test the default case. We might be missing cases where a value gets overrode with a default.

@djdongjin
Copy link
Member Author

djdongjin commented Nov 27, 2024 •
edited
Loading

We should get this into the first patch release for 2.0

SG, will create a backport.

It is worth looking into whether we should have a non default config migration test, seems this is missed is the migration test since we only test the default case. We might be missing cases where a value gets overrode with a default.

Agreed, since with default only it's hard to tell if a value comes from migration or just the default in new config. I will create a migration integration test for non-default configs.

@djdongjin djdongjin force-pushed the config-migrate-cri-grpc branch from 5238b76 to b16d13d Compare November 27, 2024 20:59
@djdongjin
Copy link
Member Author

added UT for cri grpc config migration

@djdongjin djdongjin force-pushed the config-migrate-cri-grpc branch from b16d13d to 8540fed Compare November 27, 2024 21:06
@dmcgowan dmcgowan force-pushed the config-migrate-cri-grpc branch from 7661fe1 to ed39dfa Compare November 28, 2024 00:03
@dmcgowan
Copy link
Member

Added an integration test to show it fixes the migration case against a custom config

@fuweid fuweid added this pull request to the merge queue Dec 11, 2024
Merged via the queue into containerd:main with commit e21c1a1 Dec 11, 2024
59 checks passed
@djdongjin
Copy link
Member Author

/cherrypick release/2.0

@k8s-infra-cherrypick-robot
Copy link

@djdongjin: new pull request created: #11140

In response to this:

/cherrypick release/2.0

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Dec 11, 2024
Merged
@djdongjin djdongjin mentioned this pull request Dec 11, 2024
Merged
@dmcgowan dmcgowan added cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch and removed cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch labels Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@fuweid fuweid fuweid approved these changes

Assignees
No one assigned
Labels
area/cri Container Runtime Interface (CRI) cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch size/L
Projects
Pull Request Review
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@djdongjin @dmcgowan @k8s-infra-cherrypick-robot @fuweid @k8s-ci-robot