WIP: build/deploy csi-proxy in host-process container #179

marosset · 2021-11-05T18:40:50Z

Signed-off-by: Mark Rossetti marosset@microsoft.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds functionality to build a container image containing csi-proxy.exe and deploy it as a daemonSet using HostProcess containers

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
I have testing using csi-proxy deployed in a host-process container in conjunction with azurefile-csi-driver and everything is behaving as expected.

Does this PR introduce a user-facing change?:

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

k8s-ci-robot · 2021-11-05T18:40:53Z

@marosset: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

k8s-ci-robot · 2021-11-05T18:40:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marosset
To complete the pull request process, please assign saad-ali after the PR has been reviewed.
You can assign the PR to them by writing /assign @saad-ali in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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 · 2021-11-05T18:40:59Z

Hi @marosset. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

marosset · 2021-11-05T18:43:59Z

I think this should probably get tied into https://github.com/kubernetes-csi/csi-release-tools but I need to formalize myself with that process.

image/Dockerfile

jsturtevant · 2021-11-05T18:46:00Z

image/build-container-image.sh

+if [ $(docker buildx ls | grep -c img-builder) == 0 ]; then
+    docker buildx create --name img-builder 
+fi
+docker buildx use img-builder


might consider cleaning up the image builder:

trap 'docker buildx rm img-builder' EXIT

jsturtevant · 2021-11-05T18:48:12Z

image/build-container-image.sh

+
+export DOCKER_CLI_EXPERIMENTAL=enabled
+if [ $(docker buildx ls | grep -c img-builder) == 0 ]; then
+    docker buildx create --name img-builder 


In a recent update to buildx I had to begin explicitly requesting windows:

docker buildx create --name img-builder --use --platform windows/amd64

jsturtevant · 2021-11-05T18:50:20Z

image/build-container-image.sh

+docker buildx build --platform windows/amd64 --output=type=registry --pull -f Dockerfile --build-arg BASE=mcr.microsoft.com/windows/nanoserver:1809 -t $REGISTRY/csi-proxy:$VERSION-1809 ..
+docker buildx build --platform windows/amd64 --output=type=registry --pull -f Dockerfile --build-arg BASE=mcr.microsoft.com/windows/nanoserver:ltsc2022 -t $REGISTRY/csi-proxy:$VERSION-ltsc2022 ..
+
+docker manifest create $REGISTRY/csi-proxy:$VERSION $REGISTRY/csi-proxy:$VERSION-1809 $REGISTRY/csi-proxy:$VERSION-ltsc2022


In Kubernetes test images we keep these values as separate config: https://github.com/kubernetes/kubernetes/blob/master/test/images/agnhost/BASEIMAGE

I guess we only have 2019 and WS 2022 to deal with now so this isn't an issue

jsturtevant · 2021-11-05T18:51:14Z

image/build-container-image.sh

+docker manifest inspect $REGISTRY/csi-proxy:$VERSION
+docker manifest push $REGISTRY/csi-proxy:$VERSION
+
+


nit: extra lines

mauriciopoppe

This looks interesting, thanks for the PR!

I think this should probably get tied into kubernetes-csi/csi-release-tools but I need to formalize myself with that process.

yeah, we use release-tools/build.make, it already handles creating a multiarch binary too for many Windows platforms, I'd create a Dockerfile.Windows file like https://github.com/kubernetes-csi/node-driver-registrar/blob/master/Dockerfile.Windows and modify the root Makefile to call push-multiarch instead

mauriciopoppe · 2021-11-05T19:16:09Z

deploy/csi-proxy-ds.yaml

+      hostNetwork: true
+      containers:
+        - name: csi-proxy
+          image: mrosse3/csi-proxy:v1.0.2


should be k8s.gcr.io/sig-storage/csi-proxy before the merge

mauriciopoppe · 2021-11-05T21:05:53Z

/ok-to-test

ddebroy

This is great! It would be nice to align with release-tools and a structure like https://github.com/kubernetes-csi/csi-driver-smb/tree/master/deploy .

ddebroy · 2021-11-05T23:34:02Z

deploy/csi-proxy-ds.yaml

+kind: DaemonSet
+metadata:
+  name: csi-proxy
+  namespace: kube-system # Is this the right namespace?


Looks good based on https://github.com/kubernetes-csi/csi-driver-smb/blob/master/deploy/csi-smb-node.yaml#L6 and https://github.com/kubernetes-csi/csi-driver-nfs/blob/master/deploy/csi-nfs-node.yaml#L8

deploy/csi-proxy-ds.yaml

marosset · 2021-11-08T22:36:17Z

Thanks all, let me look into aligning with the release-tools.

marosset · 2021-11-08T23:37:03Z

@mauriciopoppe I made some updates and ran make push-multiarch PULL_BASE_REF=master REGISTRY_NAME=mrosse3 BUILD_PLATFORMS="windows amd64 .exe nanoserver:1809; windows amd64 .exe nanoserver:ltsc2022".

The image it produced looks good - https://hub.docker.com/layers/mrosse3/csi-proxy/canary/images/sha256-e085d3942a3bef9c9f4ffcd6f534bc75f1d18fcca4cec3dc3093c7369bc7ab7f?context=explore

Is there anything else I need to do to integrate w/ the release tools?
This was a lot more straight-forward than I was expecting!

mauriciopoppe · 2021-11-09T17:38:48Z

@marosset I've been reading the scripts related with our build process, we have two:

I think the overall objective is to call the function gcr_cloud_build declared in release-tools/prow.sh, that function takes care of building a multiarch image, however we shouldn't break the code that generates the standalone binary too as this is another release artifact.

For the presubmit job the file used is .prow.sh which is sourcing release-tools/prow.sh and calling run_with_go, I think that .prow.sh should use the function gcr_cloud_build instead of the two run_with_go calls, gcr_cloud_build calls run_with_go under the hood.

For the postsubmit job the file used is cloudbuild.yaml which is calling .cloudbuild.sh, in other CSI repos the file .cloudbuild.sh is a symlink to release-tools/cloudbuild.sh which is calling gcr_cloud_build too, we might make it a symlink or call the same functions as release-tools/cloudbuild.sh.

marosset · 2021-11-11T18:31:17Z

@marosset I've been reading the scripts related with our build process, we have two:

presubmit: https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes-csi/csi-proxy/csi-proxy-config.yaml

postsubmit: https://github.com/kubernetes/test-infra/blob/7180a0f5fab405667c76a7c0a76af9fe260c5c74/config/jobs/image-pushing/k8s-staging-sig-storage.yaml#L559-L595

I think the overall objective is to call the function gcr_cloud_build declared in release-tools/prow.sh, that function takes care of building a multiarch image, however we shouldn't break the code that generates the standalone binary too as this is another release artifact.

For the presubmit job the file used is .prow.sh which is sourcing release-tools/prow.sh and calling run_with_go, I think that .prow.sh should use the function gcr_cloud_build instead of the two run_with_go calls, gcr_cloud_build calls run_with_go under the hood.

For the postsubmit job the file used is cloudbuild.yaml which is calling .cloudbuild.sh, in other CSI repos the file .cloudbuild.sh is a symlink to release-tools/cloudbuild.sh which is calling gcr_cloud_build too, we might make it a symlink or call the same functions as release-tools/cloudbuild.sh.

Thanks @mauriciopoppe this is very helpful.
Let me make these updates.

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset · 2021-11-11T19:30:27Z

.prow.sh

@@ -14,3 +14,6 @@ ensure_paths
 # main
 run_with_go "${CSI_PROW_GO_VERSION_BUILD}" make all "GOFLAGS_VENDOR=${GOFLAGS_VENDOR}" "BUILD_PLATFORMS=${CSI_PROW_BUILD_PLATFORMS}"
 run_with_go "${CSI_PROW_GO_VERSION_BUILD}" make -k test "GOFLAGS_VENDOR=${GOFLAGS_VENDOR}" 2>&1 | make_test_to_junit
+
+# build / push multi-arch images for validation
+gcr_cloud_build


gcr_cloud_build just calls make push-multiarch so i left in the calls to run_with_go make all and make test.
Should I update the push-multiarch make target to run the tests too so this file can just call gcr_cloud_build and not the run_with_go commands?
That seems a little convoluted to me.

k8s-ci-robot · 2021-11-11T20:16:35Z

@marosset: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-csi-csi-proxy	`498fe4f`	link	true	`/test pull-kubernetes-csi-csi-proxy`

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.

marosset · 2021-11-11T20:27:39Z

@mauriciopoppe is gcr.io/k8s-staging-sig-storage the correct registry to use for presubmit jobs?
When I didn't specify the registry explicitly in .prow.sh the envvar wasn't set in the gcr_cloud_build call.

I'm getting the following errors

#9 [auth] k8s-staging-sig-storage/csi-proxy:pull,push token for gcr.io
#9 DONE 0.0s

#8 exporting to image
#8 pushing layers 2.2s done
#8 ERROR: failed to authorize: failed to fetch oauth token: unexpected status: 403 Forbidden
------
 > exporting to image:
------
error: failed to solve: failed to fetch oauth token: unexpected status: 403 Forbidden
+ docker buildx rm multiarchimage-buildertest
make: *** [release-tools/build.make:149: push-multiarch-csi-proxy] Error 1
+ EXIT_VALUE=2
+ set +o xtrace

mauriciopoppe · 2021-11-30T17:30:40Z

.cloudbuild.sh

-# Extract tag-n-hash value from GIT_TAG (form vYYYYMMDD-tag-n-hash) for REV value.
-REV=v$(echo "$GIT_TAG" | cut -f3- -d 'v')
+: ${CSI_PROW_BUILD_PLATFORMS:="windows amd64 .exe nanoserver:1809; windows amd64 .exe nanoserver:ltsc2022"}
+: ${REGISTRY_NAME:="gcr.io/k8s-staging-sig-storage"}


@marosset so sorry for the late response. I checked what other CSI projects do and none of them push the presubmit artifacts to a remote! They instead just build the linux image locally and run a kind cluster on the location where the prow job runs for the integration tests.

I recently added a multidistro build to kubernetes-sigs/local-volume-provisioner in https://github.com/kubernetes-sigs/sig-storage-local-static-provisioner/pull/279/files, every prow job inherits the project through the env var $PROJECT which can be used for the presubmit image location.

One thing that I saw that might cause problems is that the tag is always canary for master, I think it should have some info about the git tag to avoid stepping on the changes of another contributor but we can fix that later.

This might make the presubmit job pass but I think I might be forgetting about the post submit image push to gcr.io/k8s-staging-sig-storage, I'll check that too

: ${REGISTRY_NAME:="gcr.io/k8s-staging-sig-storage"}

shouldn't be set in this file, this is the file that runs on postsubmit

mauriciopoppe · 2021-11-30T17:48:30Z

.prow.sh

@@ -4,7 +4,8 @@

 # # Only these tests make sense for csi-proxy
 : ${CSI_PROW_TESTS:="unit"}
-: ${CSI_PROW_BUILD_PLATFORMS:="windows amd64 .exe"}
+: ${CSI_PROW_BUILD_PLATFORMS:="windows amd64 .exe nanoserver:1809"}
+: ${REGISTRY_NAME:="gcr.io/k8s-staging-sig-storage"}


This should be:

: ${REGISTRY_NAME:="gcr.io/$PROJECT"}

mauriciopoppe · 2021-11-30T17:49:04Z

.prow.sh

@@ -4,7 +4,8 @@

 # # Only these tests make sense for csi-proxy
 : ${CSI_PROW_TESTS:="unit"}
-: ${CSI_PROW_BUILD_PLATFORMS:="windows amd64 .exe"}
+: ${CSI_PROW_BUILD_PLATFORMS:="windows amd64 .exe nanoserver:1809"}


CSI_PROW_BUILD_PLATFORMS should already have a default, I think we should remove this line and use that default instead

mauriciopoppe · 2021-11-30T17:49:43Z

.prow.sh

@@ -14,3 +15,6 @@ ensure_paths
 # main
 run_with_go "${CSI_PROW_GO_VERSION_BUILD}" make all "GOFLAGS_VENDOR=${GOFLAGS_VENDOR}" "BUILD_PLATFORMS=${CSI_PROW_BUILD_PLATFORMS}"
 run_with_go "${CSI_PROW_GO_VERSION_BUILD}" make -k test "GOFLAGS_VENDOR=${GOFLAGS_VENDOR}" 2>&1 | make_test_to_junit
+
+# build / push multi-arch images for validation
+gcr_cloud_build


for clarification, this file .prow.sh is what runs in the presubmit jobs and .cloudbuild.sh is the one that runs on postsubmit jobs

mauriciopoppe · 2021-12-01T19:23:25Z

@ddebroy, @jingxu97 and I talked about supporting this mode, my opinion is:

having the binary running in a container makes its deployment easier than what it's now, CSI Drivers are in control of creating a bundle with CSI Proxy too and there's no need to do the node bootstrapping downloading the binary and running it as a Windows service.
supporting a new artifact (the image) is harder, every time we create a new release we also have to promote the staging image to the k8s.gcr.io release project, the issue here is that if we push an image we have to support it for the foreseeable future.
this PR is a step in between but might not be needed, HostProcess container is already beta in 1.23, I'd put more effort in modifying this repo to become a library and instead run the CSI Drivers/Addons with the HostProcess container setup.

marosset · 2021-12-01T20:25:21Z

@ddebroy, @jingxu97 and I talked about supporting this mode, my opinion is:

having the binary running in a container makes its deployment easier than what it's now, CSI Drivers are in control of creating a bundle with CSI Proxy too and there's no need to do the node bootstrapping downloading the binary and running it as a Windows service.

supporting a new artifact (the image) is harder, every time we create a new release we also have to promote the staging image to the k8s.gcr.io release project, the issue here is that if we push an image we have to support it for the foreseeable future.

this PR is a step in between but might not be needed, HostProcess container is already beta in 1.23, I'd put more effort in modifying this repo to become a library and instead run the CSI Drivers/Addons with the HostProcess container setup.

I've been out of the office for a bit but am back now.
This makes sense. If we go the library route then we'd need to update many projects - correct?

mauriciopoppe · 2021-12-01T23:39:53Z

This makes sense. If we go the library route then we'd need to update many projects - correct?

Correct, we would:

update this codebase so that the client library that it exposes doesn't use gRPC and instead just uses the server methods
update CSI Drivers and addons to use CSI Proxy as a library

There's a doc that @ddebroy started but I haven't had a chance to complete yet: https://docs.google.com/document/d/15lgXyWytWSg1PuEcHl72eqgLoXc_i8kXmQcrXhRpjO0/edit#heading=h.6vn5bqze2ffs

marosset · 2022-01-12T21:37:31Z

/close
I'm moving this work to github.com/kubernetes-sigs/sig-windows-tools with #179

k8s-ci-robot · 2022-01-12T21:37:33Z

@marosset: Closed this PR.

In response to this:

/close
I'm moving this work to github.com/kubernetes-sigs/sig-windows-tools with #179

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.

WIP: build/deploy csi-proxy in host-process container

46da171

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

k8s-ci-robot requested review from gnufied and PatrickLang November 5, 2021 18:40

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 5, 2021

jsturtevant reviewed Nov 5, 2021

View reviewed changes

mauriciopoppe reviewed Nov 5, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 5, 2021

ddebroy reviewed Nov 5, 2021

View reviewed changes

using release-tools tooling to build/push windows image

9fb739d

Updating pre/post submit jobs to build csi-proxy contaienr image

8ccdb66

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset commented Nov 11, 2021

View reviewed changes

Adding REGISTRY_NAME to .prow.sh and .cloudbuild.sh

498fe4f

mauriciopoppe reviewed Nov 30, 2021

View reviewed changes

marosset mentioned this pull request Dec 13, 2021

[feature request]enable csi-proxy by default on Windows 1.21+ kubernetes-sigs/cluster-api-provider-azure#1814

Closed

k8s-ci-robot closed this Jan 12, 2022

marosset mentioned this pull request Jan 12, 2022

Csi proxy image kubernetes-sigs/sig-windows-tools#203

Merged

3 tasks

andyzhangx mentioned this pull request May 29, 2022

install as host process on windows node and not requiring csi-proxy kubernetes-csi/csi-driver-smb#481

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: build/deploy csi-proxy in host-process container #179

WIP: build/deploy csi-proxy in host-process container #179

marosset commented Nov 5, 2021 •

edited

Loading

k8s-ci-robot commented Nov 5, 2021

k8s-ci-robot commented Nov 5, 2021

k8s-ci-robot commented Nov 5, 2021

marosset commented Nov 5, 2021

jsturtevant Nov 5, 2021

jsturtevant Nov 5, 2021

jsturtevant Nov 5, 2021

jsturtevant Nov 5, 2021

mauriciopoppe left a comment

mauriciopoppe Nov 5, 2021

marosset Nov 5, 2021

mauriciopoppe commented Nov 5, 2021

ddebroy left a comment

ddebroy Nov 5, 2021

marosset commented Nov 8, 2021

marosset commented Nov 8, 2021 •

edited

Loading

mauriciopoppe commented Nov 9, 2021

marosset commented Nov 11, 2021

marosset Nov 11, 2021

k8s-ci-robot commented Nov 11, 2021

marosset commented Nov 11, 2021

mauriciopoppe Nov 30, 2021 •

edited

Loading

mauriciopoppe Nov 30, 2021

mauriciopoppe Nov 30, 2021

mauriciopoppe Nov 30, 2021 •

edited

Loading

mauriciopoppe Nov 30, 2021

mauriciopoppe Nov 30, 2021

mauriciopoppe commented Dec 1, 2021

marosset commented Dec 1, 2021

mauriciopoppe commented Dec 1, 2021

marosset commented Jan 12, 2022

k8s-ci-robot commented Jan 12, 2022

		docker manifest inspect $REGISTRY/csi-proxy:$VERSION
		docker manifest push $REGISTRY/csi-proxy:$VERSION

WIP: build/deploy csi-proxy in host-process container #179

WIP: build/deploy csi-proxy in host-process container #179

Conversation

marosset commented Nov 5, 2021 • edited Loading

k8s-ci-robot commented Nov 5, 2021

k8s-ci-robot commented Nov 5, 2021

k8s-ci-robot commented Nov 5, 2021

marosset commented Nov 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciopoppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciopoppe commented Nov 5, 2021

ddebroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marosset commented Nov 8, 2021

marosset commented Nov 8, 2021 • edited Loading

mauriciopoppe commented Nov 9, 2021

marosset commented Nov 11, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 11, 2021

marosset commented Nov 11, 2021

mauriciopoppe Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciopoppe Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciopoppe commented Dec 1, 2021

marosset commented Dec 1, 2021

mauriciopoppe commented Dec 1, 2021

marosset commented Jan 12, 2022

k8s-ci-robot commented Jan 12, 2022

marosset commented Nov 5, 2021 •

edited

Loading

marosset commented Nov 8, 2021 •

edited

Loading

mauriciopoppe Nov 30, 2021 •

edited

Loading

mauriciopoppe Nov 30, 2021 •

edited

Loading