-
Notifications
You must be signed in to change notification settings - Fork 264
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.
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
doc: clarify usage & deprecated tags #700
Conversation
/assign @jterry75 |
README.md
Outdated
@@ -6,6 +6,8 @@ This package contains the Golang interface for using the Windows [Host Compute S | |||
|
|||
It is primarily used in the [Moby Project](https://github.com/moby/moby), but it can be freely used by other projects as well. | |||
|
|||
This is a code-only repository and does not have a stable release. 0.8.6 is the last tagged release, and it is out of date for ContainerD and Kubernetes. Downstream projects should vendor in this codebase using commit ids instead of tags, then validate fixes and do a full test pass of the whole project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer we say something along the lines of: "The tags are associated with a vendoring scheme that is only intended for usage by Docker and the v1 container
and process
API's. This vendoing scheme is not suitable for ContainerD or Kubernetes which should use commit id's ...".
Feel free to re-word this and prettify it but that is what those numbers are for. Its not for anything other than that and its all we test them for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated - can you review again @jterry75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jterry75 i see a new release 0.8.7
that says This is the v0.8.7 release of containerd-shim-runhcs-v1.exe for containerd
So is the statement above still true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v0.8.7 is ok for the shim because that is a binary dependency based on a stable API contract (Runtime V2) from containerd. The intent is to make vendor
based commits (other than Docker) use commit id's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PatrickLang - Can you update this to be a little more generic and reflect the confusion. The goal of our releases is to:
- Provide a vendor based commit tag for Docker which is code based.
- Provide a binary release version tag for the stable release of the containerd shim supported in containerd releases.
- All others are expected to use commit hashes because we are not verifying that when we make changes to a code dependency that we did not break them. We are only verifying this with Docker based vendoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jterry75 - I'm not sure I understand where Kubernetes falls into 1-3 based on the following:
- Docker-EE basic is still in use
- Kubernetes makes extra calls not supported in Docker through HCS for gathering metrics, network stats, and HNS configuration. Those rely on hcsshim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that would put Kubernetes in the 3rd case, since they're carrying a code dependency on hcsshim directly. It's not affected by Docker's use of hcsshim, and doesn't (for example) imply they must use the same hcsshim version that Docker users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
I guess this is fine (and similar to the containerd Go Client API not being stable). Perhaps a heading above this section (to make it stand out better), and updating the description of the (last) tagged release on GitHub to include the same warning would be useful I'm not sure how Some questions; Is the intent to have a stable Go API in future? I'm not too familiar with the code/api underneath, thus wondering if it's only the Go API I should be concerned about when updating (in moby/moby); a breaking change in the Go API (e.g., a Signature Change or rename of a function) would be easily detected if the code no longer compiles (and can be fixed by adjusting the code that consumes hcsshim), but would there be a risk that the underlying implementation also changed in a breaking way? |
I'm a bit confused by this PR. It seems that since it was created, a new tagged release was made, which includes the release of the new containerd Runtime V2 shim. Does that matter? Or should we just ignore it? containerd is still currently vendored to an older commit than the latest tagged release. |
@thaJeztah - No then intent is not to have a stable go API and in fact is the opposite. With the move from Docker to Containerd for runtime the goal is that Containerd depends on our |
Would that goal require that containerd no longer vendors/imports hcsshim? It's using a few other things than just the shim, and I think it's even using the hcsshim Go API in order to call the shim? I didn't look that closely, I mainly noticed because I think hcsshim is the thing that makes containerd need cgo, pending revendoring to pick up #749. |
@TBBle - The only thing containerd "should" be using from hcsshim is the storage API's for process isolated container mounts and the content store layer processing. It is using only the Runtime V2 ttrpc API to interface with the shim so no code dependency there. |
There has been a lot of confusion since this repo used tags in the past, but that practice has stopped. I see that ContainerD has moved on to vendoring based on commitid, and after discussion is was recommended that Kubernetes do the same. This PR puts that recommendation in the README.
Related discussion:
kubernetes/kubernetes#80116 (comment)