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
WIP: Bump cloud-provider-gcp to k8s v1.24 #333
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Fedosin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5874b8d
to
377810d
Compare
/test cloud-provider-gcp-e2e-full |
1c43ac9
to
eeb6ba3
Compare
Before that we should probably do #331 to have a clean cut for 1.23. /hold |
Ran: go mod tidy && ./tools/update_vendor.sh && ./tools/update_bazel.sh
33a8cb6
to
883c4b9
Compare
I see diffs between k/k release-1.24 branch and this commit (regarding the cluster directory). Are those changes by design? |
/assign |
@Fedosin: The following test failed, say
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. |
@Fedosin: PR needs rebase. 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. |
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've reviewed /cluster
part. It seems that some of stuff needs to be different from k/k repository, to enable running CCM.
@@ -311,10 +311,9 @@ function set_binary_version() { | |||
function find-tar() { | |||
local -r tarball=$1 | |||
locations=( | |||
"${KUBE_ROOT}/bazel-bin/release/${tarball}" |
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.
It seems that this is reverting CCM specific code change, there are some other. I'm doing upgrade to newest release-1.23 k/k /cluster directory to identify all differences between k/k and cloud-provider-gcp differences in #341
@@ -506,13 +505,8 @@ EOF | |||
# If KUBERNETES_SKIP_CONFIRM is set to y, we'll automatically download binaries | |||
# without prompting. | |||
function verify-kube-binaries() { | |||
# TODO: @cheftako Remove the hack to get a local kubectl from existing tars. |
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'm not sure if we can change that (it might be cloud-provider-gcp specific)
@@ -27,7 +27,10 @@ function get-num-nodes { | |||
# NUM_NODES | |||
# NUM_WINDOWS_NODES | |||
function get-master-size { | |||
local suggested_master_size=2 |
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 needs to be different. We are running additional controller on master machine
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.
Added setting on presubmit level, no need to revert 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.
Actually, it's still needed (and also node size override to n1-standard-4), as kubetest2 does not support presets.
@@ -419,10 +404,7 @@ EVICTION_HARD="${EVICTION_HARD:-memory.available<250Mi,nodefs.available<10%,node | |||
SCHEDULING_ALGORITHM_PROVIDER="${SCHEDULING_ALGORITHM_PROVIDER:-}" | |||
|
|||
# Optional: install a default StorageClass | |||
ENABLE_DEFAULT_STORAGE_CLASS="${ENABLE_DEFAULT_STORAGE_CLASS:-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.
422-425 are cloud-provider-gcp specific.
# TLS_CIPHER_SUITES defines cipher suites allowed to be used by kube-apiserver. | ||
# If this variable is unset or empty, kube-apiserver will allow its default set of cipher suites. | ||
export TLS_CIPHER_SUITES="" | ||
|
||
# Optional: Enable credential sidecar. |
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.
570 - 582 are cloud-provider-specific, we need them.
@@ -1336,9 +1273,6 @@ ${var_name}: ${var_value} | |||
EOF | |||
done | |||
fi | |||
cat >>$file <<EOF | |||
ENABLE_CREDENTIAL_SIDECAR: $(yaml-quote ${ENABLE_CREDENTIAL_SIDECAR:-false}) |
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.
Needed.
@@ -1379,8 +1313,6 @@ KUBE_POD_LOG_READERS_GROUP: 2007 | |||
KONNECTIVITY_SERVER_RUNASUSER: 2008 | |||
KONNECTIVITY_SERVER_RUNASGROUP: 2008 | |||
KONNECTIVITY_SERVER_SOCKET_WRITER_GROUP: 2008 | |||
CLOUD_CONTROLLER_MANAGER_RUNASUSER: 2009 |
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.
Needed.
@@ -97,7 +97,7 @@ function detect_client_info() { | |||
if [ -n "${KUBERNETES_CLIENT_ARCH-}" ]; then | |||
CLIENT_ARCH="${KUBERNETES_CLIENT_ARCH}" | |||
else | |||
# TODO: migrate the kube::util::host_platform function out of hack/lib and |
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.
Might be reverted, to keep in line with usage of util.sh from /cluster
@@ -32,15 +32,25 @@ set -o pipefail | |||
|
|||
KUBE_ROOT=${KUBE_ROOT:-$(dirname "${BASH_SOURCE[0]}")/..} | |||
source "${KUBE_ROOT}/cluster/kube-util.sh" | |||
source "${KUBE_ROOT}/cluster/clientbin.sh" |
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 file is tricky, I'm not sure if we need to use cloud-provider-gcp version, or we are free to use k/k
@@ -41,7 +41,7 @@ readonly master_ssh_supported_providers="gce aws" | |||
readonly node_ssh_supported_providers="gce gke aws" | |||
readonly gcloud_supported_providers="gce gke" | |||
|
|||
readonly master_logfiles="kube-apiserver.log kube-apiserver-audit.log kube-scheduler.log kube-controller-manager.log etcd.log etcd-events.log glbc.log cluster-autoscaler.log kube-addon-manager.log konnectivity-server.log fluentd.log kubelet.cov cloud-controller-manager.log" |
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.
Please revert, so we will gather logs from ccm.
/close It has already happened. |
@jprzychodzen: Closed this PR. 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/test-infra repository. |
Ran:
go mod tidy && ./tools/update_vendor.sh