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

Image promoter: Reenable Windows test image building #89152

Merged

Conversation

claudiubelu
Copy link
Contributor

What type of PR is this?

/kind bug
/kind cleanup

/sig windows
/sig testing

What this PR does / why we need it:

The build times are a bit high for the image builder (~50 minutes), and it will a bit more when Windows support will be added to the other test images. This commit changes the machineType to N1_HIGHCPU_8.

Reenables Windows test image building. Added DOCKER_CERT_BASE_PATH (default value: $HOME), which will contain the path where the certificates needed for Remote Docker Connection can be found.

If a REMOTE_DOCKER_URL was not set for a particular OS version, exclude that image from the
manifest list. This fixes an issue where, if REMOTE_DOCKER_URL was not set for Windows Server 1909, the Windows were completely excluded from the manifest list, including for Windows Server 1809 and 1903 which could have been built and pushed.

Sets test-webserver as the default CMD for kitten and nautilus. Since they are now based on agnhost, they should be set to run test-webserver to maintain previous behaviour.

Bumps the agnhost version to 2.13, as 2.12 has already been promoted. 2.13 will contain Windows support.

Adds Windows support for the kitten and nautilus images, so they can promoted together with agnhost (they were not previously promoted).

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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


@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/windows Categorizes an issue or PR as relevant to SIG Windows. sig/testing Categorizes an issue or PR as relevant to SIG Testing. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 16, 2020
@claudiubelu
Copy link
Contributor Author

/retest

@marosset marosset added this to Backlog (v1.19) in SIG-Windows Mar 23, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2020
@claudiubelu claudiubelu force-pushed the image-promoter/reenable-windows branch from db3175f to eb1a4f8 Compare April 6, 2020 09:52
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2020
@claudiubelu claudiubelu force-pushed the image-promoter/reenable-windows branch from eb1a4f8 to 291527c Compare April 6, 2020 09:54
@marosset
Copy link
Contributor

marosset commented Apr 6, 2020

/milestone v1.19

@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Apr 6, 2020
@marosset
Copy link
Contributor

marosset commented Apr 6, 2020

/retest

@dims
Copy link
Member

dims commented Apr 6, 2020

/assign @listx @mkumatag @spiffxp

@claudiubelu
Copy link
Contributor Author

/test pull-kubernetes-node-e2e-containerd

@spiffxp
Copy link
Member

spiffxp commented Apr 7, 2020

/skip pull-kubernetes-node-e2e-containerd
ref: #89847

@spiffxp
Copy link
Member

spiffxp commented Apr 7, 2020

/approve
This seems ok, but I'd like a second pair of eyes on this

/assign @Katharine @fejta
I'd like an /lgtm from one of you

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2020
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

@spiffxp
Copy link
Member

spiffxp commented Apr 7, 2020

/override pull-kubernetes-node-e2e-containerd
ref: #89847

@k8s-ci-robot
Copy link
Contributor

@spiffxp: Overrode contexts on behalf of spiffxp: pull-kubernetes-node-e2e-containerd

In response to this:

/override pull-kubernetes-node-e2e-containerd
ref: #89847

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/test-infra repository.

@claudiubelu
Copy link
Contributor Author

Can we get OWNERS in these images? For example,

https://github.com/kubernetes/kubernetes/tree/291527c387f07c3eecae030e48f9bb5ff8254d85/test/images/agnhost

You mean in the agnhost image, or all of them? Sure, but there those images did not have any OWNER files in them in the first place. Who should the owners be? People who contributed the most to them? And since ~20 images have been centralized into agnhost, I assume that we could add an OWNER file per agnhost subcommand (e.g.: the netexec subcommand OWNERS should be the netxec image OWNERS).

In any case, we could do that in a separate PR, if we're going to add an OWNER file for every image.

@fejta
Copy link
Contributor

fejta commented Apr 10, 2020

I do not know anything about these images.

  • I'll approve a PR that adds owners to the images.
  • I'll approve a PR that both adds owners and improvements to images.
  • I cannot approve a PR that only adds improvements to unowned images.

@claudiubelu
Copy link
Contributor Author

@jasonrichardsmith

Can we get OWNERS in these images? For example,

https://github.com/kubernetes/kubernetes/tree/291527c387f07c3eecae030e48f9bb5ff8254d85/test/images/agnhost

Well, I hope this is what you wanted. :) #90062

@jasonrichardsmith
Copy link
Contributor

@claudiubelu I think you meant to tag @fejta .

@claudiubelu
Copy link
Contributor Author

@claudiubelu I think you meant to tag @fejta .

Oups! Sorry, my bad.

The build times are a bit high for the image builder (~50 minutes), and it will a bit more
when Windows support will be added to the other test images. This commit changes the
machineType to N1_HIGHCPU_8.

Reenables Windows test image building. Added DOCKER_CERT_BASE_PATH (default value: $HOME),
which will contain the path where the certificates needed for Remote Docker Connection can
be found.

If a REMOTE_DOCKER_URL was not set for a particular OS version, exclude that image from the
manifest list. This fixes an issue where, if REMOTE_DOCKER_URL was not set for Windows Server 1909,
the Windows were completely excluded from the manifest list, including for Windows Server 1809
and 1903 which could have been built and pushed.

Sets "test-webserver" as the default CMD for kitten and nautilus. Since they are now based on
agnhost, they should be set to run test-webserver to maintain previous behaviour.

Bumps the agnhost version to 2.13, as 2.12 has already been promoted. 2.13 will contain
Windows support.

Adds Windows support for the kitten and nautilus images, so they can promoted together
with agnhost (they were not previously promoted).

Adds OWNERS files to: agnhost, busybox, kitten, nautilus.
@claudiubelu
Copy link
Contributor Author

@mkumatag Please give your ok that you are willing to be the approvers for the nautilus and kitten images.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 15, 2020

@claudiubelu: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-node-e2e-containerd 291527c387f07c3eecae030e48f9bb5ff8254d85 link /test pull-kubernetes-node-e2e-containerd

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

@claudiubelu
Copy link
Contributor Author

/retest

Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

I'd love to see multiple owners but this is much better than before, thanks!

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: claudiubelu, fejta, spiffxp

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 merged commit a06d735 into kubernetes:master Apr 15, 2020
SIG-Windows automation moved this from Backlog (v1.19) to Done (v1.18) Apr 15, 2020
@marosset marosset moved this from Done (v1.18) to Done (v1.19) in SIG-Windows Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants