kubernetes-retired / cluster-api-provider-nested

Cluster API Provider for Nested Clusters
Apache License 2.0
301 stars 67 forks source link

Change crd apiversion to v1beta1 in virtualcluster #85

Closed Fei-Guo closed 3 years ago

Fei-Guo commented 3 years ago

In my local box, it seems that the envtest brings up a test environment with a Kubernetes APIServer whose CRD API version is v1beta1.

This change fixes the problem and remove the redundant CRD copies.

k8s-ci-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Fei-Guo

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: - ~~[virtualcluster/OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-nested/blob/main/virtualcluster/OWNERS)~~ [Fei-Guo] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
christopherhein commented 3 years ago

Yeah, this is one of the issues right now with the controller-gen created manifests we need ones that don't have the full definition until we move to the version that has the patches.

I wrestled with this a bunch yesterday and it appeared that there are a couple of things we have to fix to make this part work. First the make target expects the manifests in a specific location config/crds/ for yq to mutate them then with envtest I had pointed at config/crd then if you make it so that controller-gen generates the manifests then you'll end up with the objects that don't seem to have the metadata object fields defined and thus when a ClusterVersion is stored it loses the spec.apiserver.metadata.name field which then will make it so that anything in this func https://github.com/kubernetes-sigs/cluster-api-provider-nested/blob/main/virtualcluster/pkg/controller/controllers/provisioner/provisioner_native.go#L164-L208 that uses ssBdl.Name breaks. It's super annoying.

Hopefully you can find a clean way around that.

k8s-ci-robot commented 3 years ago

@Fei-Guo: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-provider-nested-test 670d6cefc2279aa2e7eddad90cd03d33eb296aa6 link /test pull-cluster-api-provider-nested-test

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
Fei-Guo commented 3 years ago

Hopefully you can find a clean way around that.

My current workaround is to use this simple CRD format

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  creationTimestamp: null
  labels:
    controller-tools.k8s.io: "1.0"
  name: clusterversions.tenancy.x-k8s.io
spec:
  group: tenancy.x-k8s.io
  names:
    kind: ClusterVersion
    plural: clusterversions
  scope: Cluster
  validation:
    openAPIV3Schema:
      properties:
        apiVersion:
          type: string
        kind:
          type: string
        metadata:
          type: object
        spec:
          properties:
            apiServer:
              properties:
                metadata:
                  type: object
                service:
                  type: object
                statefulset:
                  type: object
              type: object
            controllerManager:
              properties:
                metadata:
                  type: object
                service:
                  type: object
                statefulset:
                  type: object
              type: object
            etcd:
              properties:
                metadata:
                  type: object
                service:
                  type: object
                statefulset:
                  type: object
              type: object
          type: object
        status:
          type: object
      type: object
  version: v1alpha1
status:
  acceptedNames:
    kind: ""
    plural: ""
  conditions: []
  storedVersions: []

We probably should not define a sts in clusterversion and instead define a Pod template or a sts spec.

christopherhein commented 3 years ago

We probably should not define a sts in clusterversion and instead define a Pod template or a sts spec.

Perfect, that was the change I made to config/crd/ to make it work for the testsuite.

Fei-Guo commented 3 years ago

This is my local test output when using the v1 crd version.

STEP: bootstrapping test environment
2021-06-02T14:52:40.453-0700    DEBUG   controller-runtime.test-env starting control plane  {"api server flags": []}
2021-06-02T14:52:48.273-0700    DEBUG   controller-runtime.test-env installing CRDs
2021-06-02T14:52:48.274-0700    DEBUG   controller-runtime.test-env reading CRDs from path  {"path": "../../../config/crd"}
2021-06-02T14:52:48.282-0700    DEBUG   controller-runtime.test-env read CRDs from file {"file": "cluster.x-k8s.io_clusters.yaml"}
2021-06-02T14:52:48.286-0700    DEBUG   controller-runtime.test-env read CRDs from file {"file": "clusterversion_simple.yaml"}
2021-06-02T14:52:48.290-0700    DEBUG   controller-runtime.test-env read CRDs from file {"file": "tenancy.x-k8s.io_clusterversions.yaml"}
2021-06-02T14:52:48.295-0700    DEBUG   controller-runtime.test-env read CRDs from file {"file": "tenancy.x-k8s.io_virtualclusters.yaml"}
2021-06-02T14:52:48.295-0700    INFO    controller-runtime.test-env there are more than one CRD definitions with the same <Group, Version, Kind, Name>  {"GVKN": {"GVK":{"Group":"apiextensions.k8s.io","Version":"v1beta1","Kind":"CustomResourceDefinition"},"Name":"clusterversions.tenancy.x-k8s.io"}}
2021-06-02T14:52:48.325-0700    DEBUG   controller-runtime.test-env installing CRD  {"crd": "clusters.cluster.x-k8s.io"}
Failure [7.897 seconds]
[BeforeSuite] BeforeSuite
/Users/f.guo/go/src/sigs.k8s.io/cluster-api-provider-nested/virtualcluster/pkg/controller/controllers/suite_test.go:59

  Unexpected error:
      <*meta.NoKindMatchError | 0xc0005c0e80>: {
          GroupKind: {
              Group: "apiextensions.k8s.io",
              Kind: "CustomResourceDefinition",
          },
          SearchedVersions: ["v1"],
      }
      no matches for kind "CustomResourceDefinition" in version "apiextensions.k8s.io/v1"
  occurred

  /Users/f.guo/go/src/sigs.k8s.io/cluster-api-provider-nested/virtualcluster/pkg/controller/controllers/suite_test.go:69
------------------------------
Fei-Guo commented 3 years ago

Let me figure out the real root cause before submitted a PR. Close it.