Skip to content

Commit

Permalink
Merge pull request #2079 from ConnorJC3/devx-improvements
Browse files Browse the repository at this point in the history
Developer Experience Improvements
  • Loading branch information
k8s-ci-robot committed Jul 5, 2024
2 parents 6015f5d + 58e4226 commit 3ea9041
Show file tree
Hide file tree
Showing 11 changed files with 245 additions and 162 deletions.
119 changes: 87 additions & 32 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,46 +1,101 @@
# Contributing guidelines
# Contributing Guidelines

## Sign the CLA
## Prerequisites

Kubernetes projects require that you sign a Contributor License Agreement (CLA) before we can accept your pull requests. Please see https://git.k8s.io/community/CLA.md for more info

## Contributing A Patch
If you are new to CSI, please go through the [CSI Spec](https://github.com/container-storage-interface/spec/blob/master/spec.md) and [General CSI driver development guideline](https://kubernetes-csi.github.io/docs/developing.html) to get some basic understanding of CSI driver before you start.

1. Submit an issue describing your proposed change to the repo in question.
1. The [repo owners](OWNERS) will respond to your issue promptly.
## Contributing a Patch

1. (Optional, but recommended for large changes) Submit an issue describing your proposed change to the repo in question.
1. If your proposed change is accepted, and you haven't already done so, sign a Contributor License Agreement (see details above).
1. Fork the desired repo, develop and test your code changes.
1. Submit a pull request.
1. Fork the desired repo, develop and test your code changes (see "Development Quickstart Guide" below).
1. When your changes are complete - including tests and documentation if necessary, [submit a pull request](https://github.com/kubernetes-sigs/aws-ebs-csi-driver/compare).
1. Reviewer(s) from the [repo owners](OWNERS) will review your PR. Communicate with your reviewers and address their feedback as required.
1. After merge, your change will typically be included in the next minor release of the EBS CSI Driver and associated Helm chart, which is performed monthly.

## Development Quickstart Guide

This guide demonstrates the basic workflow for developing for the EBS CSI Driver. More detailed documentation of the available `make` targets is available in the [Makefile documentation](docs/makefile.md).

### 1. Local development

If your changes are Helm-only, skip to section 2 (manual testing) below after making your changes.

Run `make` to build a driver binary, and thus discover compiler/syntax errors.

When your change is ready for testing, run `make test` to execute the unit test suite for the driver. If you are making a significant change to the driver (such as a bugfix or new feature), add new unit tests or update existing tests where applicable.

### 2. Cluster setup

When your change is ready to test in a real Kubernetes cluster, create a `kops` cluster to test the driver against:
```bash
make cluster/create
```

If your change affects the Windows implementation of the driver, instead create an `eksctl` cluster with Windows nodes:
```bash
export WINDOWS="true"
# Note: CLUSTER_TYPE must be set for all e2e tests and cluster deletion
# Re-export it if necessary (for example, if running tests in a separate terminal tab)
export CLUSTER_TYPE="eksctl"
make cluster/create
```

Export the `KUBECONFIG` environment variable to access the cluster for manual testing:
```bash
eval "$(make cluster/kubeconfig)"
```

To test a change, build the driver image and then install it:
```bash
make cluster/image
# If testing a Helm-based change, you can use HELM_EXTRA_FLAGS
# to set your new paremeters for testing, for example:
# export HELM_EXTRA_FLAGS="--set=controller.newParameter=true"
make cluster/install
```

When you are finished manually testing, uninstall the driver:
```bash
make cluster/uninstall
```

### 3. Automated E2E tests

Before running any E2E tests, you must create a cluster (see step 2) and build an image if you have not already done so:
```bash
make cluster/image
```

## Development
Please go through [CSI Spec](https://github.com/container-storage-interface/spec/blob/master/spec.md) and [General CSI driver development guideline](https://kubernetes-csi.github.io/docs/developing.html) to get some basic understanding of CSI driver before you start.
The driver should not be preinstalled when running E2E tests (the tests will automatically install the driver). If it is, uninstall it first:
```bash
make cluster/uninstall
```

### Requirements
* Golang 1.15.+
* [Ginkgo](https://github.com/onsi/ginkgo) in your PATH for integration testing and end-to-end testing
* Docker 17.05+ for releasing
If you are making a change to the driver, the recommended test suite to run is the external tests:
```bash
# Normal external tests (excluding Windows tests)
make e2e/external
# Additionally, run the windows tests if changing Windows behavior
make e2e/external-windows
```

### Dependency
Dependencies are managed through go module. To build the project simply type: `make`
If you are making a change to the Helm chart, the recommended test suite to run is the Helm `ct` tests:
```bash
make e2e/helm-ct
```

### Testing
* To execute all unit tests, run: `make test`
* To execute sanity test run: `make test-sanity`
* To execute integration tests, run: `make test-integration`
* To execute e2e tests, run: `make test-e2e-single-az` and `make test-e2e-multi-az`
After finishing testing, cleanup your cluster :
```bash
make cluster/delete
```

### Release Process
Please see [Release Process](./docs/release.md).
### 4. Before submitting a PR

**Notes**:
* Sanity tests make sure the driver complies with the CSI specification
* EC2 instance is required to run integration test, since it is exercising the actual flow of creating EBS volume, attaching it and read/write on the disk. See [Integration Testing](./tests/integration/README.md) for more details.
* E2E tests exercises various driver functionalities in Kubernetes cluster. See [E2E Testing](./tests/e2e/README.md) for more details.
Run `make update` to automatically format go source files and re-generate automatically generated files. If `make update` produces any changes, commit them before submitting a PR.

### Helm and manifests
The Helm chart for this project is in the `charts/aws-ebs-csi-driver` directory. The manifests for this project are in the `deploy/kubernetes` directory. All of the manifests except kustomize patches are generated by running `helm template`. This keeps the Helm chart and the manifests in sync.
Run `make verify` to run linters and other similar code checking tools. Fix any issues `make verify` discovers before submitting a PR.

When updating the Helm chart:
* Generate manifests: `make generate-kustomize`
* There are values files in `deploy/kubernetes/values` used for generating some of the manifests
* When adding a new resource template to the Helm chart please update the `generate-kustomize` make target, the `deploy/kubernetes/values` files, and the appropriate kustomization.yaml file(s).
Your PR should contain a full set of unit/E2E tests when applicable. If introducing a change that is not exercised fully by automated testing (such as a new Helm parameter), please provide evidence of the feature working as intended in the PR description.
5 changes: 3 additions & 2 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,15 @@
# for info on BUILDPLATFORM, TARGETOS, TARGETARCH, etc.
FROM --platform=$BUILDPLATFORM golang:1.22 AS builder
WORKDIR /go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver
RUN go env -w GOCACHE=/gocache GOMODCACHE=/gomodcache
COPY go.* .
ARG GOPROXY
RUN go mod download
RUN --mount=type=cache,target=/gomodcache go mod download
COPY . .
ARG TARGETOS
ARG TARGETARCH
ARG VERSION
RUN OS=$TARGETOS ARCH=$TARGETARCH make
RUN --mount=type=cache,target=/gomodcache --mount=type=cache,target=/gocache OS=$TARGETOS ARCH=$TARGETARCH make

FROM public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-csi-ebs:latest-al23 AS linux-al2023
COPY --from=builder /go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/bin/aws-ebs-csi-driver /bin/aws-ebs-csi-driver
Expand Down
18 changes: 11 additions & 7 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@ endif

GO_SOURCES=go.mod go.sum $(shell find pkg cmd -type f -name "*.go")

REGISTRY?=gcr.io/k8s-staging-provider-aws
IMAGE?=$(REGISTRY)/aws-ebs-csi-driver
TAG?=$(GIT_COMMIT)

ALL_OS?=linux windows
ALL_ARCH_linux?=amd64 arm64
ALL_OSVERSION_linux?=al2023
Expand Down Expand Up @@ -100,9 +96,6 @@ update: update/gofmt update/kustomize update/mockgen update/gomod update/shfmt u
verify: verify/govet verify/golangci-lint verify/update
@echo "All verifications passed!"

.PHONY: all-push
all-push: all-image-registry push-manifest

.PHONY: cluster/create
cluster/create: bin/kops bin/eksctl bin/aws bin/gomplate
./hack/e2e/create-cluster.sh
Expand All @@ -119,6 +112,14 @@ cluster/image: bin/aws
cluster/delete: bin/kops bin/eksctl
./hack/e2e/delete-cluster.sh

.PHONY: cluster/install
cluster/install: bin/helm bin/aws
./hack/e2e/install.sh

.PHONY: cluster/uninstall
cluster/uninstall: bin/helm bin/aws
./hack/e2e/uninstall.sh

## E2E targets
# Targets to run e2e tests

Expand Down Expand Up @@ -187,6 +188,9 @@ update-sidecar-dependencies: update-truth-sidecars generate-sidecar-tags update/
## CI aliases
# Targets intended to be executed mostly or only by CI jobs

.PHONY: all-push
all-push: all-image-registry push-manifest

.PHONY: all-push-with-a1compat
all-push-with-a1compat: sub-image-linux-arm64-al2 all-image-registry push-manifest

Expand Down
5 changes: 1 addition & 4 deletions cloudbuild.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,8 @@
timeout: 5400s
steps:
- name: gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20240523-a15ad90fc9
entrypoint: ./hack/prow.sh
entrypoint: ./hack/cloudbuild.sh
env:
- GIT_TAG=${_GIT_TAG}
- PULL_BASE_REF=${_PULL_BASE_REF}
- REGISTRY_NAME=gcr.io/${_STAGING_PROJECT}
- HOME=/root
substitutions:
_STAGING_PROJECT: "k8s-staging-provider-aws"
90 changes: 24 additions & 66 deletions docs/makefile.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,86 +16,24 @@ The `Makefile` has the following dependencies:

All other tools are downloaded for you at runtime.

## Quickstart Guide

This guide demonstrates the basic workflow for developing for the EBS CSI Driver. More detailed documentation of the available `make` targets is available below.

### 1. Local development for the EBS CSI Driver

If your changes are Helm-only, skip to section 2 (Run E2E tests) below after making your changes.

During development, use `make` at any time to build a driver binary, and thus discover compiler errors. When your change is ready to be tested, run `make test` to execute the unit test suite for the driver. If you are making a significant change to the driver (such as a bugfix or new feature), please add new unit tests or update existing tests where applicable.

### 2. Run E2E tests

To create a `kops` cluster to test the driver against:
```bash
make cluster/create
```

If your change affects the Windows implementation of the driver, instead create an `eksctl` cluster with Windows nodes:
```bash
export WINDOWS="true"
# Note: CLUSTER_TYPE must be set for all e2e tests and cluster deletion
# Re-export it if necessary (for example, if running tests in a separate terminal tab)
export CLUSTER_TYPE="eksctl"
make cluster/create
```

If you are making a change to the driver, the recommended test suite to run is the external tests:
```bash
# Normal external tests (excluding Windows tests)
make e2e/external
# Instead, if testing Windows
make e2e/external-windows
```

If you are making a change to the Helm chart, the recommended test suite to run is the Helm `ct` tests:
```bash
make e2e/helm-ct
```

To cleanup your cluster after finishing testing:
```bash
make cluster/delete
```

### 3. Before submitting a PR
## Building

Run `make update` to automatically format go source files and re-generate automatically generated files. If `make update` produces any changes, commit them before submitting a PR.
### `make cluster/image`

Run `make verify` to run linters and other similar code checking tools. Fix any issues `make verify` discovers before submitting a PR.
Build and push a single image of the driver based on the local platform (the same overrides as `make` apply, as well as `OSVERSION` to override container OS version). In most cases, `make all-push` is more suitable. Environment variables are accepted to override the `REGISTRY`, `IMAGE` name, and image `TAG`.

## Building
## Local Development

### `make` or `make bin/aws-ebs-csi-driver` or `make bin/aws-ebs-csi-driver.exe`

Build a binary copy of the EBS CSI Driver for the local platform. This is the default behavior when calling `make` with no target.

The target OS and/or architecture can be overridden via the `OS` and `ARCH` environment variables (for example, `OS=linux ARCH=arm64 make`)

### `make image`

Build and push a single image of the driver based on the local platform (the same overrides as `make` apply, as well as `OSVERSION` to override container OS version). In most cases, `make all-push` is more suitable. Environment variables are accepted to override the `REGISTRY`, `IMAGE` name, and image `TAG`.

### `make all-push`

Build and push a multi-arch image of the driver based on the OSes in `ALL_OS`, architectures in `ALL_ARCH_linux`/`ALL_ARCH_windows`, and OS versions in `ALL_OSVERSION_linux`/`ALL_OSVERSION_windows`. Also supports `REGISTRY`, `IMAGE`, and `TAG`.

## Local Development

### `make test`

Run all unit tests with race condition checking enabled.

### `make test/coverage`

Outputs a filtered version of the each package's unit test coverage profiling via go's coverage tool to a local `coverage.html` file.

### `make test-sanity`

Run the official [CSI sanity tests](https://github.com/kubernetes-csi/csi-test). _Warning: Currently, 3 of the tests are known to fail incorrectly._

### `make verify`

Performs local verification that other than unit tests (linters, manifest updates, etc)
Expand Down Expand Up @@ -198,6 +136,26 @@ eval "$(make cluster/kubeconfig)"

Deletes a cluster created by `make cluster/create`. You must pass the same `CLUSTER_TYPE` and `CLUSTER_NAME` as used when creating the cluster.

### `make cluster/install`

Install the EBS CSI Driver to the cluster via Helm. You must have already run `make cluster/image` to build the image for the cluster, or provide an image of your own.

#### Example: Install the EBS CSI Driver to a cluster for testing

```bash
make cluster/install
```

### `make cluster/uninstall`

Uninstall an installation of the EBS CSI Driver previously installed by `make cluster/install`.

#### Example: Uninstall the EBS CSI Driver

```bash
make cluster/uninstall
```

## E2E Tests

Run E2E tests against a cluster created by `make cluster/create`. You must pass the same `CLUSTER_TYPE` and `CLUSTER_NAME` as used when creating the cluster. You must have already run `make cluster/image` to build the image for the cluster, or provide an image of your own.
Expand Down
2 changes: 1 addition & 1 deletion hack/prow.sh → hack/cloudbuild.sh
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ loudecho "Set up QEMU"
docker run --rm --privileged multiarch/qemu-user-static --reset -p yes

loudecho "Push manifest list containing amazon linux and windows based images to GCR"
export REGISTRY=$REGISTRY_NAME
export IMAGE=gcr.io/k8s-staging-provider-aws/aws-ebs-csi-driver
export TAG=$GIT_TAG
export VERSION=$PULL_BASE_REF
IMAGE=gcr.io/k8s-staging-provider-aws/aws-ebs-csi-driver make -j $(nproc) all-push-with-a1compat
2 changes: 2 additions & 0 deletions hack/e2e/build-image.sh
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ function build_and_push() {
export ALL_ARCH_linux="${IMAGE_ARCH}"
fi
make -j $(nproc) all-push

loudecho "Image pushed to ${IMAGE_NAME}:${IMAGE_TAG}"
}

if [[ "${CREATE_MISSING_ECR_REPO}" == true ]]; then
Expand Down
43 changes: 43 additions & 0 deletions hack/e2e/install.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#!/bin/bash

# Copyright 2024 The Kubernetes Authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# This script deletes a cluster that was created by `create-cluster.sh`
# CLUSTER_NAME and CLUSTER_TYPE are expected to be specified by the caller
# All other environment variables have default values (see config.sh) but
# many can be overridden on demand if needed

set -euo pipefail

BASE_DIR="$(dirname "$(realpath "${BASH_SOURCE[0]}")")"
BIN="${BASE_DIR}/../../bin"

source "${BASE_DIR}/config.sh"
source "${BASE_DIR}/util.sh"

if [ -z "${HELM_VALUES_FILE:-}" ]; then
if [[ "${CLUSTER_TYPE}" == "kops" ]]; then
HELM_VALUES_FILE="${BASE_DIR}/kops/values.yaml"
elif [[ "${CLUSTER_TYPE}" == "eksctl" ]]; then
HELM_VALUES_FILE="${BASE_DIR}/eksctl/values.yaml"
else
echo "Cluster type ${CLUSTER_TYPE} is invalid, must be kops or eksctl" >&2
exit 1
fi
fi

loudecho "Installing driver via ${DEPLOY_METHOD}"
install_driver
loudecho "Sucessfully installed driver via ${DEPLOY_METHOD}"
Loading

0 comments on commit 3ea9041

Please sign in to comment.