🌱 test/framework: improve the way we inject the ci-artifacts script #4420

sbueringer · 2021-04-01T06:56:37Z

What this PR does / why we need it:

This PR changes the way we patch the ci-artifacts.sh script. Up until now we used strategic merge patch. This overwrote the files and preKubeadmCommands arrays. That was the reason this functionality could not be reused in CAPO and CAPZ (kubernetes-sigs/cluster-api-provider-azure#1018 (comment)). The new version is implemented with JSON patch and just appends the script to the arrays. Because we append the script, it's also possible to run some provider-specific scripts before.

Further improvements:

only download control plane images on control plane nodes
use wget to download artifacts instead of gsutil so save a few minutes because we don't have to install gsutil

Some more details on the linked issue.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #4386

Notes:

I removed the bindata the same way as in 🌱Remove go-bindata #4378, as it lead to test errors because it got out of sync.

sbueringer · 2021-04-01T07:02:27Z

test/framework/kubernetesversions/data/debian_injection_script_control_plane.envsubst.sh

-    DEBIAN_FRONTEND=noninteractive apt-get install -y apt-transport-https curl
+    export DEBIAN_FRONTEND=noninteractive
+    # sometimes the network is not immediately available, so we have to retry the apt-get update
+    retry 10 5 "apt-get update"


I had to do this because the network did not immediately work after boot in our case. It's fine if we don't want this in the main repo, I can move it to a provider-specific script which is run before in CAPO.

sbueringer · 2021-04-01T07:05:23Z

test/framework/kubernetesversions/template.go

-	//go:embed data/kustomization.yaml
-	kustomizationYamlBytes []byte
+	//go:embed data/debian_injection_script_control_plane.envsubst.sh
+	debianInjectionScriptControlPlaneBytes string


An alternative to duplicating the script would have been to template the images we pull (that's the only difference between the two scripts). I just thought another layer of templating would be a bad idea.

I'm not sure if there is a good way to auto-discover if the script runs on a control plane or worker node.

sbueringer · 2021-04-01T07:08:08Z

test/framework/kubernetesversions/template.go

+// * scriptPath: is the path where the script will be stored at
+// * scriptContent: content of the script
+func generateInjectScriptJSONPatch(sourceTemplate []byte, objectKind, objectName, jsonPatchPathPrefix, scriptPath, scriptContent string) ([]byte, error) {
+	filesPathExists, preKubeadmCommandsPathExists, err := checkIfArraysAlreadyExist(sourceTemplate, objectKind, objectName, jsonPatchPathPrefix)


This is where it gets pretty hacky. I couldn't find a way via JSON patch to just "create an array if it isn't there". That's why I have to check first if the arrays already exist and only add them if they don't exist before.

I could have always set the arrays to an empty array but this would have overwritten every preexisting array elements.

I thought about just parsing the relevant objects, changing them and generating a JSON patch for the diff, maybe that would be better.

What do you think?

CecileRobertMichon · 2021-04-02T16:53:54Z

cc @chewong @jackfrancis

vincepri

/lgtm
/assign @fabriziopandini

sbueringer · 2021-05-11T19:59:24Z

Would be also good to get a review from some CAPA folks as CAPA is using the old version of this util at the moment.

P.S. Fyi, I'm using a 100% exact copy of this package in CAPO on the main branch and it works fine there.

sbueringer · 2021-05-24T19:32:20Z

@fabriziopandini @vincepri @CecileRobertMichon What do you think we should do to get this PR merged? :) Anyone to cc/ping for further review?

vincepri · 2021-05-25T14:36:03Z

cc @sedefsavas @randomvariable @yastij

CecileRobertMichon · 2021-05-26T00:27:41Z

lgtm

waiting for someone from CAPA to review

sedefsavas · 2021-05-26T21:48:56Z

@sbueringer
My only comment is debian_injection_script_control_plane.envsubst.sh and debian_injection_script_worker.envsubst.sh are exactly same except this difference:

declare -a CONTAINERS_TO_TEST=("kube-proxy")
declare -a CONTAINERS_TO_TEST=("kube-apiserver" "kube-controller-manager" "kube-proxy" "kube-scheduler")

For the sake of maintaining them easier, is there a way to combine these?

randomvariable · 2021-07-13T09:52:33Z

/lgtm

randomvariable · 2021-07-13T09:53:07Z

Let's rebase on top of #4922 and then I can test it again.

sbueringer · 2021-07-13T11:16:24Z

Included the changes from #4922 and squashed.

randomvariable · 2021-07-13T11:46:24Z

testing now

randomvariable · 2021-07-13T12:01:14Z

Seems to be working

I would amend your commit to add @jackfrancis as a co-author and then can lgtm.

randomvariable · 2021-07-13T12:04:37Z

Here's the generated CAPA template https://gist.github.com/randomvariable/0b8e4d4334fcaf120c2ad902b1ff4510

Signed-off-by: Stefan Büringer buringerst@vmware.com Co-authored-by: Jack Francis <jack.francis@microsoft.com>

sbueringer · 2021-07-13T13:23:16Z

Seems to be working

I would amend your commit to add @jackfrancis as a co-author and then can lgtm.

Good point, should be correct now.

randomvariable · 2021-07-13T13:54:17Z

/lgtm

CecileRobertMichon

/approve

k8s-ci-robot · 2021-07-13T16:31:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spiffxp · 2021-07-16T21:25:32Z

ref: kubernetes/k8s.io#2318

If you're not going to use gsutil anymore, I've got another suggestion I'll PR up shortly

spiffxp · 2021-07-16T21:54:47Z

Opened #4958 (not using gsutil means we don't need to reference GCS bucket names anymore, which would be awesome)

sbueringer · 2021-07-17T05:53:47Z

ref: kubernetes/k8s.io#2318

If you're not going to use gsutil anymore, I've got another suggestion I'll PR up shortly

Nice, I removed gsutil because it took a bit long to install, so that wasn't really worth it. Good to see that it also helps with getting rid of references to the bucket

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 1, 2021

k8s-ci-robot requested review from CecileRobertMichon and justinsb April 1, 2021 06:56

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 1, 2021

sbueringer force-pushed the pr-improve-inject-debian-script branch from 0f420ff to 8c42b45 Compare April 1, 2021 07:00

sbueringer commented Apr 1, 2021

View reviewed changes

sbueringer force-pushed the pr-improve-inject-debian-script branch from 8c42b45 to c552b88 Compare April 1, 2021 07:10

sbueringer changed the title ~~[WIP] 🌱 test/framework: improves the way we inject the debian script~~ 🌱 test/framework: improves the way we inject the debian script Apr 1, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2021

sbueringer changed the title ~~🌱 test/framework: improves the way we inject the debian script~~ 🌱 test/framework: improve the way we inject the ci-artifacts script Apr 1, 2021

sbueringer force-pushed the pr-improve-inject-debian-script branch from c552b88 to 90a96a2 Compare April 1, 2021 07:23

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 1, 2021

sbueringer force-pushed the pr-improve-inject-debian-script branch 2 times, most recently from 36a0f59 to 2eb0e2b Compare May 7, 2021 19:09

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 7, 2021

vincepri approved these changes May 11, 2021

View reviewed changes

k8s-ci-robot assigned fabriziopandini and vincepri May 11, 2021

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

k8s-ci-robot assigned randomvariable Jul 13, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2021

sbueringer force-pushed the pr-improve-inject-debian-script branch from ba9c0a5 to 0c3182f Compare July 13, 2021 11:15

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2021

randomvariable mentioned this pull request Jul 13, 2021

🌱 update upstream k8s CI image store reference #4922

Closed

sbueringer force-pushed the pr-improve-inject-debian-script branch 2 times, most recently from b816733 to 5f95ba2 Compare July 13, 2021 13:20

test/framework: improves the way we inject the debian script

b2d9539

Signed-off-by: Stefan Büringer buringerst@vmware.com Co-authored-by: Jack Francis <jack.francis@microsoft.com>

sbueringer force-pushed the pr-improve-inject-debian-script branch from 5f95ba2 to b2d9539 Compare July 13, 2021 13:22

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2021

CecileRobertMichon reviewed Jul 13, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2021

k8s-ci-robot merged commit 38fabab into kubernetes-sigs:master Jul 13, 2021

k8s-ci-robot added this to the v0.4 milestone Jul 13, 2021

sbueringer deleted the pr-improve-inject-debian-script branch July 13, 2021 16:59

spiffxp mentioned this pull request Jul 16, 2021

🌱 Use dl.k8s.io instead of hardcoded GCS URIs #4958

Merged

spiffxp mentioned this pull request Jul 16, 2021

Deprecate and migrate away from gs://kubernetes-release-dev and gcr.io/kubernetes-ci-images kubernetes/k8s.io#2318

Closed

42 tasks

jsturtevant mentioned this pull request Jul 22, 2021

Inject Windows custom binaries for use in PRs and running against Kubernetes CI kubernetes-sigs/cluster-api-provider-azure#1388

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 test/framework: improve the way we inject the ci-artifacts script #4420

🌱 test/framework: improve the way we inject the ci-artifacts script #4420

sbueringer commented Apr 1, 2021 •

edited

Loading

sbueringer Apr 1, 2021

sbueringer Apr 1, 2021

sbueringer Apr 1, 2021

CecileRobertMichon commented Apr 2, 2021

vincepri left a comment

sbueringer commented May 11, 2021 •

edited

Loading

sbueringer commented May 24, 2021 •

edited

Loading

vincepri commented May 25, 2021

CecileRobertMichon commented May 26, 2021

sedefsavas commented May 26, 2021

randomvariable commented Jul 13, 2021

randomvariable commented Jul 13, 2021

sbueringer commented Jul 13, 2021

randomvariable commented Jul 13, 2021

randomvariable commented Jul 13, 2021 •

edited

Loading

randomvariable commented Jul 13, 2021

sbueringer commented Jul 13, 2021

randomvariable commented Jul 13, 2021

CecileRobertMichon left a comment

k8s-ci-robot commented Jul 13, 2021

spiffxp commented Jul 16, 2021

spiffxp commented Jul 16, 2021

sbueringer commented Jul 17, 2021 •

edited

Loading

🌱 test/framework: improve the way we inject the ci-artifacts script #4420

🌱 test/framework: improve the way we inject the ci-artifacts script #4420

Conversation

sbueringer commented Apr 1, 2021 • edited Loading

sbueringer Apr 1, 2021

Choose a reason for hiding this comment

sbueringer Apr 1, 2021

Choose a reason for hiding this comment

sbueringer Apr 1, 2021

Choose a reason for hiding this comment

CecileRobertMichon commented Apr 2, 2021

vincepri left a comment

Choose a reason for hiding this comment

sbueringer commented May 11, 2021 • edited Loading

sbueringer commented May 24, 2021 • edited Loading

vincepri commented May 25, 2021

CecileRobertMichon commented May 26, 2021

sedefsavas commented May 26, 2021

randomvariable commented Jul 13, 2021

randomvariable commented Jul 13, 2021

sbueringer commented Jul 13, 2021

randomvariable commented Jul 13, 2021

randomvariable commented Jul 13, 2021 • edited Loading

randomvariable commented Jul 13, 2021

sbueringer commented Jul 13, 2021

randomvariable commented Jul 13, 2021

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 13, 2021

spiffxp commented Jul 16, 2021

spiffxp commented Jul 16, 2021

sbueringer commented Jul 17, 2021 • edited Loading

sbueringer commented Apr 1, 2021 •

edited

Loading

sbueringer commented May 11, 2021 •

edited

Loading

sbueringer commented May 24, 2021 •

edited

Loading

randomvariable commented Jul 13, 2021 •

edited

Loading

sbueringer commented Jul 17, 2021 •

edited

Loading