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

Inject Windows custom binaries for use in PRs and running against Kubernetes CI #1388

Merged

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented May 25, 2021

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1276

Special notes for your reviewer:

is based on top of #1119. Will remove draft status once that PR merges.

requires kubernetes-sigs/cluster-api#4662 to run against k/k main branch

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Adds ability to run Windows Kubernetes e2e tests against CI and PR artifacts

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@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. kind/feature Categorizes issue or PR as related to a new feature. 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. labels May 25, 2021
@k8s-ci-robot k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 25, 2021
@jsturtevant jsturtevant force-pushed the windows-custom-binaries branch from ce2e39f to e0a7baa Compare May 25, 2021 23:51
@jsturtevant
Copy link
Contributor Author

/assign @chewong

this will require the updates in #1389 to run against the main branch of k/k

@jsturtevant jsturtevant marked this pull request as ready for review May 25, 2021 23:56
@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 May 25, 2021
image: sigwindowstools/kube-proxy:${KUBERNETES_VERSION}-nanoserver
- name: USE_CI_VERSION
value: "${USE_CI_VERSION:false}"
image: sigwindowstools/kube-proxy:${KUBERNETES_VERSION/+/_}-nanoserver
Copy link
Member

Choose a reason for hiding this comment

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

any plans to migrate away from dockerhub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, once we move to HostProcess containers we will build these images just like the Linux Kube-proxy image.

templates/addons/windows/kube-proxy-windows.yaml Outdated Show resolved Hide resolved
@jsturtevant jsturtevant force-pushed the windows-custom-binaries branch 2 times, most recently from ef32d45 to a774807 Compare June 4, 2021 21:52
@chewong
Copy link
Member

chewong commented Jun 7, 2021

/retest

Copy link
Member

@chewong chewong left a comment

Choose a reason for hiding this comment

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

Few nits. Otherwise LGTM (other than the kustomize issue)

scripts/ci-build-kubernetes.sh Outdated Show resolved Hide resolved
@jsturtevant jsturtevant mentioned this pull request Jun 7, 2021
3 tasks
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2021
@jsturtevant jsturtevant force-pushed the windows-custom-binaries branch from a774807 to 551a98b Compare June 8, 2021 21:10
@jsturtevant
Copy link
Contributor Author

pull-cluster-api-provider-azure-e2e-windows is failing due to not having linux nodes. For upstream tests we don't need the Linux nodes but we do want one around for our capz specific tests. working on a fix

Opened kubernetes/test-infra#22485 for validating upstream but is blocked on kube-test viper change being worked on in #1421

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2021
@jsturtevant
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows

@jsturtevant
Copy link
Contributor Author

/hold
until #1421 merged with the kubetest updates so we can run kubernetes/test-infra#22485

@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 9, 2021
@jsturtevant
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows
/test pull-cluster-api-provider-azure-windows-upstream-with-ci-artifacts
/retest

@marosset
Copy link
Contributor

/retest

@CecileRobertMichon
Copy link
Contributor

Error: Unexpected non-nil/non-zero extra argument at index 1:
<*errors.errorString>: &errors.errorString{s:"system machine pools not ready"}

@alexeldeib is this a known flake? I don't think I've seen that one before

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2021
@jsturtevant jsturtevant force-pushed the windows-custom-binaries branch from fe6d1e0 to 44efade Compare August 11, 2021 19:21
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2021
@jsturtevant
Copy link
Contributor Author

looks like e2e-exp is passing again. I will run the conformance jobs and the PR job

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows
/test pull-cluster-api-provider-azure-windows-upstream-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-v1alpha4

@jsturtevant jsturtevant force-pushed the windows-custom-binaries branch from 44efade to 5df84fc Compare August 12, 2021 16:15
@jsturtevant
Copy link
Contributor Author

The tests passed last round but I had a bad commit message, I've rebased

/test pull-cluster-api-provider-azure-upstream-v1alpha4-windows
/test pull-cluster-api-provider-azure-windows-upstream-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-v1alpha4

@jsturtevant
Copy link
Contributor Author

@CecileRobertMichon tests have passed several times in a row now :-) no changes other than the rebases.

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon 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

Thanks @jsturtevant! I look forward to running these in CI

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, devigned

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:
  • OWNERS [CecileRobertMichon,devigned]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@CecileRobertMichon
Copy link
Contributor

@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 Aug 12, 2021
@jsturtevant
Copy link
Contributor Author

yes, this type of issue is occurring regularly on Linux as well. I believe it is a flake but can't really track it down till it we get more data. That test isn't a part of this set of changes anyways. It is part of the changes merged in #1119

I don't think adding code fixes to the PR would valuable anyways. Once we get the CI running regularly I can figure out if it is a flake or something else.

@CecileRobertMichon
Copy link
Contributor

fair, let's get this merged

/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 Aug 12, 2021
@CecileRobertMichon CecileRobertMichon mentioned this pull request Aug 12, 2021
3 tasks
@k8s-ci-robot k8s-ci-robot merged commit b9f62a3 into kubernetes-sigs:master Aug 12, 2021
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/provider/azure Issues or PRs related to azure provider 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to inject kube binaries from kubernetes CI builds on Windows
6 participants