[WIP] Migrate to CRDs by vikaschoudhary16 · Pull Request #61 · openshift/cluster-api-provider-libvirt

vikaschoudhary16 · 2018-10-12T10:50:51Z

Vendor CRD base cluster api
Make code changes to use new cluster-api and build it.
Local testing to verify if machine object creation using kubectl results into spawning of libvirt instance
Local testing to verify if deleting machine object, triggers libvirt instance deletion
Handle LibvirtMachineProviderStatus
Fix Makefile and Dockerfile
Fix test cases
Update machine object status

command to build at the moment:

go build -o bin/manager github.com/openshift/cluster-api-provider-libvirt/cmd/manager

Local testing is done using these steps:

Launch cluster using minikube
Create cluster-api-bootstarp.yml

[vikas@fed-node cluster-api-provider-libvirt]$ kustomize build vendor/sigs.k8s.io/cluster-api/config/default/ >> /home/vikas/cluster-api-bootstrap.yaml

kubectl apply -f /home/vikas/cluster-api-bootstrap.yaml
Run libvirt actuator binary:

bin/manager --kubeconfig=/root/.kube/config --alsologtostderr

Create a cluster object using kubectl

apiVersion: cluster.k8s.io/v1alpha1
kind: Cluster
metadata:
  labels:
    controller-tools.k8s.io: "1.0"
  name: cluster-sample
spec:
  clusterNetwork:
    pods:
      cidrBlocks:
      - "167.1.1.0/24"
    services:
      cidrBlocks:
      - "70.1.1.0/24"
    serviceDomain: "abc.com"

Create a machine object using kubectl

---
apiVersion: cluster.k8s.io/v1alpha1
kind: Machine
metadata:
  labels:
    controller-tools.k8s.io: "1.0"
  name: machine-sample2
spec:
  providerConfig:
    ValueFrom: null
    value:
      apiVersion: libvirtproviderconfig/v1alpha1
      autostart: false
      domainMemory: 2048
      domainVcpu: 2
      ignKey: "/var/lib/libvirt/images/worker.ign"
      kind: LibvirtMachineProviderConfig
      networkInterfaceAddress: 192.168.124.0/24
      networkInterfaceName: tes
      uri: qemu+tcp://192.168.122.1/system
      volume:
        baseVolumeID: "/var/lib/libvirt/images/coreos_base"
        poolName: default
  versions:
    kubelet: ''

Verify results using virsh commands.

paulfantom · 2018-10-12T11:49:15Z

-COPY --from=builder /go/bin/machine-controller /go/bin/controller-manager /
+WORKDIR /root/
+COPY --from=builder /go/src/github.com/openshift/cluster-api-provider-libvirt/manager .
+ENTRYPOINT ["./manager"]


Why we are adding entrypoint?

There are still parts to be cleaned up. Dockerfile nad Makefile i mentioned explicitly in description. Will cleanup soon.

paulfantom · 2018-10-12T11:50:13Z


 # Final container
 FROM openshift/origin-base
-RUN yum install -y ca-certificates libvirt-libs openssh-clients


Aren't those libraries needed to connect to libvirt hypervisor via qemu+ssh method?

paulfantom · 2018-10-12T11:50:48Z

-RUN mkdir -p /go/src/github.com/openshift/cluster-api-provider-libvirt
+# Copy in the go src
 WORKDIR /go/src/github.com/openshift/cluster-api-provider-libvirt
+COPY pkg/    pkg/


What does it do?

paulfantom · 2018-10-12T11:53:06Z

+
+# Run go vet against code
+vet:
+	go vet ./pkg/... ./cmd/...


Why this changed from using hack/go-vet.sh script?

paulfantom · 2018-10-12T11:53:34Z

+
+# Run go fmt against code
+fmt:
+	go fmt ./pkg/... ./cmd/...


Why this changed from using hack/go-fmt.sh script?

paulfantom · 2018-10-12T11:53:58Z

-.PHONY: all
-all: build images check
-
-NO_DOCKER ?= 0


Why this is removed?

paulfantom · 2018-10-12T11:54:37Z

-
-.PHONY: test
-test: # Run unit test
-	$(DOCKER_CMD) go test -race -cover ./cmd/... ./cloud/...


Running go should be containerized to ensure local test env is identical with CI one.

Running go should be containerized to ensure local test env is identical with CI one.

I think our Makefile's are upside down. They should favour the developer who types a lot of make commands. In dev, I just want a local binary.

Produced binary is preserved in bin/ directory and our makefiles allow to use non-containerized builds.

paulfantom · 2018-10-18T08:49:19Z

Shouldn't this PR use crd as target branch instead of master?

openshift-ci-robot · 2018-10-18T08:59:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vikaschoudhary16

If they are not already assigned, you can assign the PR to them by writing /assign @vikaschoudhary16 in a comment when ready.

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

The pull request process is described here

Details

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

openshift-ci-robot · 2018-10-18T13:42:09Z

@vikaschoudhary16: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/unit	`6b2f3f0`	link	`/test unit`
ci/jenkins/e2e	`6b2f3f0`	link	`/test e2e`

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.

Details

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.

enxebre · 2018-10-19T08:33:04Z

@vikaschoudhary16 please shout when this is ready for review :)
can you please before rebase into reasonable commits? this is the ones we used on aws for consistency

revendor for CRDs
add kubebuilder generated skeleton
drop old machine controller and build new one
move provider config for new layout
update actuator for using new provider config
update docker and Makefile
fix lints

Previously we suited the layout on master moving the cloud folder into packages
Also you'll want to revendor the cluster api to pick kubernetes-sigs/cluster-api#546
cc @ingvagabund

ingvagabund · 2018-10-19T19:50:08Z

Closing in favor of #72

vikaschoudhary16 · 2018-10-20T11:36:51Z

oh, this was very aggressive overtake.

It was holiday in India on Friday
I was not well throughout week but was still planning to work on this during weekend that i told Alberto too.

ingvagabund · 2018-10-20T11:46:22Z

Sorry about that Vikas :( Alberto restructured his PR during the week with additional changes that we discussed, the e2e framework has changed, some downstream patches were applied. I did not want you to spend time duplicating the same job we did with Alberto.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 12, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2018

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 12, 2018

openshift-ci-robot requested review from paulfantom and spangenberg October 12, 2018 10:51

vikaschoudhary16 force-pushed the crd branch from a125bcd to ca92c1c Compare October 12, 2018 11:36

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2018

vikaschoudhary16 changed the title ~~[WIP] Use CRD based cluster-api~~ [WIP] Migrate to CRDs Oct 12, 2018

paulfantom reviewed Oct 12, 2018

View reviewed changes

paulfantom suggested changes Oct 12, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2018

vikaschoudhary16 added 3 commits October 18, 2018 11:19

Re-Vendoring

fdfce1f

Making kubebuilder project

65ecb15

Fixes for updating machine status

17d41df

vikaschoudhary16 force-pushed the crd branch from 6a57504 to cd1559d Compare October 18, 2018 08:51

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2018

vikaschoudhary16 changed the base branch from master to crd October 18, 2018 08:52

vikaschoudhary16 force-pushed the crd branch from cd1559d to cc51dae Compare October 18, 2018 08:59

Rebased

c8f306f

vikaschoudhary16 force-pushed the crd branch from cc51dae to c8f306f Compare October 18, 2018 11:35

vikaschoudhary16 added 2 commits October 18, 2018 18:08

go-vet

1ee92ed

images

6b2f3f0

ingvagabund closed this Oct 19, 2018

vikaschoudhary16 deleted the crd branch October 22, 2018 04:24

Conversation

vikaschoudhary16 commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Local testing is done using these steps:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulfantom Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frobware Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulfantom commented Oct 18, 2018

Uh oh!

openshift-ci-robot commented Oct 18, 2018

Uh oh!

openshift-ci-robot commented Oct 18, 2018

Uh oh!

enxebre commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ingvagabund commented Oct 19, 2018

Uh oh!

vikaschoudhary16 commented Oct 20, 2018

Uh oh!

ingvagabund commented Oct 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vikaschoudhary16 commented Oct 12, 2018 •

edited

Loading

paulfantom Oct 12, 2018 •

edited

Loading

frobware Oct 12, 2018 •

edited

Loading

enxebre commented Oct 19, 2018 •

edited

Loading