fix: improve code and repair edge case problems with the internal PodSpec updates. #31
Conversation
hessjcg
commented
Sep 29, 2022
•
hessjcg
commented
- Significant rewrites for parts of podspec_updates.go to fix broken integration test edge cases
- When deleted, the AuthProxyWorkloads resource hangs around for a few minutes with its metadata.DeletionTimestamp set. In some cases, we need to ignore workloads that have already been deleted.
- Correctly determine if a port is in use or not. Fixes an edge case where the ConnectionString on an AuthProxyWorkload changes, but the port number remains the same.
- Correctly handle EnvVars for removed AuthProxyWorkloads
- Correctly handle Volumes and VolumeMounts for removed AuthProxyWorkloads
- Code cleanup from lint in most of the other files.
- Rename function params in podspec_updates.go for consistency
- Add the health check to the containers.
internal/names/names.go
Outdated
| i, err := h.Write(bytes) | ||
|
|
||
| if err != nil || i != len(bytes) { | ||
| panic(fmt.Errorf("unable to calculate hash for bytes %v %v", bytes, err)) |
There was a problem hiding this comment.
This should return an error. Panic is only for very very bad things.
There was a problem hiding this comment.
This is an error that should never happen though. So It seems that panic is ok here?
There was a problem hiding this comment.
In that case, shall we rename the function to mustHash?
internal/podspec_updates.go
Outdated
| @@ -145,15 +145,20 @@ func ReconcileWorkload(instList cloudsqlapi.AuthProxyWorkloadList, workload Work | |||
| // the workload. | |||
| func filterMatchingInstances(wl cloudsqlapi.AuthProxyWorkloadList, workload Workload) []*cloudsqlapi.AuthProxyWorkload { | |||
| matchingAuthProxyWorkloads := make([]*cloudsqlapi.AuthProxyWorkload, 0, len(wl.Items)) | |||
| for i, _ := range wl.Items { | |||
There was a problem hiding this comment.
Why not just take a reference to the item here instead of below?
for i, item := range wl.Items { /* ... */ }There was a problem hiding this comment.
I'm trying to avoid copying the csqlWorkload. This way I append a pointer to the object in wl.Items[i] rather than making a copy. I think the AuthProxyWorkload instance tips the scale into "pass a pointer" rather than the conventional "pass a value"
There was a problem hiding this comment.
Generally I prefer measuring first before making optimizations, but it's not really a big deal either way. Perhaps a comment explaining why you're not doing the usual thing here would be good.
There was a problem hiding this comment.
This isn't an optimization. I want matchingAuthProxyWorkloads to hold pointers to the AuthProxyWorkload in pl.Items.
This is a bug:
var matchingAuthProxyWorkloads []*AuthProxyWorkload
for _, item := range wl.Items {
matchingAuthProxyWorkloads = append(matchingAuthProxyWorkloads, &item)
}
because then every item in the slice matchingAuthProxyWorkloads holds a pointer to the same memory location: the location of &item. And item holds a copy the last value of the slice wl.Items. (I learned this the hard way.)
It could be this:
var matchingAuthProxyWorkloads []*AuthProxyWorkload
for _, item := range wl.Items {
matchingAuthProxyWorkloads = append(matchingAuthProxyWorkloads, item.DeepCopy())
}
But then I know it is always going to make two copies, which seems glutinous to me.
So this seems the best approach:
var matchingAuthProxyWorkloads []*AuthProxyWorkload
for i := range wl.Items {
matchingAuthProxyWorkloads = append(matchingAuthProxyWorkloads, &wl.Items[i])
}
So it really does need to be pass by reference.
There was a problem hiding this comment.
Should pl.Items be a slice of pointers in that case? This seems like an easy mistake to make otherwise.
hessjcg
force-pushed
the
fix-podspec-updates
branch
from
5e4691e to
a33b0eb
Compare
| "AuthProxyWorkload", csqlWorkload.Namespace+"/"+csqlWorkload.Name) | ||
| matchingAuthProxyWorkloads = append(matchingAuthProxyWorkloads, csqlWorkload) | ||
| func filterMatchingInstances(pl *cloudsqlapi.AuthProxyWorkloadList, wl Workload) []*cloudsqlapi.AuthProxyWorkload { | ||
| matchingAuthProxyWorkloads := make([]*cloudsqlapi.AuthProxyWorkload, 0, len(pl.Items)) |
There was a problem hiding this comment.
Are you worried about memory usage here? Would you mind briefly explaining why?
There was a problem hiding this comment.
I'm not as worried about memory usage as correctness. See comment above.
| // applyVolumeThings implements complex reconcile logic that is duplicated for both | ||
| // VolumeMount and Volume on containers. | ||
| func applyVolumeThings[T corev1.VolumeMount | corev1.Volume]( | ||
| s *updateState, |
There was a problem hiding this comment.
I appreciate the shorter line length, but if there are cases like this that feel a bit tedious, it's fine to just make the line what it will be. It's just a general concern about readability that enforces some length.
There was a problem hiding this comment.
Hope I wasn't the one who said something about this line.
There was a problem hiding this comment.
No, I was just viewing side-by-side and having a narrower window made life easier.
