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.

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

Updating base container image for pause image on Windows #110379

Merged
merged 1 commit into from Jun 15, 2022
Merged

Updating base container image for pause image on Windows #110379

merged 1 commit into from Jun 15, 2022

Conversation

marosset
Copy link
Contributor

@marosset marosset commented Jun 3, 2022 •
edited

Signed-off-by: Mark Rossetti marosset@microsoft.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

The PR updates the base image used to build the Windows pause container images to an image that has some registry changes.
We are using an intermediate container image that is built on a Windows machine because Windows registry changes do not 'stick' when the images are built with buildkit. (more details in #109161)

Which issue(s) this PR fixes:

Fixes #109161

Special notes for your reviewer:

Please see #109161 and https://github.com/microsoft/windows-pause-image-base for details on why this is neccessary.

Does this PR introduce a user-facing change?

Updating base image for Windows pause container images to one built on Windows machines to address limitations of building Windows container images on Linux machines.

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

/sig windows

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 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. sig/windows Categorizes an issue or PR as relevant to SIG Windows. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 3, 2022
@marosset
Copy link
Contributor Author

marosset commented Jun 3, 2022

/cc @dims @jsturtevant @claudiubelu

@marosset
Copy link
Contributor Author

marosset commented Jun 3, 2022

After we finalize the process for how to consume an intermediate layer for the Windows pause image we can do a proper fix for this hack

kubernetes/build/pause/Dockerfile_windows

Lines 21 to 24 in 4988bfc

# NOTE(claudiub): We're replacing the diagtrack.dll as a means to disable the
# DiagTrack service (it cannot run without this DLL). We do not need this
# service in the pause image and there's no reason for it to have any CPU usage.
ADD windows/pause.c /Windows/System32/diagtrack.dll

by cleanly disabling the service during the windows-pause-image-base build.

I suspect there are a number of other services we can disable to improve pod startup time

@marosset
Copy link
Contributor Author

marosset commented Jun 3, 2022

I built https://hub.docker.com/r/mrosse3/pause/tags with these changes

@sftim
Copy link
Contributor

sftim commented Jun 7, 2022

Does this need any release note? I'd expect one.

@dims
Copy link
Member

dims commented Jun 7, 2022

/approve

@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 7, 2022
@dims
Copy link
Member

dims commented Jun 7, 2022

@marosset please update TAG as well (same Makefile at the top) to 3.8

@dims
Copy link
Member

dims commented Jun 7, 2022

/hold

@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 Jun 7, 2022
@marosset
Copy link
Contributor Author

marosset commented Jun 7, 2022

Does this need any release note? I'd expect one.

I'll add one.
TBH I was expecting more discussion and wanted to make sure the release note reflected the outcome.

@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 8, 2022
@marosset
Copy link
Contributor Author

marosset commented Jun 8, 2022

/triage accepted
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 8, 2022
@marosset
Copy link
Contributor Author

marosset commented Jun 8, 2022

@marosset please update TAG as well (same Makefile at the top) to 3.8

Done (and I updated CHANGELOG.md too)

@marosset
Copy link
Contributor Author

marosset commented Jun 8, 2022

Does this need any release note? I'd expect one.

Added!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, marosset

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

@jsturtevant
Copy link
Contributor

/lgtm

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

marosset commented Jun 8, 2022

/hold

Let me address #110379 (comment) and bump the new pause image version before this merges

@zhiweiv zhiweiv mentioned this pull request Jun 9, 2022
Closed
4 tasks
@marosset
Updating base container image for pause image on Windows
5375102 
Signed-off-by: Mark Rossetti <marosset@microsoft.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2022
@marosset
Copy link
Contributor Author

marosset commented Jun 9, 2022

We released another version of the pause base image that has the diagtrack service disabled.
I'm OK to unhold the changes but I'll let @dims take another look if needed.

Also not that the new base images have notary signatures!

@dims
Copy link
Member

dims commented Jun 9, 2022

@marosset LGTM

@marosset marosset added this to In Review (v1.25) in SIG-Windows Jun 13, 2022
@jsturtevant
Copy link
Contributor

/lgtm

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

Looks good, mcr.microsoft.com/oss/kubernetes/windows-pause-image-base:v0.2 exists and it's a manifest list which contains the mentioned OS Versions that we need. And the mcr.microsoft.com/oss/kubernetes/windows-pause-image-base:v0.2-windows-1809-amd64, mcr.microsoft.com/oss/kubernetes/windows-pause-image-base:v0.2-windows-20H2-amd64, mcr.microsoft.com/oss/kubernetes/windows-pause-image-base:v0.2-windows-ltsc2022-amd64 images exist as well.

/lgtm

@marosset
Copy link
Contributor Author

/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 Jun 15, 2022
@k8s-ci-robot k8s-ci-robot merged commit 7de86ff into kubernetes:master Jun 15, 2022
SIG-Windows automation moved this from In Review (v1.25) to Done (v1.25) Jun 15, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jun 15, 2022
@marosset
Copy link
Contributor Author

Here is the manifest that got pushed as a result of the
https://testgrid.k8s.io/sig-k8s-infra-gcb#post-kubernetes-push-image-pause

https://console.cloud.google.com/gcr/images/k8s-staging-kubernetes/GLOBAL/pause@sha256:9001185023633d17a2f98ff69b6ff2615b8ea02a825adffa40422f51dfdcde9d/details?tag=3.8&tab=pull

Looks good to me!

@marosset marosset mentioned this pull request Jul 21, 2022
Merged
@marosset marosset deleted the windows-pause-image-base branch October 31, 2022 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claudiubelu claudiubelu Awaiting requested review from claudiubelu

@dims dims Awaiting requested review from dims

@jsturtevant jsturtevant Awaiting requested review from jsturtevant

@puerco puerco Awaiting requested review from puerco

@Verolop Verolop Awaiting requested review from Verolop

Assignees

@jsturtevant jsturtevant

@claudiubelu claudiubelu

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-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG-Windows
  
Done (v1.25)
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

Cannot modify registry keys during Windows pause image build process
6 participants
@marosset @sftim @dims @k8s-ci-robot @jsturtevant @claudiubelu