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

Add colocation and ordering for clvm #373

Merged
merged 3 commits into from
May 16, 2024
Merged

Conversation

tahliar
Copy link
Collaborator

@tahliar tahliar commented May 2, 2024

PR creator: Description

Added a step to make sure VGs are only activated after DLM is already running.

PR creator: Are there any relevant issues/feature requests?

  • bsc#1222440
  • jsc#DOCTEAM-1357

PR creator: Which product versions do the changes apply to?

When opening a PR, check all versions of the documentation that your PR applies to.

  • SLE-HA 15
    • 15 next (current main, no backport necessary)
    • 15 SP5
    • 15 SP4
    • 15 SP3
    • 15 SP2
    • 15 SP1
  • SLE-HA 12
    • 12 SP5
    • 12 SP4

PR reviewer only: Have all backports been applied?

The doc team member merging your PR will take care of backporting to older documents.
When opening a PR, do not set the following check box.

  • all necessary backports are done

@tahliar
Copy link
Collaborator Author

tahliar commented May 2, 2024

Step 3 (point 2) had a command to add the resource to the DLM group, but I've moved it to the newly added step 4:

[removed outdated image]

bsc#1222440
jsc#DOCTEAM-1357
@tahliar tahliar force-pushed the DOCTEAM-1357-lvm-colocation branch from 8fb3f09 to 148788f Compare May 2, 2024 05:16
Copy link
Contributor

@gao-yan gao-yan left a comment

Choose a reason for hiding this comment

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

LGTM. Only one nitpicking

<listitem>
<para>
For multiple VGs using either activation mode, you can add constraints
that apply to batches of resources:
Copy link
Contributor

@gao-yan gao-yan May 3, 2024

Choose a reason for hiding this comment

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

The example in below looks like it's exclusive activation mode, given that vg1 and vg2 are not explicitly cloned here. How about being more clear, for example something like:

"For multiple VGs, you can add constraints that apply to batches of resource. For instance for exclusive activation mode:
..."

And after the example:
"For multiple VGs using shared activation mode, you can first clone vg1 and vg2, and add similar constraints that apply to the clone resources instead".

Just an idea, I believe there are other ways to clarify it :-)

Copy link

Choose a reason for hiding this comment

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

Cool. Thx.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, thank you, I didn't catch that because the original procedure didn't have a step for cloning, but that's because it added the shared mode VG to the group, and the group is cloned. So of course without the group you have to clone. I'll add another dot point to address this.

@tahliar
Copy link
Collaborator Author

tahliar commented May 8, 2024

@gao-yan @jirib Here's the updated procedure:

[outdated]
image

@jirib
Copy link

jirib commented May 9, 2024

[Updated after rethinking it a bit] Shared activated VG means it is active on all nodes; thus, same as for DLM, they should be part of g-storage; using colocation instead of just a group is useful - iiuc - only if multiple VGs are present, so one does not create dependency, similar as for OCFS2 at https://documentation.suse.com/sle-ha/15-SP5/html/SLE-HA-all/cha-ha-ocfs2.html#pro-ocfs2-mount-multiple .

IMO, there should be to unordered items - shared activation and exclusive one, plus an example for one VG and multiple VGs.

BTW, I am not sure but isn't "batches of resources" what Pacemaker calls "resource sets"? @gao-yan ?

@zzhou1
Copy link
Contributor

zzhou1 commented May 10, 2024

[Updated after rethinking it a bit] Shared activated VG means it is active on all nodes; thus, same as for DLM, they should be part of g-storage; using colocation instead of just a group is useful - iiuc - only if multiple VGs are present, so one does not create dependency, similar as for OCFS2 at https://documentation.suse.com/sle-ha/15-SP5/html/SLE-HA-all/cha-ha-ocfs2.html#pro-ocfs2-mount-multiple .

agree.

IMO, there should be to unordered items - shared activation and exclusive one, plus an example for one VG and multiple VGs.

I don't think I fully understand your sentence above. The resource set in "()" does indicate the constraints configuration for "sequential=false" and "require-all=true" internally.

BTW, I am not sure but isn't "batches of resources" what Pacemaker calls "resource sets"? @gao-yan ?

Yes, more precisely the parentheses resource set in crmsh syntax ;)

@tahliar
Copy link
Collaborator Author

tahliar commented May 13, 2024

IMO, there should be to unordered items - shared activation and exclusive one, plus an example for one VG and multiple VGs.

Good idea, I'll update it to do so.

BTW, I am not sure but isn't "batches of resources" what Pacemaker calls "resource sets"? @gao-yan ?

Funnily enough, in hindsight I have no idea why I said "batches of resources" instead of just "multiple resources" 🤔

@tahliar
Copy link
Collaborator Author

tahliar commented May 13, 2024

@jirib @zzhou1 @gao-yan I've updated again, hopefully all good now:

image

@zzhou1
Copy link
Contributor

zzhou1 commented May 13, 2024

Ah, great, now, I understand Jiri's previous comments, thank you Thalia ! ps, My English should improve from this ;)

@gao-yan
Copy link
Contributor

gao-yan commented May 13, 2024

@jirib @zzhou1 @gao-yan I've updated again, hopefully all good now:

Thanks, @tahliar . It looks even more clear now.

IMO, there should be to unordered items - shared activation and exclusive one, plus an example for one VG and multiple VGs.

I don't think I fully understand your sentence above. The resource set in "()" does indicate the constraints configuration for "sequential=false" and "require-all=true" internally.

BTW, I am not sure but isn't "batches of resources" what Pacemaker calls "resource sets"? @gao-yan ?

Yes, more precisely the parentheses resource set in crmsh syntax ;)

Yes, the terminology is "resource set".

We demonstrated a type of resource set which implies sequential="true":

https://susedoc.github.io/doc-sleha/main/single-html/SLE-HA-administration/#sec-ha-config-basics-constraints-rscset-constraints

In here it's a different type which basically has sequential=false as Roger pointed out.

@tahliar tahliar requested a review from dariavladykina May 14, 2024 03:39
Copy link
Contributor

@dariavladykina dariavladykina left a comment

Choose a reason for hiding this comment

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

LGTM.

@tahliar tahliar merged commit 7823e93 into main May 16, 2024
4 checks passed
@tahliar tahliar deleted the DOCTEAM-1357-lvm-colocation branch May 16, 2024 00:35
tahliar added a commit that referenced this pull request May 16, 2024
* Add colocation and ordering for clvm

bsc#1222440
jsc#DOCTEAM-1357

* Apply tech review feedback

* Restructure step 4
tahliar added a commit that referenced this pull request May 16, 2024
* Add colocation and ordering for clvm

bsc#1222440
jsc#DOCTEAM-1357

* Apply tech review feedback

* Restructure step 4
tahliar added a commit that referenced this pull request May 16, 2024
* Add colocation and ordering for clvm

bsc#1222440
jsc#DOCTEAM-1357

* Apply tech review feedback

* Restructure step 4
tahliar added a commit that referenced this pull request May 16, 2024
* Add colocation and ordering for clvm

bsc#1222440
jsc#DOCTEAM-1357

* Apply tech review feedback

* Restructure step 4
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.

5 participants