Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Flag enabling SplitBrain #465

Merged
merged 1 commit into from
May 5, 2021
Merged

Flag enabling SplitBrain #465

merged 1 commit into from
May 5, 2021

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented May 3, 2021 •
edited
Loading

SplitBrainCheck flag; If false (default) splitBrain is disabled, no metter what EdgeDNSType you use.
To enable SplitBrain, set SPLIT_BRAIN_CHECK to true

  • I added new field to depresolver Config (SplitBrainCheck) and introduced new env var SPLIT_BRAIN_CHECK.
  • I updated infoblox provider to skip TXT interractions when !SplitBrainCheck
  • Me & k0da made e2e tests against test clusters
  • Fix heartbeat issue

Signed-off-by: kuritka [email protected]

k0da
k0da previously approved these changes May 3, 2021
View reviewed changes
Copy link
Collaborator

@k0da k0da left a comment

Choose a reason for hiding this comment

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

LGTM

somaritane
somaritane suggested changes May 3, 2021
View reviewed changes
Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

@kuritka , looks good, just a few suggestions on logging and feature flag naming

@kuritka kuritka force-pushed the SplitBrainThresholdSeconds_switch branch 2 times, most recently from f106459 to cc5681e Compare May 4, 2021 08:33
@kuritka kuritka requested a review from somaritane May 4, 2021 10:09
@kuritka kuritka force-pushed the SplitBrainThresholdSeconds_switch branch from 53fe738 to 959fc36 Compare May 4, 2021 16:07
@kuritka
Copy link
Collaborator Author

kuritka commented May 4, 2021

@somaritane @ytsarev @k0da
pushed,

  • e2e tests with @k0da
  • with k0da we also fixed the heartbeat issue [1] [2]

k0da
k0da requested changes May 4, 2021
View reviewed changes
chart/k8gb/templates/operator.yaml Outdated
@@ -121,3 +121,5 @@ spec:
value: {{ quote .Values.k8gb.log.level }}
- name: NO_COLOR
value: "true"
- name: SPLIT_BRAIN_PREVENTION
value: {{ quote .Values.k8gb.noSplitBrain }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Chart value name is contraversial with env.

I suggest: noSplitBrainDetection

Copy link
Collaborator

Choose a reason for hiding this comment

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

or diableSplitBrainDetection

Copy link
Contributor

Choose a reason for hiding this comment

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

@k0da +1, no sure though why we need to use no- or disable- prefix for variables, we can simply set the default value to false, like already done here: https://github.com/AbsaOSS/k8gb/pull/465/files#diff-b94e4b6c67fca17dce52708f765679ec46a8553531b432d30b8da1b11b1bc89cR84.
I'm also fine with either splitBrainDetection or splitBrainPrevention, it's just should be named consistently over the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

Can we have just splitBrain true/false?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ytsarev, as I mentioned earlier, split-brain is the name of the issue happening in the distributed system. So if we tell that we want to enable or disable the split-brain - this looks exactly confusing from user point of view

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more correct to tell that we want to enable or disable the split-brain detection or prevention feature. We don't want to enable the problem, we want to enable the solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

splitBrainCheck true/false

Copy link
Collaborator Author

@kuritka kuritka May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@somaritane, @k0da
SPLIT_BRAIN_CHECK

  • default value (empty or not set) false
  • invalid value - false
  • true, t, 1,T ,True, ... - enables split-brain otherwise it's disabled

If we do it this way, then enabling split-brain will require to set SPLIT_BRAIN_CHECK to true on each k8gb you want to have split-brain, otherwise it will be disabled by default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@kuritka looks good 👍 , let's also update the PR title and body for consistency with amendments.

ytsarev
ytsarev requested changes May 4, 2021
View reviewed changes
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

Consider making splitbrain 'enabling' semantics instead of prevention. It will be much more obvious for end user

@kuritka kuritka force-pushed the SplitBrainThresholdSeconds_switch branch from 959fc36 to 92045ae Compare May 4, 2021 19:50
@kuritka
Copy link
Collaborator Author

kuritka commented May 4, 2021

Consider making splitbrain 'enabling' semantics instead of prevention. It will be much more obvious for end user

yes I see - with default value true, but what if value is invalid, also true? disabling semantics is much easier in this.

@ytsarev
Copy link
Member

ytsarev commented May 5, 2021

Consider making splitbrain 'enabling' semantics instead of prevention. It will be much more obvious for end user

yes I see - with default value true, but what if value is invalid, also true? disabling semantics is much easier in this.

Ok, https://github.com/AbsaOSS/k8gb/pull/465#discussion_r626320501 proposal looks fine

@kuritka kuritka force-pushed the SplitBrainThresholdSeconds_switch branch from 92045ae to 36e3c2c Compare May 5, 2021 08:57
k0da
k0da previously approved these changes May 5, 2021
View reviewed changes
Copy link
Collaborator

@k0da k0da left a comment

Choose a reason for hiding this comment

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

LGTM

@kuritka kuritka changed the title Flag disabling SplitBrain Flag enabling SplitBrain May 5, 2021
@kuritka
Flag enabling SplitBrain
19208ac 
**SplitBrainCheck flag**; If false (default) splitBrain is disabled, no metter what EdgeDNSType you use.
To enable SplitBrain, set `SPLIT_BRAIN_CHECK` to true

- I added new field to depresolver Config (SplitBrainCheck) and introduced new env var `SPLIT_BRAIN_CHECK`.
- I updated infoblox provider to skip TXT interractions when !NoSplitBrain
- Me & k0da made e2e tests against test clusters
- Fix heartbeat issue

Signed-off-by: kuritka <[email protected]>
@kuritka kuritka force-pushed the SplitBrainThresholdSeconds_switch branch from d59fdd0 to 19208ac Compare May 5, 2021 09:42
somaritane
somaritane approved these changes May 5, 2021
View reviewed changes
Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

@kuritka LGTM 👍

@kuritka kuritka merged commit baa8db7 into master May 5, 2021
@kuritka kuritka deleted the SplitBrainThresholdSeconds_switch branch May 5, 2021 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@somaritane somaritane somaritane approved these changes

@ytsarev ytsarev ytsarev approved these changes

@k0da k0da k0da left review comments

@donovanmuller donovanmuller Awaiting requested review from donovanmuller donovanmuller is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kuritka @ytsarev @somaritane @k0da