-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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
CSI Cluster Registry and Node Info CRDs #67803
Changes from 5 commits
5ceb26d
bed2c39
c3a2752
c8ff210
7d673cb
fdeb895
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -36,6 +36,7 @@ import ( | |||||||||
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 +193,14 @@ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @liggitt wouldn't have expected this to show up in code using CRD There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like
defaulted at kubernetes/staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1/defaults.go Lines 29 to 31 in 95c99eb
I'm not seeing a fallback specified There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this related to #62175? |
||||||||||
|
||||||||||
attachDetachController, attachDetachControllerErr := | ||||||||||
attachdetach.NewAttachDetachController( | ||||||||||
ctx.ClientBuilder.ClientOrDie("attachdetach-controller"), | ||||||||||
csiclientset.NewForConfigOrDie(csiClientConfig), | ||||||||||
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 |
---|---|---|
|
@@ -55,6 +55,7 @@ import ( | |
"k8s.io/client-go/tools/record" | ||
certutil "k8s.io/client-go/util/cert" | ||
"k8s.io/client-go/util/certificate" | ||
csiclientset "k8s.io/csi-api/pkg/client/clientset/versioned" | ||
"k8s.io/kubernetes/cmd/kubelet/app/options" | ||
"k8s.io/kubernetes/pkg/api/legacyscheme" | ||
api "k8s.io/kubernetes/pkg/apis/core" | ||
|
@@ -387,6 +388,7 @@ func UnsecuredDependencies(s *options.KubeletServer) (*kubelet.Dependencies, err | |
DockerClientConfig: dockerClientConfig, | ||
KubeClient: nil, | ||
HeartbeatClient: nil, | ||
CSIClient: nil, | ||
EventClient: nil, | ||
Mounter: mounter, | ||
OOMAdjuster: oom.NewOOMAdjuster(), | ||
|
@@ -607,6 +609,13 @@ func run(s *options.KubeletServer, kubeDeps *kubelet.Dependencies, stopCh <-chan | |
glog.Warningf("Failed to create API Server client for heartbeat: %v", err) | ||
} | ||
|
||
// csiClient works with CRDs that support json only | ||
clientConfig.ContentType = "application/json" | ||
csiClient, err := csiclientset.NewForConfig(clientConfig) | ||
if err != nil { | ||
glog.Warningf("Failed to create CSI API client: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What value does csiClient have at this point? It is referenced below. Typically one assumes one shouldn't use a value paired with a non-nil error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The pattern in this files seems to be just log it and use the value any way. I'm happy to change it for this to either return an error (fail fast) or nil the |
||
} | ||
|
||
kubeDeps.KubeClient = kubeClient | ||
if heartbeatClient != nil { | ||
kubeDeps.HeartbeatClient = heartbeatClient | ||
|
@@ -615,6 +624,7 @@ func run(s *options.KubeletServer, kubeDeps *kubelet.Dependencies, stopCh <-chan | |
if eventClient != nil { | ||
kubeDeps.EventClient = eventClient | ||
} | ||
kubeDeps.CSIClient = csiClient | ||
} | ||
|
||
// If the kubelet config controller is available, and dynamic config is enabled, start the config and status sync loops | ||
|
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.