search

samba-in-kubernetes / samba-operator

An operator for a Samba as a service on PVCs in kubernetes
Apache License 2.0
108 stars 24 forks source link

resources: avoid implicit sharing of volMount between slices #212

Closed synarete closed 2 years ago

synarete commented 2 years ago

In Go a slice is a fancy name for pointer and size, encapsulated within single sliceHeader struct[1]. When two (or more) distinct sliceHeaders refer to the same underlying array of objects, and modify it, the end result depends on the capacity of the original underlying array[2]. (Note that append may increase the capacity more then the number of elements added).

In our case, in buildADPodSpec function, joinVols refer to the same underlying array as smbAllVols and smbServerVols, and thus when appending it over-write values. This, in turn, causes a containers to have configurations with undefined behaviour.

Using a simple solution: duplicate volMount arrays so that each slice has its own (unique) copy. The performance penalty is minor.

[1] https://go.dev/blog/slices [2] https://go.dev/ref/spec#Appending_and_copying_slices

Signed-off-by: Shachar Sharon ssharon@redhat.com

Signed-off-by: Shachar Sharon ssharon@redhat.com

phlogistonjohn commented 2 years ago

Hrm. This makes me wish I had more time to push #179 more. I'll do a proper review as soon as I can.

synarete commented 2 years ago

Hrm. This makes me wish I had more time to push #179 more. I'll do a proper review as soon as I can.

Indeed, #179 looks cleaner and more comprehensive then my #212

phlogistonjohn commented 2 years ago

Hrm. This makes me wish I had more time to push #179 more. I'll do a proper review as soon as I can.

Indeed, #179 looks cleaner and more comprehensive then my #212

It also tries to do a lot more, so I'm happy to review/take this first. I'm just a little disappointed I didn't spend more time with the other PR so that we didn't have to retread the same path.

Looking at the code now.

phlogistonjohn commented 2 years ago

@spuiuk I'd like a 2nd set of eyes on this please.

synarete commented 2 years ago

Nothing jumps out at me as a problem, but I was the person who wrote the original slice-abusing version :-P I don't love the need to provide the extra capacity int values, and wouldn't like to keep it for the long term, but to fix bugs in the short term I think its fine.

@phlogistonjohn A version without the extra capacity int value is trivial -- would you like me to make this fix?

phlogistonjohn commented 2 years ago

Yeah, let's do that. I think it'll be simpler to understand & maintain.

synarete commented 2 years ago

Yeah, let's do that. I think it'll be simpler to understand & maintain.

ACK -- will do now

phlogistonjohn commented 2 years ago

/test centos-ci/sink-clustered/mini-k8s-1.23

spuiuk commented 2 years ago

Interesting problem which is difficult to spot by someone new to Go. Thanks for the explanation.

phlogistonjohn commented 2 years ago

/test centos-ci/sink-clustered/mini-k8s-1.23