Adding basic volume syncing #137
Conversation
Adds syncing for basic volume types (secret/configmap/projected secret and configmap). Also changes the virtual kubelet to use a cache from controller-runtime rather than a client for some operations.
| // note: this needs to handle downward api volumes as well, but more thought is needed on how to do that | ||
| if volume.ConfigMap != nil { | ||
| if err := p.syncConfigmap(ctx, podNamespace, volume.ConfigMap.Name); err != nil { | ||
| return fmt.Errorf("unable to sync configmap volume %s: %w", volume.Name, err) |
There was a problem hiding this comment.
Not really sure what is the correct way here but since we are using zap all around maybe we can use zap.Fields instead of fmt.Errorf, better actually if the logger can be passed to the transformVolumes function and add the fields there so that it can actually appear what kind of resources are we working on, as an example:
https://github.com/rancher/k3k/blob/main/pkg/controller/cluster/pod.go#L65-L66
There was a problem hiding this comment.
I don't have a problem with things near log statement using zap, but I think that errors in the program shouldn't be coupled tightly to the logger in case we want to change it.
There was a problem hiding this comment.
There should be 1 way to convey information for readability and consistency. We lose both if there are more than 1 way to relay information. It should use the structured logger.
There was a problem hiding this comment.
Like I said above, I can definitely change this when logging, but when returning an error internally we should avoid reliance on unnecessary third party libraries. I can make a structured log statement here in addition to the error return if you would prefer.
k3k-kubelet/controller/configmap.go
Outdated
| return reconcile.Result{}, nil | ||
| } | ||
| var virtual corev1.ConfigMap | ||
| err := s.VirtualClient.Get(ctx, req.NamespacedName, &virtual) |
There was a problem hiding this comment.
This err Val should be scope locked to keep consistent with Go standards and the repo pattern already established.
k3k-kubelet/controller/configmap.go
Outdated
| Name: translated.Name, | ||
| } | ||
| var host corev1.ConfigMap | ||
| err = s.HostClient.Get(ctx, translatedKey, &host) |
There was a problem hiding this comment.
This err Val should be scope locked for the same reasons as above. Also, an if err nil checks should be done first and then operate on the err value conditionals.
k3k-kubelet/controller/configmap.go
Outdated
| for key, value := range translated.Labels { | ||
| host.Labels[key] = value | ||
| } | ||
| err = s.HostClient.Update(ctx, &host) |
There was a problem hiding this comment.
Scope lock the error value here and throughout the PR.
k3k-kubelet/controller/configmap.go
Outdated
|
|
||
| // Add adds a given resource to the list of resources that will be synced. Safe to call multiple times for the | ||
| // same resource. | ||
| func (s *ConfigMapSyncer) AddResource(ctx context.Context, namespace string, name string) error { |
There was a problem hiding this comment.
Remove the type declaration from the first string arg. It's not necessary since the second is a string.
k3k-kubelet/provider/provider.go
Outdated
| @@ -40,17 +51,31 @@ type Provider struct { | |||
| logger zap.SugaredLogger | |||
| } | |||
|
|
|||
| func New(config rest.Config, Namespace string, Name string) (*Provider, error) { | |||
| coreClient, err := cv1.NewForConfig(&config) | |||
| func New(hostConfig rest.Config, hostMgr manager.Manager, virtualMgr manager.Manager, Namespace string, Name string) (*Provider, error) { | |||
There was a problem hiding this comment.
Remove first type declaration for the manager.
k3k-kubelet/provider/provider.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("unable to sync volumes for pod %s/%s: %w", pod.Namespace, pod.Name, err) | ||
| } | ||
| p.logger.Infof("Creating pod %s/%s for pod %s/%s", tPod.Namespace, tPod.Name, pod.Namespace, pod.Name) |
There was a problem hiding this comment.
This should use positional refs.
| // note: this needs to handle downward api volumes as well, but more thought is needed on how to do that | ||
| if volume.ConfigMap != nil { | ||
| if err := p.syncConfigmap(ctx, podNamespace, volume.ConfigMap.Name); err != nil { | ||
| return fmt.Errorf("unable to sync configmap volume %s: %w", volume.Name, err) |
There was a problem hiding this comment.
There should be 1 way to convey information for readability and consistency. We lose both if there are more than 1 way to relay information. It should use the structured logger.
k3k-kubelet/provider/provider.go
Outdated
| // getSecretsAndConfigmaps retrieves a list of all secrets/configmaps that are in use by a given pod. Useful | ||
| // for removing/seeing which virtual cluster resources need to be in the host cluster. | ||
| func getSecretsAndConfigmaps(pod *corev1.Pod) ([]string, []string) { | ||
| secrets := []string{} |
There was a problem hiding this comment.
var secrets []string for here and below. This is the standard the repo is putting forth.
* Adding basic volume syncing Adds syncing for basic volume types (secret/configmap/projected secret and configmap). Also changes the virtual kubelet to use a cache from controller-runtime rather than a client for some operations.
Adds syncing for basic volume types (secret/configmap/projected secret and configmap). Also changes the virtual kubelet to use a cache from controller-runtime rather than a client for some operations.
Related to #115.