Conversation
pkg/storageos/configmap.go
Outdated
| return fmt.Errorf("failed to get storageos status: %v", err) | ||
| } | ||
| if status.Phase != storageosv1.ClusterPhaseRunning { | ||
| return fmt.Errorf("storageos api not ready, retrying") |
There was a problem hiding this comment.
errors.New does the same but in more efficient way.
| configmap := &corev1.ConfigMap{} | ||
| err := cm.client.Get(context.TODO(), cm.NamespacedName, configmap) | ||
| return configmap, err | ||
| if err := cm.client.Get(context.TODO(), cm.NamespacedName, configmap); err != nil { |
There was a problem hiding this comment.
I know it is out of the scope of the change, but are we able to replace TODO or we really don't know what to do with context here?
There was a problem hiding this comment.
Yes, it would be better to pass in a proper context, but this is a wider change to do consistently. The k8s client function signature changed and this is in keeping with the existing code. I'd prefer to leave as-is for now and we can do a follow-on PR if needed.
test/e2e/clusterCSINodeV2_test.go
Outdated
| if logErr != nil { | ||
| t.Error(errors.Wrap(logErr, "failed to fetch operator logs")) | ||
| } | ||
| t.Log(logs) |
There was a problem hiding this comment.
I suggest to Log first and than check logErr, because for me looks more readable if Error and Fatal appear at the end
There was a problem hiding this comment.
I need to leave the Fatal at the end as it exits, but I'm re-arranged to try and make it more readable.
There was a problem hiding this comment.
perfect i just say:
t.Log(logs)
if logErr != nil t.Error(errors.Wrap(logErr, "failed to fetch operator logs"))
t.Fatal(err)
Because Error doesn't stops execution.
internal/pkg/storageos/client.go
Outdated
| Timeout: HTTPTimeout, | ||
| Transport: transport, | ||
| } | ||
| config.HTTPClient = httpc |
There was a problem hiding this comment.
Is there a reason that you assign &http.Client{} to httpc first?
| func (s *Deployment) ensureConfigMap() error { | ||
| config := configFromSpec(s.stos.Spec, CSIV1Supported(s.k8sVersion)) | ||
|
|
||
| labels := make(map[string]string) |
There was a problem hiding this comment.
What's going on here?
It looks like the call to s.k8sResourceManager.ConfigMap(configmapName, s.stos.Spec.GetResourceNS(), labels, config).Get() calls ResourceManager.combineLabels() which creates a new map and combines the ResourceManager.Labels with the labels that are passed in.
Would passing in nil risk causing an explosion later because we'd instantiate r with a nil map for it's labels?
There was a problem hiding this comment.
Should be just fine to pass nil. The checks are handled internally.
There was a problem hiding this comment.
No, combineLabels checks for nil and returns r.labels, which is never nil since it's set in the constructor. So passing in nil is ok, and it would be safe to remove labels := make(map[string]string). I'd prefer to keep it as I think it makes it more clear that we don't want to set specific labels.
pkg/storageos/configmap.go
Outdated
| // dynamically via the API. | ||
| if existing != nil { | ||
| // Don't update if unchanged. | ||
| if reflect.DeepEqual(config, existing) { |
There was a problem hiding this comment.
How can a map[string]string and *v1.ConfigMap ever be deeply equal? Would you not need to turn the configMap into a map in order to compare them?
There was a problem hiding this comment.
doh! Very well spotted. It needs to compare with existing.Data. I'm adding a unit test for the function...
There was a problem hiding this comment.
So that test became a bit more involved than I expected...
775765d to
937b81e
Compare
darkowlzz
left a comment
There was a problem hiding this comment.
LGTM!
A lot of detailed tests. Thanks.
Previously the StorageOSCluster CR was treated as immutable. With this change, a user can modify settings in the CR spec and those changes will get passed to StorageOS.
Some configuration items at set via an API call to the StorageOS control plane API. Others are set in a ConfigMap that is passed to the node container as env vars. Items that are configured via the API are also set in the ConfigMap once they have been successfully applied.
The control plane determines which configuration items can be updated at run-time, with a restart, or not at all.