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

building and testing sidecar containers #23

Closed
pohly opened this issue May 8, 2018 · 11 comments
Closed

building and testing sidecar containers #23

pohly opened this issue May 8, 2018 · 11 comments
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@pohly
Copy link
Collaborator

pohly commented May 8, 2018

For new developers it would be useful to document how they can build and test CSI. This could include:

  • building and pushing container images for driver-registrar/hostpath (aka drivers)/external-attacher/external-provisioner
  • running the kubernetes/test/e2e/storage/csi_volumes.go test for hostpath
@pohly
Copy link
Collaborator Author

pohly commented May 8, 2018

Right now, building container images is inconsistent. Some makefiles have a "push" target, some don't. Some build "canary" as version, others the latest stable release.

My proposal is to have the following in all four makefiles:

REGISTRY_NAME=quay.io/k8scsi
IMAGE_NAME=<actual image name>
IMAGE_VERSION=canary
IMAGE_TAG=$(REGISTRY_NAME)/$(IMAGE_NAME):$(IMAGE_VERSION)

container: ....
	docker build -t $(IMAGE_TAG) .

push: container
	docker push $(IMAGE_TAG)

Then one can do:

for i in driver-registrar/ external-attacher/ external-provisioner/ drivers/; do make -C $i REGISTRY_NAME=localhost:5000 push; done

The rationale for only supporting building/pushing of the hostpath container in drivers is that long-term that should be the only image in the repo. Already now it is the only image that gets built by make.

I have that working and can create PRs once there is some positive feedback about the idea (no need to discuss it in four places).

I also have a change ready for kubernetes/test/e2e/storage which makes it possible to change the image version and/or registry:

go run hack/e2e.go -- --provider=local --test --test_args="--ginkgo.focus=CSI.plugin.test.using.CSI.driver..hostPath -csiImageVersion=canary -csiImageRegistry=localhost:5000"

This can be used to test the canary images on quay.io/k8scsi before tagging them as a new release.

@lpabon
Copy link
Member

lpabon commented May 9, 2018

@pohly The drivers repo is going to be dismantled. See kubernetes-retired/drivers#81. AFAIK, the side-car containers are all pushing canary containers into Quay.io.

I guess I would need to understand more why there would be a need to build all at the same time. But I am open to discussion.

Lastly, are the e2e tests using local cluster?

@pohly
Copy link
Collaborator Author

pohly commented May 10, 2018

@pohly The drivers repo is going to be dismantled. See kubernetes-retired/drivers#81.

Yes. That's why I think it is okay to focus on building just the hostpath container image in the "drivers" repo, because that is going to remain.

AFAIK, the side-car containers are all pushing canary containers into Quay.io.

Indeed, they do now. My revision of driver-registrar was a bit old and still had IMAGE_VERSION=v0.2.0.

I guess I would need to understand more why there would be a need to build all at the same time.

Not at the same time, just the same way.

Lastly, are the e2e tests using local cluster?

They can use a local cluster or something remote. I myself run the test with a cluster brought up with local-up-cluster.sh.

pohly added a commit to pohly/driver-registrar that referenced this issue May 11, 2018
This ensures that Makefile variables and the "push" target behave the
same in driver-registrar, drivers, external-attacher and
external-provisioner.

Related-to: kubernetes-csi/docs#23
pohly added a commit to pohly/drivers that referenced this issue May 11, 2018
This ensures that Makefile variables and the "push" target behave the
same in driver-registrar, drivers, external-attacher and
external-provisioner.

Related-to: kubernetes-csi/docs#23
pohly added a commit to pohly/external-attacher that referenced this issue May 11, 2018
This ensures that Makefile variables and the "push" target behave the
same in driver-registrar, drivers, external-attacher and
external-provisioner.

Related-to: kubernetes-csi/docs#23
pohly added a commit to pohly/external-provisioner that referenced this issue May 11, 2018
This ensures that Makefile variables and the "push" target behave the
same in driver-registrar, drivers, external-attacher and
external-provisioner.

Related-to: kubernetes-csi/docs#23
@pohly
Copy link
Collaborator Author

pohly commented May 11, 2018

I have created four PRs, one for each of the repos, where I add the same change: enabling make REGISTRY_NAME=localhost:5000 push

pohly added a commit to pohly/external-attacher that referenced this issue May 14, 2018
This ensures that Makefile variables and the "push" target behave the
same in driver-registrar, drivers, external-attacher and
external-provisioner.

Related-to: kubernetes-csi/docs#23
pohly added a commit to pohly/external-attacher that referenced this issue May 15, 2018
This ensures that Makefile variables and the "push" target behave the
same in driver-registrar, drivers, external-attacher and
external-provisioner.

Related-to: kubernetes-csi/docs#23
@pohly
Copy link
Collaborator Author

pohly commented Jun 6, 2018

I think the best solution for this issue is to proceed with the unification of the different repos, see https://docs.google.com/document/d/1xC0Cm11WTebT9m_9yZbfzud24GJRxlp55gdR5QaMHmM/edit

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue Jun 8, 2018
Automatic merge from submit-queue (batch tested with PRs 60699, 63780). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

e2e/storage: parameterize container images

**What this PR does / why we need it**:

The CSI integration test for hostpath was hard-coded to use the latest
stable release of the sidecar and hostpath container images. This
makes sense for regression testing of changes made in Kubernetes
itself, but the same test is also useful for testing the "canary"
images on quay.io before tagging them as a new release or for testing
locally produced images. Both is now possible via command line
parameters.

**Which issue(s) this PR fixes**:
Related-to: kubernetes-csi/docs#23

**Special notes for your reviewer**:

The commit message has usage instructions.

```release-note
NONE
```

/sig storage
@lpabon
Copy link
Member

lpabon commented Jun 22, 2018

Right now this process is on hold until K8S 1.11 is released.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 24, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 24, 2019
@msau42
Copy link
Collaborator

msau42 commented Jun 7, 2019

are there more things to do for this?

@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

5 participants