Skip to content

Refactor ChiStatus internals to make concurrent access safe#1119

Merged
sunsingerus merged 1 commit intoAltinity:0.21.0from
zcross:zcross/concurrency/status
Apr 24, 2023
Merged

Refactor ChiStatus internals to make concurrent access safe#1119
sunsingerus merged 1 commit intoAltinity:0.21.0from
zcross:zcross/concurrency/status

Conversation

@zcross
Copy link
Copy Markdown
Contributor

@zcross zcross commented Mar 2, 2023 •
edited
Loading

Background: #1109
Related PR(s):

This PR adds sync.RWMutex based synchronization over the mutable state of ChiStatus. It should be relatively straightforward. (Also, around the initialization of ClickHouseInstallation's own status field via EnsureStatus).

The most eyebrow raising bits are probably in the sequenced lock acquisition in MergeActions and CopyFrom creating some risk of deadlocking (to be discussed, I'm sure). The only other potentially "hairy" concern is some of the generated code I've found for DeepCopy helpers on all types.

Meta: I began this by trying to define a mediator through which all updates would be made. However, the presence of logic inside some of the other raw types inside of the v1 package (such as the ClickHouseInstallation AKA chi) made it apparent to me that it's not that much cleaner to separate the two. My workaround to this was to at least make sure that all field level read and write accesses to the ChiStatus struct were strictly within its single source file (type_status) – with the exception of the deepcopy generated files (which don't seem to be used directly AFAIK, and hopefully aren't used at runtime from potentially competing goroutines).

Note: I'll squash all commits from feedback pre-merge.

Important items to consider before making a Pull Request

Please check items PR complies to:

  • All commits in the PR are squashed. More info
  • The PR is made into dedicated next-release branch, not into master branch1. More info
  • The PR is signed. More info

@zcross zcross mentioned this pull request Mar 2, 2023
Merged
3 tasks
@zcross
Copy link
Copy Markdown
Contributor Author

zcross commented Mar 2, 2023

FYI @sunsingerus – apologies for closing the first attempt at this ChiStatus refactor, but I force pushed it without realizing that would prevent re-opening.

@zcross
Copy link
Copy Markdown
Contributor Author

zcross commented Mar 3, 2023

Hmm, I just began a branch to refactor Announcer (and its variations like chi.Announcer and the others). However, It seems to me like this code is all already goroutine safe as all of the "fluent style" (chained) function calls operate on and return struct values rather than pointers, i.e., beginning with a copy by value before mutating any state:

b := a
b.SomeField = newLogCallSpecificValue
return b

Seem like a correct assessment? If so, there won't really be an "announcer PR" – I'll be turning to a final (third) PR to actually update reconciliation logic (well, returning to my original draft, basing it off of this and the registry branches, and then cleaning it up for PR-readiness)

zcross
zcross commented Mar 6, 2023
View reviewed changes
Comment thread pkg/apis/clickhouse.altinity.com/v1/type_chi.go

// FillStatus fills .Status
func (chi *ClickHouseInstallation) FillStatus(endpoint string, pods, fqdns []string, ip string) {
chi.EnsureStatus().CHOpVersion = version.Version
Copy link
Copy Markdown
Contributor Author

@zcross zcross Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version is always statically available so these 3 field setter calls were pushed within the new Fill function (and therefore absent from the passed params)

@zcross zcross mentioned this pull request Mar 6, 2023
Merged
3 tasks
@sunsingerus
Copy link
Copy Markdown
Collaborator

sunsingerus commented Apr 14, 2023

Code-generator produces errors on generics introduced in this PR. This issue blocks merge atm and needed to be looked into.

@sunsingerus
Copy link
Copy Markdown
Collaborator

sunsingerus commented Apr 14, 2023

@zcross merge is blocked atm until further understanding on code-generator + generics situation.

@zcross
Copy link
Copy Markdown
Contributor Author

zcross commented Apr 14, 2023

Ok, ACK that I saw this and will take a shot at cleaning it up next week. Re: generics, I'll just replace them with a few type specific functions as needed for simplicity. BTW: how can I reproduce the problem you're seeing for my own iteration? (E.g., what make/build command are you running) Thanks!

@sunsingerus
Copy link
Copy Markdown
Collaborator

sunsingerus commented Apr 14, 2023

clientset, informer and other code is generated with ./dev/run_code_generator.sh command

clickhouse-operator$ ./dev/run_code_generator.sh 
...
Generating clientset for clickhouse.altinity.com:v1 at github.com/altinity/clickhouse-operator/pkg/client/clientset
W0414 17:07:52.225131 2075270 parse.go:818] Making unsupported type entry "T" for: &types.TypeParam{check:(*types.Checker)(nil), id:0x2, obj:(*types.TypeName)(0xc006262780), index:0, bound:(*types.Interface)(0xc0000bcaa0)}
...

@zcross
Copy link
Copy Markdown
Contributor Author

zcross commented Apr 18, 2023

Sorry for not getting to this until today, but I just:

  • Resolved merge conflicts (fairly straightforward)
  • Replaced generics (which I didn't notice were the first generic usage in this repo) with type specific equivalents – only string/int/installation pointer types were needed so no big deal

@sunsingerus sunsingerus merged commit ca83371 into Altinity:0.21.0 Apr 24, 2023
@zcross zcross mentioned this pull request May 9, 2023
Closed
@zcross zcross mentioned this pull request Jul 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zcross @sunsingerus