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

Add support for building Windows pause image #91452

Merged
merged 2 commits into from Oct 23, 2020
Merged

Add support for building Windows pause image #91452

merged 2 commits into from Oct 23, 2020

Conversation

claudiubelu
Copy link
Contributor

@claudiubelu claudiubelu commented May 26, 2020 •
edited

What type of PR is this?

/kind feature

/sig windows

What this PR does / why we need it:

We can use docker buildx in order to build and push Windows images from the same Linux node, as long as the Dockerfile does not have any RUN commands in the Windows step.

We also need to create a non-default builder instance in order to be able to build and push Windows images.

The Windows images have to be built and pushed directly to the registry. Because of this, the make target push has been removed (the target all will build and push the images).

We need wincat for a few kubectl proxy scenarios.

For Windows containers without Hyper-V isolation, the host OS Version and the Container OS Version need to match, which is why we added multiple Windows OS Versions to the building process.

Adds support for Windows OS Versions: 1809, 1903, 1909, 2004.

Bumped pause image version to 3.4.

Co-Authored-By: Claudiu Belu cbelu@cloudbasesolutions.com
Co-Authored-By: Ben Moss bmoss@pivotal.io

Signed-off-by: Leah Hanson lhanson@pivotal.io

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Support for Windows container images (OS Versions: 1809, 1903, 1909, 2004) was added the pause:3.4 image.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 26, 2020
@k8s-ci-robot k8s-ci-robot requested review from dims and listx May 26, 2020 13:36
@claudiubelu claudiubelu changed the title WIP: Add support for building Windows pause image Add support for building Windows pause image Jun 16, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 16, 2020
@claudiubelu claudiubelu force-pushed the windows/pause-image branch 2 times, most recently from 7dc389d to db9e589 Compare June 16, 2020 14:47
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 16, 2020
@claudiubelu
Copy link
Contributor Author

/cc @dims

@claudiubelu claudiubelu force-pushed the windows/pause-image branch 3 times, most recently from 2585a1d to ba4e395 Compare June 24, 2020 13:38
@claudiubelu
Copy link
Contributor Author

/test pull-kubernetes-integration

@claudiubelu claudiubelu mentioned this pull request Jun 29, 2020
Closed
@claudiubelu
Copy link
Contributor Author

/retest

dims
dims reviewed Jul 29, 2020
View reviewed changes
build/pause/Makefile Outdated Show resolved Hide resolved
@dims
Copy link
Member

dims commented Jul 29, 2020

cc @endocrimes

@claudiubelu
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-100-performance

@dims
Copy link
Member

dims commented Aug 10, 2020

/assign @BenTheElder @cblecker
/unassign

@claudiubelu
Copy link
Contributor Author

/sig node
/sig release
Is there some history on the windows sources? (a previous image somewhere, or someone from node / windows can comment on these binaries?)

I assume you mean for wincat.go, right? If so, the code was taken almost as is from here: https://github.com/kubernetes-sigs/sig-windows-tools/tree/master/cmd/wincat , and the original author, Ben Moss, gave us his blessing.

wincat is being used for port-forwarding, and this is the current usage [1]. We've been including wincat into our Windows pause images for a while [2]. Because of this, the port-forwarding tests work.

[1] https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/dockershim/docker_streaming_windows.go#L31
[2] https://github.com/kubernetes-sigs/sig-windows-tools/blob/master/cmd/wincat/Dockerfile

@BenTheElder
Copy link
Member

Is it used for port-forwarding in containerd as well?

In either case, I think we should add a comment in the soruce or a README.md about this, if the runtimes no longer need it for port forwarding (such as with socat on linux) we might want to remove it in the future. As it stands someone will have to go spelunking to find this PR's descriptoin.

@BenTheElder
Copy link
Member

/approve
/hold
(in case for handling README now)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, claudiubelu, dims

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 Sep 29, 2020
@claudiubelu
Copy link
Contributor Author

Is it used for port-forwarding in containerd as well?

In either case, I think we should add a comment in the soruce or a README.md about this, if the runtimes no longer need it for port forwarding (such as with socat on linux) we might want to remove it in the future. As it stands someone will have to go spelunking to find this PR's descriptoin.

That's a question I've raised myself yesterday too. I've tried it on a containerd env, and no. :) And that's something I'll be working on next.

In any case, I will update the readme shortly.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2020
claudiubelu
claudiubelu commented Sep 30, 2020
View reviewed changes
Copy link
Contributor Author

@claudiubelu claudiubelu left a comment •
edited

Choose a reason for hiding this comment

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

I've added some comments that explain the purpose of wincat, and I've linked this PR too, in case more information is needed.

Additionally, I'm going to link here the original issues that lead to wincat and some of its bug fixes:

#78857
#75479
kubernetes-sigs/sig-windows-tools#5

build/pause/Makefile

# NOTE(claudiub): The Windows pause image also requires the wincat binary we're compiling for the
# port-forwarding scenarios. If it's no longer necessary, it can be removed.
# For more information, see: https://github.com/kubernetes/kubernetes/pull/91452
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment here mentioning the purpose of the wincat binary. I've also linked this PR, in case anyone needs to read more information about it.

build/pause/windows/wincat/wincat.go
*/

// package wincat connects to the given host and port and redirects its stdin to the connection and
// the connection's output to stdout. This is currently being used for port-forwarding for Windows Pods.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also explains the purpose of wincat.

@claudiubelu
Copy link
Contributor Author

Update on the containerd port-forwarding: I was using containerd 1.3.4. I have now updated to 1.4.0, and port-forwarding works for Windows containerd too.

I also found this: containerd/cri#1284

As you can see, wincat is used by containerd too.

@marosset
Copy link
Contributor

marosset commented Oct 7, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2020
@mkorbi
Copy link

mkorbi commented Oct 20, 2020

Hello @claudiubelu
This PR has been updated frequently, but it looks like the merge is blocked for some days, so I'd like to check what's the status. The code freeze is starting 12.11.2020 (about 3-4 weeks from now) and while there is still plenty of time, we want to ensure that each PR has a chance to be merged on time.
As the PR is tagged for 1.20, is it still planned for this release?

@claudiubelu
Copy link
Contributor Author

Hello @claudiubelu
This PR has been updated frequently, but it looks like the merge is blocked for some days, so I'd like to check what's the status. The code freeze is starting 12.11.2020 (about 3-4 weeks from now) and while there is still plenty of time, we want to ensure that each PR has a chance to be merged on time.
As the PR is tagged for 1.20, is it still planned for this release?

This PR has already been approved and LGTM'd, it only has a hold on it. IMO, we can proceed with it, I have answered Ben's question.

@dims
Copy link
Member

dims commented Oct 22, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 22, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 68e4f1f into kubernetes:master Oct 23, 2020
This was referenced Oct 27, 2020
Closed
Closed
@pacoxu
Copy link
Member

pacoxu commented Jan 19, 2021 •
edited

I try to pull k8s.gcr.io/pause:3.4 and failed on Linux.

A quick question here: why not build pause image that combined windows and Linux pause image?
Current Linux use pause 3.2 or 3.3 and Windows use 3.4?

@claudiubelu @BenTheElder

@claudiubelu
Copy link
Contributor Author

.gcr.io/pause:3.4

Hello.

The pause:3.4 image has been built, but it was not published / promoted yet: https://github.com/kubernetes/k8s.io/blob/08581c1cd0937d893f0d1ffdfec8ceb8d7aa8356/k8s.gcr.io/images/k8s-staging-kubernetes/images.yaml

Will send a PR to promote the image.

@pacoxu pacoxu mentioned this pull request Jan 20, 2021
Merged
10 tasks
@jsturtevant jsturtevant mentioned this pull request Feb 17, 2021
Closed
@jsturtevant jsturtevant mentioned this pull request Mar 4, 2021
Merged
@jsturtevant jsturtevant mentioned this pull request Jul 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dims dims dims left review comments

@benmoss benmoss benmoss requested changes

@BenTheElder BenTheElder BenTheElder left review comments

@endocrimes endocrimes endocrimes left review comments

@marosset marosset marosset left review comments

@jeremyje jeremyje jeremyje requested changes

@listx listx Awaiting requested review from listx

Assignees

@dims dims

@spiffxp spiffxp

@justaugustus justaugustus

@BenTheElder BenTheElder

@cblecker cblecker

@marosset marosset

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet