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 k-csi gen-jobs to enable different clusters to be used #29812

Merged
merged 1 commit into from
Jun 21, 2023
Merged

Update k-csi gen-jobs to enable different clusters to be used #29812

merged 1 commit into from
Jun 21, 2023

Conversation

rjsadow
Copy link
Contributor

@rjsadow rjsadow commented Jun 14, 2023

There are three parts to this PR:

  1. Provide the ability to define components on a cluster by cluster basis. This change also transition
  2. Increase and standardize the resource quotas. They are likely over tuned right now but can be lowered in the future.
  3. Moves both the external-snapshotter and external-resizer jobs to the EKS cluster.

ref: #29722
ref: #29763

/cc @msau42 @saad-ali @xing-yang @jsafrane @pohly

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/config Issues or PRs related to code in /config area/jobs sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 14, 2023
@rjsadow
Copy link
Contributor Author

rjsadow commented Jun 14, 2023

Should I commit the changes to the job files that were made with the updated gen-jobs.sh? I wasn't sure if there's an automated way to get them updated or just manually running the script.

@pohly
Copy link
Contributor

pohly commented Jun 15, 2023

It's manual, so please include the generated files in this PR.

# Declare an associative array
declare -A cluster_mapping

cluster_mapping["eks-prow-build-cluster"]="external-snapshotter external-resizer"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems complicated. Wouldn't it be simpler to do a switch?

case "$repo" in
   external-snapshotter|external-resizer) echo "eks-prow-build-cluster";;
   *) echo "default"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can update it to a case. Personally, I think the array structure will be more clean when dealing with all of the repos under csi.

Copy link
Contributor

Choose a reason for hiding this comment

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

The variable may be simpler, but the shell code to work with it isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I'll update it and add in the generated files in the next few hours.

# Declare an associative array
declare -A cluster_mapping

cluster_mapping["eks-prow-build-cluster"]="external-snapshotter external-resizer"
Copy link
Member

Choose a reason for hiding this comment

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

why only snapshotter and resizer repos?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose to migrate in steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, I loosely plan on doing three to four follow on PRs to migrate all the repos.

cpu: 4
limits:
memory: "9Gi"
cpu: 4
Copy link
Contributor

Choose a reason for hiding this comment

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

These resources seem too much. The suggestion from #29722 (comment) is to start with 2 CPUs (which mirrors the previous 2000m) and 4GB, then watch https://monitoring-eks.prow.k8s.io/d/96Q8oOOZk/builds?from=now-24h&to=now to see the actual usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original requests made for all csi jobs were 2 CPU and 9 GB. I agree that the 4 cpu and 6 cpu for build jobs is likely a bit high. I'll probably lower it to 2 and 4 CPU respectively.

Copy link
Contributor

Choose a reason for hiding this comment

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

The original requests made for all csi jobs were 2 CPU and 9 GB.

No, not quite. That was only for jobs which need to build Kubernetes from source. Everything else used "cpu: 2000m".

Let's use the original 2 cpu, 9 GiB for build jobs and 2 cpu, 4 GiB for the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks for clarifying! Will update.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 16, 2023
Signed-off-by: Ricky Sadowski <richard.j.sadowski@gmail.com>
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023
@msau42
Copy link
Member

msau42 commented Jun 21, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, rjsadow

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2023
@k8s-ci-robot k8s-ci-robot merged commit 00c007f into kubernetes:master Jun 21, 2023
@k8s-ci-robot
Copy link
Contributor

@rjsadow: Updated the job-config configmap in namespace default at cluster test-infra-trusted using the following files:

  • key csi-driver-host-path-config.yaml using file config/jobs/kubernetes-csi/csi-driver-host-path/csi-driver-host-path-config.yaml
  • key csi-driver-iscsi-config.yaml using file config/jobs/kubernetes-csi/csi-driver-iscsi/csi-driver-iscsi-config.yaml
  • key csi-driver-nfs-config.yaml using file config/jobs/kubernetes-csi/csi-driver-nfs/csi-driver-nfs-config.yaml
  • key csi-driver-nvmf-config.yaml using file config/jobs/kubernetes-csi/csi-driver-nvmf/csi-driver-nvmf-config.yaml
  • key csi-lib-utils-config.yaml using file config/jobs/kubernetes-csi/csi-lib-utils/csi-lib-utils-config.yaml
  • key csi-proxy-config.yaml using file config/jobs/kubernetes-csi/csi-proxy/csi-proxy-config.yaml
  • key csi-release-tools-config.yaml using file config/jobs/kubernetes-csi/csi-release-tools/csi-release-tools-config.yaml
  • key csi-test-config.yaml using file config/jobs/kubernetes-csi/csi-test/csi-test-config.yaml
  • key external-attacher-config.yaml using file config/jobs/kubernetes-csi/external-attacher/external-attacher-config.yaml
  • key external-health-monitor-config.yaml using file config/jobs/kubernetes-csi/external-health-monitor/external-health-monitor-config.yaml
  • key external-provisioner-config.yaml using file config/jobs/kubernetes-csi/external-provisioner/external-provisioner-config.yaml
  • key external-resizer-config.yaml using file config/jobs/kubernetes-csi/external-resizer/external-resizer-config.yaml
  • key external-snapshotter-config.yaml using file config/jobs/kubernetes-csi/external-snapshotter/external-snapshotter-config.yaml
  • key lib-volume-populator-config.yaml using file config/jobs/kubernetes-csi/lib-volume-populator/lib-volume-populator-config.yaml
  • key livenessprobe-config.yaml using file config/jobs/kubernetes-csi/livenessprobe/livenessprobe-config.yaml
  • key node-driver-registrar-config.yaml using file config/jobs/kubernetes-csi/node-driver-registrar/node-driver-registrar-config.yaml
  • key volume-data-source-validator-config.yaml using file config/jobs/kubernetes-csi/volume-data-source-validator/volume-data-source-validator-config.yaml

In response to this:

There are three parts to this PR:

  1. Provide the ability to define components on a cluster by cluster basis. This change also transition
  2. Increase and standardize the resource quotas. They are likely over tuned right now but can be lowered in the future.
  3. Moves both the external-snapshotter and external-resizer jobs to the EKS cluster.

ref: #29722
ref: #29763

/cc @msau42 @saad-ali @xing-yang @jsafrane @pohly

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.

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/config Issues or PRs related to code in /config area/jobs 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. 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.

4 participants