CSI Cluster Registry and Node Info CRDs #67803
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,11 +31,13 @@ import ( | |
| "net/http" | ||
|
|
||
| "k8s.io/api/core/v1" | ||
| apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
| utilfeature "k8s.io/apiserver/pkg/util/feature" | ||
| cacheddiscovery "k8s.io/client-go/discovery/cached" | ||
| "k8s.io/client-go/dynamic" | ||
| clientset "k8s.io/client-go/kubernetes" | ||
| csiclientset "k8s.io/csi-api/pkg/client/clientset/versioned" | ||
| "k8s.io/kubernetes/pkg/controller" | ||
| endpointcontroller "k8s.io/kubernetes/pkg/controller/endpoint" | ||
| "k8s.io/kubernetes/pkg/controller/garbagecollector" | ||
|
|
@@ -192,9 +194,17 @@ func startAttachDetachController(ctx ControllerContext) (http.Handler, bool, err | |
| if ctx.ComponentConfig.AttachDetachController.ReconcilerSyncLoopPeriod.Duration < time.Second { | ||
| return nil, true, fmt.Errorf("Duration time must be greater than one second as set via command line option reconcile-sync-loop-period.") | ||
| } | ||
| csiClientConfig := ctx.ClientBuilder.ConfigOrDie("attachdetach-controller") | ||
| // csiClient works with CRDs that support json only | ||
| csiClientConfig.ContentType = "application/json" | ||
|
There was a problem hiding this comment. This shouldn't have been necessary. Clients should have priority order content accept types which should result in json being served. What happened when you didn't set this? There was a problem hiding this comment. @liggitt wouldn't have expected this to show up in code using CRD There was a problem hiding this comment. looks like defaulted at I'm not seeing a fallback specified There was a problem hiding this comment.
|
||
|
|
||
| crdClientConfig := ctx.ClientBuilder.ConfigOrDie("attachdetach-controller") | ||
|
|
||
| attachDetachController, attachDetachControllerErr := | ||
| attachdetach.NewAttachDetachController( | ||
| ctx.ClientBuilder.ClientOrDie("attachdetach-controller"), | ||
| csiclientset.NewForConfigOrDie(csiClientConfig), | ||
| apiextensionsclient.NewForConfigOrDie(crdClientConfig), | ||
| ctx.InformerFactory.Core().V1().Pods(), | ||
| ctx.InformerFactory.Core().V1().Nodes(), | ||
| ctx.InformerFactory.Core().V1().PersistentVolumeClaims(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,16 +21,21 @@ package attachdetach | |
| import ( | ||
| "fmt" | ||
| "net" | ||
| "reflect" | ||
| "time" | ||
|
|
||
| "github.com/golang/glog" | ||
| authenticationv1 "k8s.io/api/authentication/v1" | ||
| "k8s.io/api/core/v1" | ||
| apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" | ||
| apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/labels" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/apimachinery/pkg/util/runtime" | ||
| "k8s.io/apimachinery/pkg/util/wait" | ||
| utilfeature "k8s.io/apiserver/pkg/util/feature" | ||
| coreinformers "k8s.io/client-go/informers/core/v1" | ||
| clientset "k8s.io/client-go/kubernetes" | ||
| "k8s.io/client-go/kubernetes/scheme" | ||
|
|
@@ -39,6 +44,8 @@ import ( | |
| kcache "k8s.io/client-go/tools/cache" | ||
| "k8s.io/client-go/tools/record" | ||
| "k8s.io/client-go/util/workqueue" | ||
| csiapiv1alpha1 "k8s.io/csi-api/pkg/apis/csi/v1alpha1" | ||
| csiclient "k8s.io/csi-api/pkg/client/clientset/versioned" | ||
| "k8s.io/kubernetes/pkg/cloudprovider" | ||
| "k8s.io/kubernetes/pkg/controller" | ||
| "k8s.io/kubernetes/pkg/controller/volume/attachdetach/cache" | ||
|
|
@@ -47,6 +54,7 @@ import ( | |
| "k8s.io/kubernetes/pkg/controller/volume/attachdetach/reconciler" | ||
| "k8s.io/kubernetes/pkg/controller/volume/attachdetach/statusupdater" | ||
| "k8s.io/kubernetes/pkg/controller/volume/attachdetach/util" | ||
| "k8s.io/kubernetes/pkg/features" | ||
| "k8s.io/kubernetes/pkg/util/mount" | ||
| "k8s.io/kubernetes/pkg/volume" | ||
| volumeutil "k8s.io/kubernetes/pkg/volume/util" | ||
|
|
@@ -96,6 +104,8 @@ type AttachDetachController interface { | |
| // NewAttachDetachController returns a new instance of AttachDetachController. | ||
| func NewAttachDetachController( | ||
| kubeClient clientset.Interface, | ||
| csiClient csiclient.Interface, | ||
| crdClient apiextensionsclient.Interface, | ||
| podInformer coreinformers.PodInformer, | ||
| nodeInformer coreinformers.NodeInformer, | ||
| pvcInformer coreinformers.PersistentVolumeClaimInformer, | ||
|
|
@@ -122,6 +132,8 @@ func NewAttachDetachController( | |
| // deleted (probably can't do this with sharedInformer), etc. | ||
| adc := &attachDetachController{ | ||
| kubeClient: kubeClient, | ||
| csiClient: csiClient, | ||
| crdClient: crdClient, | ||
| pvcLister: pvcInformer.Lister(), | ||
| pvcsSynced: pvcInformer.Informer().HasSynced, | ||
| pvLister: pvInformer.Lister(), | ||
|
|
@@ -135,6 +147,11 @@ func NewAttachDetachController( | |
| pvcQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "pvcs"), | ||
| } | ||
|
|
||
| // Install required CSI CRDs on API server | ||
| if utilfeature.DefaultFeatureGate.Enabled(features.CSICrdAutoInstall) { | ||
|
There was a problem hiding this comment. How does this get uninstalled on a rollback? There was a problem hiding this comment. We thought about downgrade and decided not to have an automated mechanism to uninstall on downgrade. Even if the CRD (and any CRs) exist, the older versions will simply ignore it. |
||
| adc.installCRDs() | ||
| } | ||
|
|
||
| if err := adc.volumePluginMgr.InitPlugins(plugins, prober, adc); err != nil { | ||
| return nil, fmt.Errorf("Could not initialize volume plugins for Attach/Detach Controller: %+v", err) | ||
| } | ||
|
|
@@ -237,6 +254,14 @@ type attachDetachController struct { | |
| // the API server. | ||
| kubeClient clientset.Interface | ||
|
|
||
| // csiClient is the client used to read/write csi.storage.k8s.io API objects | ||
| // from the API server. | ||
| csiClient csiclient.Interface | ||
|
|
||
| // crdClient is the client used to read/write apiextensions.k8s.io objects | ||
| // from the API server. | ||
| crdClient apiextensionsclient.Interface | ||
|
|
||
| // pvcLister is the shared PVC lister used to fetch and store PVC | ||
| // objects from the API server. It is shared with other controllers and | ||
| // therefore the PVC objects in its store should be treated as immutable. | ||
|
|
@@ -642,6 +667,65 @@ func (adc *attachDetachController) processVolumesInUse( | |
| } | ||
| } | ||
|
|
||
| // installCRDs creates the specified CustomResourceDefinition for the CSIDrivers object. | ||
| func (adc *attachDetachController) installCRDs() error { | ||
|
There was a problem hiding this comment. I am a little concerned about the implications of controller-manager doing schema changes when it starts up. Not claiming it's definitely problematic, just still thinking about it. There was a problem hiding this comment. Ya, we're doing this for alpha because it's the path of least resistance. For beta, I was talking to @timstclair and we were thinking about having a new/dedicated controller that is responsible for installing CRDs (and reinstalling them if they are uninstalled, e.g. like add on manager). Can revisit that for beta. There was a problem hiding this comment. Should we just use addons for alpha? I drafted those changes here: tallclair@a58008e There was a problem hiding this comment.
That attitude is concerning. Constructing APIs via CRD should not lower our standards for design or make us more willing to ship things we haven't considered the implications of. |
||
| crd := &apiextensionsv1beta1.CustomResourceDefinition{ | ||
|
There was a problem hiding this comment. I think the definition files should be stored in yaml format in a file. I don't think it's a good idea to construct them at runtime, it's too easy to make a change to the code and not realize you're introducing a compatibility problem. There was a problem hiding this comment. Noted. When we move this to a more permanent solution in beta (see #67803 (comment)), will consider this as well. There was a problem hiding this comment.
Why was this comment not considered necessary to address? I am somewhat dismayed by the attitude (echoed in the follow up PR) that the issues raised here don't matter for alpha. |
||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: csiapiv1alpha1.CsiDriverResourcePlural + "." + csiapiv1alpha1.GroupName, | ||
| }, | ||
| Spec: apiextensionsv1beta1.CustomResourceDefinitionSpec{ | ||
| Group: csiapiv1alpha1.GroupName, | ||
| Version: csiapiv1alpha1.SchemeGroupVersion.Version, | ||
| Scope: apiextensionsv1beta1.ClusterScoped, | ||
| Names: apiextensionsv1beta1.CustomResourceDefinitionNames{ | ||
| Plural: csiapiv1alpha1.CsiDriverResourcePlural, | ||
| Kind: reflect.TypeOf(csiapiv1alpha1.CSIDriver{}).Name(), | ||
| }, | ||
| }, | ||
|
There was a problem hiding this comment. Why not use the There was a problem hiding this comment. Oh nice, I didn't realize validation was already supported. Looks like auto generation of OpenAPIV3Schema from types.go is still in the works kubernetes/kube-openapi#37. Before we move to beta, hopefully that is ready and we can use it. If not, we'll just construct the Validation schema manually then. |
||
| } | ||
| res, err := adc.crdClient.ApiextensionsV1beta1().CustomResourceDefinitions().Create(crd) | ||
|
|
||
| if err != nil && !apierrors.IsAlreadyExists(err) { | ||
| glog.Errorf("failed to create CSIDrivers CRD: %#v, err: %#v", | ||
| res, err) | ||
| } else if apierrors.IsAlreadyExists(err) { | ||
|
There was a problem hiding this comment. is it safe to pass a nil error to this function? There was a problem hiding this comment. Good point. Cleaned up the block to prevent that. |
||
| glog.Warningf("CSIDrivers CRD already exists: %#v, err: %#v", | ||
| res, err) | ||
| } else { | ||
| glog.Infof("CSIDrivers CRD created successfully: %#v", | ||
| res) | ||
| } | ||
|
|
||
| crd = &apiextensionsv1beta1.CustomResourceDefinition{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: csiapiv1alpha1.CsiNodeInfoResourcePlural + "." + csiapiv1alpha1.GroupName, | ||
| }, | ||
| Spec: apiextensionsv1beta1.CustomResourceDefinitionSpec{ | ||
| Group: csiapiv1alpha1.GroupName, | ||
| Version: csiapiv1alpha1.SchemeGroupVersion.Version, | ||
| Scope: apiextensionsv1beta1.ClusterScoped, | ||
| Names: apiextensionsv1beta1.CustomResourceDefinitionNames{ | ||
| Plural: csiapiv1alpha1.CsiNodeInfoResourcePlural, | ||
| Kind: reflect.TypeOf(csiapiv1alpha1.CSINodeInfo{}).Name(), | ||
| }, | ||
| }, | ||
| } | ||
| res, err = adc.crdClient.ApiextensionsV1beta1().CustomResourceDefinitions().Create(crd) | ||
|
|
||
| if err != nil && !apierrors.IsAlreadyExists(err) { | ||
| glog.Errorf("failed to create CSINodeInfo CRD: %#v, err: %#v", | ||
| res, err) | ||
| } else if apierrors.IsAlreadyExists(err) { | ||
| glog.Warningf("CSINodeInfo CRD already exists: %#v, err: %#v", | ||
| res, err) | ||
| } else { | ||
| glog.Infof("CSINodeInfo CRD created successfully: %#v", | ||
| res) | ||
| } | ||
|
|
||
| return nil | ||
|
There was a problem hiding this comment. How well will the rest of the system cope if only one of these is installed (say the other one has a transient error)? There was a problem hiding this comment. Different parts of the volume plugin provisioning/attach/mount process look at CRs for one of these CRDs. If one of the CRDs fails to create, it would just change where in the provision/attach/mount process subsequent failure will happen. Since it is better to fail fast, I changed the errors here to return error instead of silently log. |
||
| } | ||
|
|
||
| // VolumeHost implementation | ||
| // This is an unfortunate requirement of the current factoring of volume plugin | ||
| // initializing code. It requires kubelet specific methods used by the mounting | ||
|
|
@@ -751,3 +835,7 @@ func (adc *attachDetachController) GetNodeName() types.NodeName { | |
| func (adc *attachDetachController) GetEventRecorder() record.EventRecorder { | ||
| return adc.recorder | ||
| } | ||
|
|
||
| func (adc *attachDetachController) GetCSIClient() csiclient.Interface { | ||
| return adc.csiClient | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really a good idea to crashloop controller manager if this fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could log an error here, but eventually when it is needed, it will fail. So I think, it's better to fail loudly at initialization if something is wrong, rather then at some point during runtime. And this seems like the standard pattern for config initialization for controllers.