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

Update to latest /cluster directory from kubernetes/kubernetes #216

Merged
merged 5 commits into from
May 10, 2021

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented May 5, 2021

Fixes #214. I'll be writing up a design for how to keep the /cluster directory up-to-date more regularly through automation. Until then, this PR helps "catch up" with kubernetes/kubernetes /cluster directory. In addition to the immediate benefits of having a fresh /cluster directory, I expect this will help us with long term automation by eliminating merge conflicts caused by some of the adhoc cherry-picked commits from kubernetes/kubernetes.

Commits in this PR:

  • 1st commit: Rebases /cluster directory with the /cluster directory from kubernetes/kubernetes at HEAD as of a couple days ago (c5b900b69cf). The latest cluster directory PR from kubernetes/kubernetes included in this rebase is Fix typo in comment kubernetes#97399. In total, this rebase bundles up over 600 commits that have made changes to /cluster since the kubernetes v1.18 release, which is when the /cluster directory first introduced by PR Deploy Kubernetes from cloud-provider-gcp. #143. Since this PR is expected to merge after we cut our v1.21 cloud-provider-gcp branch, this should set is up well for v1.22.
  • 2nd commit: Selectively re-applies direct contributions made to the /cluster directory of cloud-provider-gcp that are clobbered by the rebase of the /cluster directory. These are listed out in detail in the below "Selectively re-applied changes in 2nd commit from previous cloud-provider-gcp PRs" section. This commit also fixes some hack/lib/util.sh -> cluster/util.sh fixes that has been overlooked and some executable file permission bits that were out-of-sync with kubernetes/kubernetes.
  • 3rd commit fixes Migrate seccomp annotation to API field in manifests #177 for cloud-controller-manager. All the other controllers are fixed by the rebase.

Notes for reviewers

  • The 2nd commit clearly shows what cloud-provider-gcp specific changes we're reapplying. This can be compared with previous PRs to check for correctness, but can also be shown to be correct more easily by checking the combined commits, which I explain next.
  • The diff of the combined commits should NOT undo any of the to cloud-provider-gcp specific changes already made, it should only contain upstream changes from kubernetes/kubernetes. Also, the combined diff should not mess with our BUILD or OWNER files or add files from kubernetes/kubernetes that we currently exclude (like cluster/images).

Alternative considered

Individually merge all 600+ commits from kubernetes/kubernetes into cloud-provider-gcp. I actually tried to introduce some automation for this (https://github.com/jpbetz/cloud-provider-gcp/blob/merge-cluster-changes/tools/merge-cluster-changes.sh). The problem using the automation is that there have been enough cherry-picks of more recent kubernetes/kubernetes /cluster directory changes into cloud-provider-gcp for files like configure-helper.sh that merges don't cleanly apply, and manual resolving 600+ merge conflicts would be required, which is incredibly tedious and error prone. Since there only a handful of PRs that really changed the /cluster directory in cloud-provider-gcp, it is much easier and safer to go with the rebase/reapply route. Once this PR merges and we're rebased I'll can circle back and see if the automation will work better, which I hope it might given that the problematic cherry-picks should be out of the way.

Selectively re-applied changes in 2nd commit from previous cloud-provider-gcp PRs

  • Deploy Kubernetes from cloud-provider-gcp. #143

    • cluster/addons/addon-manager/kube-addons.sh (moved mostly to cluster/addons/addon-manager/kube-addons-main.sh)
      • is_cloud_leader decl and if is_leader || is_cloud_leader
    • cluster/common.sh
      • hack/lib/util.sh -> cluster/util.sh
      • set_binary_version locations changes to just bazel-bin
      • verify-kube-binaries hack to get a local kubectl from existing tars
    • cluster/gce/config-test.sh
      • CLOUD_CONTROLLER_MANAGER_TEST_ARGS
    • cluster/gce/gci/configure-helper.sh
      • CLOUD_CONTROLLER_MANAGER_TOKEN set and used to append_or_replace_prefixed_line
      • system:cloud-controller-manager added (2 times) in Policy objects
      • start-kube-controller-manager: --cloud-provider=external
      • start-cloud-controller-manager decl
      • CLOUD_CONTROLLER_MANAGER_CPU_REQUEST
      • CLOUD_CONTROLLER_MANAGER_TOKEN
      • start-cloud-controller-manager called in main
    • cluster/gce/gci/configure.sh
      • try-load-docker-image "${img_dir}/cloud-controller-manager.tar"
      • cp "${src_dir}/cloud-controller-manager.tar" "${dst_dir}"
  • Add basic cluster up/down e2e test. #144

    • cluster/gce/gci/configure.sh - sha512 changes
  • Add logdump for e2e create. #148

    • cluster/log-dump/log-dump.sh - adds cloud-controller-manager.log as party of cherry-pick
  • Fix CCM image. #151

    • cloud-node-controller-role.yaml fixes
    • cluster/gce/gci/configure-helper.sh
      • add convert-manifest-params
      • start-kube-controller-manager
        • --external-cloud-volume-plugin=gce
      • start-cloud-controller-manager
        • --v=4, params+=" --port=10253"
      • kube_rc_docker_tag changes
    • cluster/gce/gci/configure-kubeapiserver.sh
      • start-kube-apiserver: --cloud-provider=external
    • cluster/gce/manifests/cloud-controller-manager.manifest
      • --log-file, --logtostderr (switched to --log_file by ???)
  • Fix shellcheck failure in cluster/gce/config-default.sh #152

  • Create the bucket for tars based on $PROJECT #154

    • cluster/gce/util.sh: add PROJECT to gsutil call (also in upstream ?)
  • Add auth-provider-gcp for out-of-tree credential provision #168

    • cluster/gce/config-default.sh: ENABLE_CREDENTIAL_SIDECAR setup
    • cluster/gce/gci/configure-helper.sh: create-sidecar-config if ENABLE_CREDENTIAL_SIDECAR and create-sidecar-config decl
    • cluster/gce/gci/configure.sh: mv auth-provider-gcp to bin
    • cluster/gce/util.sh: construct-linux-kubelet-flags with --image-credential-provider-config, ENABLE_CREDENTIAL_SIDECAR: $(yaml-quote …)
  • Disable local loopback for volume host #181

    • cluster/gce/gci/configure-helper.sh: --volume-host-allow-local-loopback
  • Bump cloud-provider-gcp to v1.21 #204

    • cluster/gce/manifests/cloud-controller-manager.manifest: --log-file -> --log_file

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 5, 2021
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 5, 2021
@jpbetz jpbetz force-pushed the cluster-replace-experiment branch 2 times, most recently from 7d700ba to e521822 Compare May 5, 2021 16:05
@jpbetz
Copy link
Contributor Author

jpbetz commented May 5, 2021

/assign @cici37 @jiahuif @DangerOnTheRanger

@jpbetz
Copy link
Contributor Author

jpbetz commented May 5, 2021

/retest

Error was:

I0505 16:24:25.984760    2725 down.go:53] about to delete nodeport firewall rule
I0505 16:24:25.984826    2725 local.go:42] ⚙️ gcloud compute firewall-rules delete --project k8s-boskos-gce-project-06 kt2-b67aea30-adbb-minion-nodeports
ERROR: (gcloud.compute.firewall-rules.delete) Could not fetch resource:
 - The resource 'projects/k8s-boskos-gce-project-06/global/firewalls/kt2-b67aea30-adbb-minion-nodeports' was not found

edit: actual problem was --terminated-pod-gc-threshold flag being set like @DangerOnTheRanger has found. Adding a temp fix commit on this PR until @DangerOnTheRanger merges a fix.

@DangerOnTheRanger
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 May 5, 2021
@jpbetz jpbetz force-pushed the cluster-replace-experiment branch from fde83b6 to 1e31d3d Compare May 6, 2021 19:20
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2021
@jpbetz
Copy link
Contributor Author

jpbetz commented May 7, 2021

Sanity check that pulling a private image works as expected: https://gist.github.com/jpbetz/31f4f720ebac8ad2a0c652eaeeb2640f

@cheftako
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 10, 2021
@cici37
Copy link
Contributor

cici37 commented May 10, 2021

/test cloud-provider-gcp-verify-all

@jpbetz jpbetz force-pushed the cluster-replace-experiment branch from 538c0ce to e892dd4 Compare May 10, 2021 21:21
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2021
@jpbetz
Copy link
Contributor Author

jpbetz commented May 10, 2021

cloud-provider-gcp-verify-all - will be fixed by #225
cloud-provider-gcp-e2e-create - startup failure with kube-controller-manager, this was passing so I've rebased and will look more deeply if that doesn't fix it

@jpbetz
Copy link
Contributor Author

jpbetz commented May 10, 2021

/test cloud-provider-gcp-verify-all

@jpbetz
Copy link
Contributor Author

jpbetz commented May 10, 2021

/hold

The last two cloud-provider-gcp-e2e-create failures show the kube-controller-manager failed to start on one run and the scheduler failed to start on the next. It appears to be a master CPU resource issue:

I0510 21:29:49.928577    1924 predicate.go:113] "Failed to admit pod, unexpected error while attempting to recover from admission failure" pod="kube-system/kube-scheduler-kt2-bc2069a0-b1d5-master" err="preemption: error finding a set of pods to preempt: no set of running pods found to reclaim resources: [(res: cpu, q: 25), ]"

cloud-provider-gcp-e2e-create uses a n1-standard-1 for master (1 vCPU).

CPU requests for the controllers are unchanged in this PR: https://github.com/kubernetes/cloud-provider-gcp/pull/216/files#diff-ada96bec4aba13195248d6dd21ee67e333bead2bd898b81a1b089d53bd06b4d8R3403-R3405 so I'm a bit unclear what's caused this to start happening. I don't see any other obvious changes to requested CPU in this PR.

@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 May 10, 2021
@jpbetz
Copy link
Contributor Author

jpbetz commented May 10, 2021

I checked the master (I set it to a n1-standard-2 to make sure it runs everything):

$ kubectl describe node kubernetes-master
  Namespace                   Name                                          CPU Requests  CPU Limits  Memory Requests  Memory Limits  Age
  ---------                   ----                                          ------------  ----------  ---------------  -------------  ---
  kube-system                 cloud-controller-manager-kubernetes-master    200m (10%)    0 (0%)      0 (0%)           0 (0%)         23m
  kube-system                 etcd-server-events-kubernetes-master          100m (5%)     0 (0%)      0 (0%)           0 (0%)         23m
  kube-system                 etcd-server-kubernetes-master                 200m (10%)    0 (0%)      0 (0%)           0 (0%)         23m
  kube-system                 fluentd-gcp-v3.2.0-gj25w                      100m (5%)     1 (50%)     200Mi (2%)       500Mi (6%)     23m
  kube-system                 konnectivity-server-kubernetes-master         25m (1%)      0 (0%)      0 (0%)           0 (0%)         22m
  kube-system                 kube-addon-manager-kubernetes-master          5m (0%)       0 (0%)      50Mi (0%)        0 (0%)         22m
  kube-system                 kube-apiserver-kubernetes-master              250m (12%)    0 (0%)      0 (0%)           0 (0%)         23m
  kube-system                 kube-controller-manager-kubernetes-master     200m (10%)    0 (0%)      0 (0%)           0 (0%)         23m
  kube-system                 kube-scheduler-kubernetes-master              75m (3%)      0 (0%)      0 (0%)           0 (0%)         23m
  kube-system                 l7-lb-controller-kubernetes-master            10m (0%)      0 (0%)      50Mi (0%)        0 (0%)         22m
Allocated resources:
  (Total limits may be over 100 percent, i.e., overcommitted.)
  Resource                   Requests     Limits
  --------                   --------     ------
  cpu                        1165m (58%)  1 (50%)
  memory                     300Mi (4%)   500Mi (6%)
  ephemeral-storage          0 (0%)       0 (0%)
  hugepages-2Mi              0 (0%)       0 (0%)
  attachable-volumes-gce-pd  0  

edit: Previously konnectivity-server-kubernetes-master was no being run on the master so there was 25m less requested CPU.

We're 165m over the n1-standard-1 limit.

I'll decrease cloud-controller-manager-kubernetes-master to 50m (after discussing briefly with @cheftako) and fluentd-gcp to 75m (which was not explicitly set and I suspect this could be decreased further).

@jpbetz jpbetz force-pushed the cluster-replace-experiment branch from 4e1cf93 to b1a71bd Compare May 10, 2021 23:13
@jpbetz
Copy link
Contributor Author

jpbetz commented May 10, 2021

/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 May 10, 2021
@cheftako
Copy link
Member

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, jpbetz

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

NODE_SIZE=${NODE_SIZE:-n1-standard-2}
NUM_NODES=${NUM_NODES:-3}
NUM_WINDOWS_NODES=${NUM_WINDOWS_NODES:-0}
# TODO: Migrate to e2-standard machine family.
MASTER_SIZE=${MASTER_SIZE:-n1-standard-$(get-master-size)}
Copy link
Member

Choose a reason for hiding this comment

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

So that may turn out to be n1-standard-2 or -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good find. Looks like for 3 node tests we should expect n1-standard-1:

function get-master-size {

@k8s-ci-robot k8s-ci-robot merged commit 3ab8427 into kubernetes:master May 10, 2021
@jpbetz
Copy link
Contributor Author

jpbetz commented May 11, 2021

Figured out why this PR was triggering CPU requested resource limit exceeded problems.

I hadn't merged in this change to default to a n1-standard-2 for masters:

local suggested_master_size=2

We are better off, I think, it establishing some lower limits and allowing n1-standard-1 masters to continue to be used, so I'm not going to reapply that change, but I wanted to make record of it on this PR.

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
6 participants