-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Adding preset to use Azure resources, moving 2022 master job to azure #32998
Adding preset to use Azure resources, moving 2022 master job to azure #32998
Conversation
Hi @ritikaguptams. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
/cc @nawazkh @jackfrancis |
fyi @dtzar |
config/jobs/kubernetes-sigs/cluster-api-provider-azure/cluster-api-provider-azure-presets.yaml
Outdated
Show resolved
Hide resolved
config/jobs/kubernetes-sigs/cluster-api-provider-azure/cluster-api-provider-azure-presets.yaml
Outdated
Show resolved
Hide resolved
config/jobs/kubernetes-sigs/cluster-api-provider-azure/cluster-api-provider-azure-presets.yaml
Outdated
Show resolved
Hide resolved
config/jobs/kubernetes-sigs/cluster-api-provider-azure/cluster-api-provider-azure-presets.yaml
Outdated
Show resolved
Hide resolved
@ritikaguptams can fix up our testing PR too? test-infra/config/jobs/kubernetes-sigs/sig-windows/release-master-windows-presubmits.yaml Line 412 in 28b6fd8
|
/ok-to-test |
0c38e9c
to
bfab502
Compare
interval: 3h | ||
decorate: true | ||
decoration_config: | ||
timeout: 4h | ||
labels: | ||
preset-dind-enabled: "true" | ||
preset-kind-volume-mounts: "true" | ||
preset-azure-cred-wi: "true" | ||
preset-azure-community: "true" | ||
preset-azure-anonymous-pull: "true" # Sets REGISTRY which is needed when building CCM/CNM images |
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.
we will need to remove this if we add Registry to the preset.
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.
For windows we don't need it, but we might need to create a different preset for CAPZ which uses USE_LOCAL_KIND_REGISTRY
which is in that preset
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.
@jackfrancis registry seems common to jobs but USE_LOCAL_KIND_REGISTRY
doesn't. Should we create a new one or just roll USE_LOCAL_KIND_REGISTRY into the new capz preset (it doesn't hurt windows to have that set)
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.
based on this
- name: REGISTRY
value: "capzcicommunity.azurecr.io"
yeah it seems like we should include USE_LOCAL_KIND_REGISTRY=false
in the preset
Is that what you mean?
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.
Done
bfab502
to
7f5d116
Compare
a0b969f
to
79c8df5
Compare
@@ -485,7 +473,6 @@ presubmits: | |||
preset-dind-enabled: "true" | |||
preset-kind-volume-mounts: "true" | |||
preset-azure-cred-wi: "true" | |||
preset-azure-anonymous-pull: "true" # Sets REGISTRY which is needed when building CCM/CNM images |
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 don't think we want to be dropping this preset on this job yet.
interval: 3h | ||
decorate: true | ||
decoration_config: | ||
timeout: 4h | ||
labels: | ||
preset-dind-enabled: "true" | ||
preset-kind-volume-mounts: "true" | ||
preset-azure-cred-wi: "true" | ||
preset-azure-anonymous-pull: "true" # Sets REGISTRY which is needed when building CCM/CNM images | ||
preset-azure-community: "true" | ||
preset-capz-windows-common: "true" | ||
preset-capz-containerd-1-7-latest: "true" | ||
preset-windows-private-registry-cred: "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.
This secret won't be in the new k8s-infra-prow-build
. We should remove it
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.
Done
af6d81c
to
1bbbd55
Compare
Signed-off-by: ritikaguptams <ritikagupta@microsoft.com>
1bbbd55
to
6259f2c
Compare
preset-capz-windows-common: "true" | ||
preset-capz-containerd-1-7-latest: "true" | ||
preset-windows-private-registry-cred: "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.
Just a note this will break the test Kubernetes e2e suite.[It] [sig-node] Container Runtime blackbox test when running a container with a new image should be able to pull from private registry with secret [NodeConformance]
we are tracking fixing this in kubernetes-sigs/windows-testing#446
/lgtm |
Quite late to the party, thank you. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, jsturtevant, ritikaguptams 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 |
@@ -259,7 +258,6 @@ periodics: | |||
path_alias: sigs.k8s.io/cloud-provider-azure | |||
workdir: false | |||
spec: | |||
serviceAccountName: prowjob-default-sa |
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.
What was the reasoning behind dropping this? We dont need this anymore?
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 understood we only needed this to "simulate" the community infra service account config, and once we migrated this is the configuration we will expect by default.
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.
Its not needed in the community cluster, the default service account will work
@ritikaguptams: Updated the
In response to this: 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-sigs/prow repository. |
No description provided.