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

kubemark: fix and enhance kubemark scripts for IKS #76909

Merged
merged 1 commit into from Apr 29, 2019

Conversation

@Huang-Wei
Copy link
Member

commented Apr 22, 2019

What type of PR is this?

/kind bug
/sig scalability
/sig ibmcloud
/cc @rtheis @kitch
/assign @shyamjvs @wojtek-t

What this PR does / why we need it:

  • fix shell script issues
    • bx is deprecated; rename to ibmcloud
    • remove unnecessary variable replacement in hollow-node_template.yaml
    • add replacement logic for HOLLOW_KUBELET_TEST_ARGS and HOLLOW_PROXY_TEST_ARGS
    • don't hardcode KUBEMARK_IMAGE_REGISTRY to brandondr96
  • make cluster number and spec configurable
    • make number and spec of workers configurable
    • separate NUM_NODES and KUBEMARK_NUM_NODES

Special notes for your reviewer:

After #76848 gets merged, users can build their own kubemark image. At this moment, due to kubemark issue #69735, this PR can be verified:

CLOUD_PROVIDER=iks [NUM_NODES=2] [KUBEMARK_NUM_NODES=10] ENABLE_KUBEMARK_CLUSTER_AUTOSCALER=N KUBEMARK_IMAGE_REGISTRY=hweicdl PROJECT=kubemark KUBEMARK_IMAGE_TAG=v1.12.7 test/kubemark/start-kubemark.sh

Does this PR introduce a user-facing change?:

Kubemark scripts have been fixed for IKS clusters.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@Huang-Wei: GitHub didn't allow me to request PR reviews from the following users: rtheis, kitch.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind bug
/sig scalability
/sig ibmcloud
/cc @rtheis @kitch
/assign @shyamjvs @wojtek-t

What this PR does / why we need it:

  • fix shell script issues
  • bx is deprecated; rename to ibmcloud
  • remove unnecessary variable replacement in hollow-node_template.yaml
  • add replacement logic for HOLLOW_KUBELET_TEST_ARGS and HOLLOW_PROXY_TEST_ARGS
  • don't hardcode KUBEMARK_IMAGE_REGISTRY to brandondr96
  • make cluster number and spec configurable
  • make number and spec of workers configurable
  • separate NUM_NODES and KUBEMARK_NUM_NODES

Special notes for your reviewer:

After #76848 gets merged, users can build their own kubemark image. At this moment, due to kubemark issue #69735, this PR can be verified:

CLOUD_PROVIDER=iks [NUM_NODES=2] [KUBEMARK_NUM_NODES=10] ENABLE_KUBEMARK_CLUSTER_AUTOSCALER=N KUBEMARK_IMAGE_REGISTRY=hweicdl PROJECT=kubemark KUBEMARK_IMAGE_TAG=v1.12.7 test/kubemark/start-kubemark.sh

Does this PR introduce a user-facing change?:

Kubemark scripts have been fixed for IKS clusters.

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.

@wojtek-t

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@Huang-Wei - I'm happy to approve it once someone familiar with iks and ibmcloud will lgtm it.

@rtheis
rtheis approved these changes Apr 23, 2019
Copy link

left a comment

lgtm

@@ -25,8 +25,13 @@ CLUSTER_LOCATION="${CLUSTER_LOCATION:-wdc06}"
REGISTRY_LOGIN_URL="${REGISTRY_LOGIN_URL:-https://api.ng.bluemix.net}"

# User defined
# number of real workers in spawnTester cluster

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Apr 23, 2019

Member

Nothing in kubemark should depend on NUM_NODES (as it shouldn't matter on how many nodes it has been run).

Instead, you should do the same that gce is doing:
NUM_NODES=${KUBEMARK_NUM_NODES:-10}
https://github.com/kubernetes/kubernetes/blob/master/cluster/kubemark/gce/config-default.sh#L29

Then you won't need a bunch of changes below too.

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Apr 23, 2019

Author Member

Thanks for the prompt reivew! I read into the code, and noticed GCE works basically as below:

Firstly --gcp-nodes is passed to kubetest, and then it's passed onto env var:

		{
			Env:    "NUM_NODES",
			Option: &o.gcpNodes,
			Name:   "--gcp-nodes",
		},

And then it uses bash (default) deployer to spin up a real cluster, which in turns calls hack/e2e-internal/e2e-up.sh:

func (b *bashDeployer) Up() error {
	script := "./hack/e2e-internal/e2e-up.sh"
    ...

By this step, env var NUM_NODES literally means the number of the real cluster.

After that (the cluster is ready), KUBEMARK_NUM_NODES can overwrite the value of NUM_NODES (as the old value is not necessary any more), and we starts a hollow-node cluster as a rc.

NUM_NODES=${KUBEMARK_NUM_NODES:-10}

So basically GCE works in 2 phases: spin up a cluster and start the hollow-node rc. And they're working in serial, so we can overwrite NUM_NODES in latter phase with KUBEMARK_NUM_NODES.

For IKS, the embarrassing thing is kubetest doesn't support IKS yet, so start-kubemark.sh for IKS combines the 2 phases together. So IMO it's good to separate the 2 variables for IKS; otherwise it'd confuse people which literally means the number of the real cluster, and which means the number of the hollow-node rc.

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Apr 26, 2019

Member

OK - I didn't know you don't have support in kubetest.
In such case, it's probably the easiest thing to do to unblock you.

Can you please add a TODO with a comment why you do that and to change that once kubetest supports IKS?

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Apr 26, 2019

Author Member

That makes sense.

Updated. PTAL.

else
echo -e "${color_red}Invalid response, please try again:${color_norm}"
complete-login
fi
bx cr login
ibmcloud cr login

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Apr 23, 2019

Member

For the future - it would be useful to separate changes like s/bx/ibmcloud/ to a separate PR :-)

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Apr 23, 2019

Author Member

Absolutely. Will do next time.

kubemark: fix and enhance kubemark scripts for IKS
- fix shell script issues
  - `bx` is deprecated; rename to `ibmcloud`
  - remove unnecessay variable replacement in hollow-node_template.yaml
  - add replacement logic for HOLLOW_KUBELET_TEST_ARGS and HOLLOW_PROXY_TEST_ARGS
  - don't hardcode KUBEMARK_IMAGE_REGISTRY to brandondr96
- make cluster number and spec configurable
  - make number and spec of workers configurable
  - separate NUM_NODES and KUBEMARK_NUM_NODES

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:fix-iks-kubemark branch from cd85862 to fbec01d Apr 26, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

/retest

@wojtek-t

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 29, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei, wojtek-t

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 merged commit 9b437f9 into kubernetes:master Apr 29, 2019

20 checks passed

cla/linuxfoundation Huang-Wei authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@Huang-Wei Huang-Wei deleted the Huang-Wei:fix-iks-kubemark branch Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.