Skip to content
New issue

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

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

Already on GitHub? # to your account

[release/0.8] Revert containerd dep to 1.4.9 #1147

Merged
merged 1 commit into from
Sep 10, 2021

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Sep 3, 2021

Due to some unfortunate deps in the root go.mod in containerd + some k8s staging
directory behavior, containerd 1.5 errors out when trying to get vendored into
kubernetes/kubernetes. Because we depend on ctrd 1.5, if hcsshim tries to
get bumped it will ask that we also bump their dep of containerd to 1.5
and things will fail also. This reverts our containerd dep for the release/0.8
branch to 1.4 so we're able to circumvent this, at least temporarily.

Issue tracking this problem that contains k8s proposed solution and statements on
this: kubernetes/kubernetes#104827

"Components that have a transitive dependency to anything in k8s.io/kubernetes,
including the staging components, can not be added as a dependency. The
detection and prevention of this in the dependency management scripts is
intentional."

While this work is temporary just to get k8s a new hcsshim release, longer
term we will need to find a way to trim our containerd dep either here, or
containerd will need to get rid of it's k8s.io/* dependencies in their root go.mod
to remove this circular import issue.

The tests ran to make sure nothing broke with this

-All of the tests that currently get ran on a K8s pull request which includes
the AKS engine e2e containerd test suite.
-Containerd integration tests
-Our cri-containerd tests with a shim built from this commit

Signed-off-by: Daniel Canter dcanter@microsoft.com

@dcantah dcantah requested a review from a team as a code owner September 3, 2021 21:52
@kevpar
Copy link
Member

kevpar commented Sep 3, 2021

It would be helpful to link any issues tracking the overall fix for this problem, as well as any test pass results for the testing you did on this.

@dcantah
Copy link
Contributor Author

dcantah commented Sep 3, 2021

Don't have any tests to link as I ran them locally. Let me see if the e2e tests that Mark had ran have a link or log file. I can add some issues though, not sure if there's one central "Fixing X". They're kind of scattered around on others discovering the same problem

@kevpar
Copy link
Member

kevpar commented Sep 3, 2021

We think this is something that needs to be resolved centrally in kubernetes, right? So we should try to make sure there is a single primary tracking issue from that perspective.

@dcantah
Copy link
Contributor Author

dcantah commented Sep 3, 2021

I've made an issue on our end to look into getting rid of the ctrd dep in our root go.mod, but yes. We were going to reach out to some folks to inquire on this. Want some people more familiar with the background of why the staging directory is the way it is to open an issue, incase this is just par for the course ( I hope not..). Will do on Tuesday.

@dcantah
Copy link
Contributor Author

dcantah commented Sep 8, 2021

@kevpar Anything else needed to get this in? The advice we've been given is just to trim deps in projects that need to get vendored into k8s/k8s by the way :/ #1148 (comment)

@kevpar
Copy link
Member

kevpar commented Sep 8, 2021

@kevpar Anything else needed to get this in? The advice we've been given is just to trim deps in projects that need to get vendored into k8s/k8s by the way :/ #1148 (comment)

I'd like to have the commit message updated with the issue link and latest context/plan (k8s does not allow circular dependencies, so this is a temporary fix, and long term we need to remove the dependency loop in some way).

@dcantah
Copy link
Contributor Author

dcantah commented Sep 8, 2021

@kevpar Sure, will update. Gonna do another pseudo-vendor run again to make sure nothing changed in the last week in k8s also.

@dcantah
Copy link
Contributor Author

dcantah commented Sep 9, 2021

@kevpar Updated the commit message, ptal (also force pushed as there was no new changes obviously, just the commit msg)

@kevpar kevpar self-assigned this Sep 9, 2021
@dcantah
Copy link
Contributor Author

dcantah commented Sep 9, 2021

kubernetes/kubernetes#104880 Running the k8s test that get run on a pr right now (just the windows/linux containerd tests left) against a branch I made on here with this work.

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

Only other thing that would be nice to see is a description of what testing was done (so we can review if this breaks something down the line). Otherwise LGTM.

@dcantah
Copy link
Contributor Author

dcantah commented Sep 9, 2021

Only other thing that would be nice to see is a description of what testing was done (so we can review if this breaks something down the line). Otherwise LGTM.

Ok, I will add to the commit desc. All the k8s tests just came back green. Gonna run our cri-containerd tests one more time with a shim built from this branch

Due to some unfortunate deps in the root go.mod in containerd + some k8s staging
directory behavior, containerd 1.5 errors out when trying to get vendored into
kubernetes/kubernetes. Because we depend on ctrd 1.5, if hcsshim tries to
get bumped it will ask that we also bump their dep of containerd to 1.5
and things will fail also. This reverts our containerd dep for the release/0.8
branch to 1.4 so we're able to circumvent this, at least temporarily.

Issue tracking this problem that contains k8s proposed solution and statements on
this: kubernetes/kubernetes#104827

"Components that have a transitive dependency to anything in k8s.io/kubernetes,
including the staging components, can not be added as a dependency. The
detection and prevention of this in the dependency management scripts is
intentional."

While this work is temporary just to get k8s a new hcsshim release, longer
term we will need to find a way to trim our containerd dep either here, or
containerd will need to get rid of it's k8s.io/* dependencies in their root go.mod
to remove this circular import issue.

The tests ran to make sure nothing broke with this
---------------------------------------------------------------------
-All of the tests that currently get ran on a K8s pull request which includes
the AKS engine e2e containerd test suite.
-Containerd integration tests
-Our cri-containerd tests with a shim built from this commit

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah
Copy link
Contributor Author

dcantah commented Sep 10, 2021

Our integration tests came back alright (minus the tests that I can't actually run on my machine e.g. the device ones). Going to check this in.

@dcantah dcantah merged commit 717ae58 into microsoft:release/0.8 Sep 10, 2021
@thaJeztah
Copy link
Contributor

Perhaps on the hcsshim side, it would make sense to make cmd/ (or each binary under cmd/) separate go modules. That way the "library code" provided by hcshhim, and "binary code" would be separated.

I was working on something similar for https://github.com/containerd/imgcrypt - containerd/imgcrypt#50 (haven't found time to continue that work yet)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants