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.

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

Fixes for DRBD >= 8.4. #1496

Merged
merged 1 commit into from Aug 14, 2020
Merged

Fixes for DRBD >= 8.4. #1496

merged 1 commit into from Aug 14, 2020

Conversation

mbakke
Copy link
Contributor

@mbakke mbakke commented Jul 2, 2020

This is a cleaned-up version of #1296, taking the comments from @apoikos into account. I have tested this with drbd-utils 9.13.1 and kernel module version 8.4.11 (Linux 5.7.6).

More testing very welcome!

Cc @vladimir-ipatov

lib/storage/drbd.py Outdated Show resolved Hide resolved
@rbott
Copy link
Member

rbott commented Jul 3, 2020

I have successfully tested this with a full DRBD cluster QA run on Debian Buster (DRBD 8.4.10, drbd-utils 9.5.0) and Ubuntu Focal Fossa (DRBD 8.4.11, drbd-utils 9.11.0). I will also run this through Debian Bullseye tomorrow.

However, the error handling the in the retry code seems wrong, but that is already an open discussion/code comment:

ganeti/lib/storage/drbd.py

Lines 407 to 421 in b5663b4

def _WaitForMinorSyncParams():
"""Call _SetMinorSyncParams and raise RetryAgain on errors.
"""
if self._SetMinorSyncParams(minor, self.params):
raise utils.RetryAgain()
else:
return []
if self._NeedsLocalSyncerParams():
# Retry because disk config for DRBD resource may be still uninitialized.
try:
utils.Retry(_WaitForMinorSyncParams, 1.0, 5.0)
except utils.RetryTimeout:
base.ThrowError("drbd%d: can't set the synchronization parameters: %s" %
(minor, utils.CommaJoin(sync_errors)))

We need to sort this out but other than that, LGTM.

Add drbdsetup new-device and new-mirror for network part assembling of
drbd (needed for diskless states of drbd), relocate syncer parameters
setting to local part (in 8.4 syncer require disk-config).

Co-authored-by: Marius Bakke <marius@devup.no>
@mbakke
Copy link
Contributor Author

mbakke commented Jul 11, 2020

@rbott thanks a lot for reviewing, I think the error handling issue has been resolved with the latest revision.

@apoikos apoikos self-requested a review July 12, 2020 09:48
Copy link
Member

@apoikos apoikos left a comment

Choose a reason for hiding this comment

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

Thanks to both, @vladimir-ipatov and @mbakke!

In principle it looks good, however I'd like to link this PR to specific issues. Given that DRBD 8.4 generally works without this PR, what is it exactly that we are fixing here? Neither this PR, nor #1296 point to specific issues, and we need that rationale at least for the changelog. I'm not saying these changes are unnecessary, I just want a documented justification for them :)

# For DRBD >= 8.4, syncer init must be done after local, not in net.
info = DRBD8.GetProcInfo()
version = info.GetVersion()
return version["k_minor"] >= 4
Copy link
Member

Choose a reason for hiding this comment

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

Additionally checking that version["k_major"] == 8 would make the code more robust.

@vladimir-ipatov
Copy link
Contributor

Thanks to both, @vladimir-ipatov and @mbakke!

In principle it looks good, however I'd like to link this PR to specific issues. Given that DRBD 8.4 generally works without this PR, what is it exactly that we are fixing here? Neither this PR, nor #1296 point to specific issues, and we need that rationale at least for the changelog. I'm not saying these changes are unnecessary, I just want a documented justification for them :)

Hello!

Main issue is:
Without this fixes ganeti with drbd 8.4 will not assemble drbd device if one of two nodes don't have lvm volumes for it (diskless).
According to my information, there is no issues on github about it.

It was mentioned in my pull request description (but may be uncertainly :) )"
"fixes for drbd >= 8.4: add drbdsetup new-device and new-mirror for network part assembling of drbd (needed for diskless states of drbd)"

May be there is some other minor issues, I don't remember.

@apoikos apoikos merged commit e929665 into ganeti:master Aug 14, 2020
@apoikos
Copy link
Member

apoikos commented Aug 14, 2020

Merged, thanks to both of you!

@mbakke mbakke deleted the drbd84 branch August 17, 2020 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants