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

Add storage format #463

Merged
merged 2 commits into from
Apr 4, 2022
Merged

Add storage format #463

merged 2 commits into from
Apr 4, 2022

Conversation

didib
Copy link
Member

@didib didib commented Mar 29, 2022 •
edited by sandrobonazzola
Loading

Allow passing storage_format as is allowed by the Python SDK.

Bug-Url: https://bugzilla.redhat.com/1932147

@didib didib force-pushed the add-storage-format branch 2 times, most recently from 85a4749 to 5879f08 Compare March 29, 2022 11:51
sandrobonazzola
sandrobonazzola approved these changes Mar 29, 2022
View reviewed changes
Copy link
Member

@sandrobonazzola sandrobonazzola left a comment

Choose a reason for hiding this comment

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

LGTM but I would prefer some more people to review this one

arachmani
arachmani reviewed Mar 29, 2022
View reviewed changes
Copy link
Member

@arachmani arachmani left a comment

Choose a reason for hiding this comment

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

The code LGTM, can you please add a changelog?

@didib
Copy link
Member Author

didib commented Mar 31, 2022

Verified in [1] with [2] and current.

The commit messages in [2] are not very up to date - to get to this success, I also had to put host-0 to maintenance and remove it, prior to taking the backup.

Without current, something like [1] failed with the error message in the bug. With it, HE deploy succeeded. [1] failed later on adding host-1 (because it already exists).

So current PR can be merged as-is. If you prefer to not include the middle patch, I don't mind removing it, or you can remove it too if you want.

[1] https://redir.apps.ovirt.org/dj/job/ds-ost-baremetal_manual/32505

[2] oVirt/ovirt-system-tests#93

@arachmani
Copy link
Member

So current PR can be merged as-is. If you prefer to not include the middle patch, I don't mind removing it, or you can remove it too if you want.

The middle commit contains code that looks similar to create_storage_domain.yml.
It's also not that trivial how to use it, I think we should remove it or perhaps combine them and add an explanation.

@didib
Copy link
Member Author

didib commented Apr 4, 2022

The middle commit contains code that looks similar to create_storage_domain.yml. It's also not that trivial how to use it, I think we should remove it or perhaps combine them and add an explanation.

I'd remove it.

@didib
ovirt_storage_domain: Support custom storage format
df0b554 
Allow passing storage_format as is allowed by the Python SDK.

Bug-Url: https://bugzilla.redhat.com/1932147
@didib
roles: hosted_engine_setup: Pass storage format
c893653 
- Check the version of the cluster we are adding the host to

- Fail if it's before 4.2. AFAICT the engine also does not support older
storage domains - see [1] and other changes to
VersionStorageFormatUtil.java .

- Set storage_format to v4 if it's 4.2, otherwise to v5

[1] oVirt/ovirt-engine@373c3f8e
@didib
Copy link
Member Author

didib commented Apr 4, 2022

Ran OST, it passed HE deploy and failed later like before, as expected. So looks good to me, can be merged.

https://redir.apps.ovirt.org/dj/job/ds-ost-baremetal_manual/32907

arachmani
arachmani approved these changes Apr 4, 2022
View reviewed changes
Copy link
Member

@arachmani arachmani left a comment

Choose a reason for hiding this comment

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

LGTM

@arachmani arachmani merged commit e242c76 into oVirt:master Apr 4, 2022
mnecas pushed a commit to mnecas/ovirt-ansible-collection that referenced this pull request Jun 13, 2022
@didib @mnecas
Add storage format (oVirt#463)
2ce958e 
Allow passing storage_format as is allowed by the Python SDK.
 
* roles: hosted_engine_setup: Pass storage format

- Check the version of the cluster we are adding the host to
- Fail if it's before 4.2. AFAICT the engine also does not support older
storage domains - see [1] and other changes to
VersionStorageFormatUtil.java .
- Set storage_format to v4 if it's 4.2, otherwise to v5

Bug-Url: https://bugzilla.redhat.com/1932147
[1] oVirt/ovirt-engine@373c3f8e
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandrobonazzola sandrobonazzola sandrobonazzola approved these changes

@arachmani arachmani arachmani approved these changes

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

@ahadas ahadas Awaiting requested review from ahadas

@bennyz bennyz Awaiting requested review from bennyz

@nirs nirs Awaiting requested review from nirs

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

Successfully merging this pull request may close these issues.
3 participants
@didib @arachmani @sandrobonazzola