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

test(ci): adding wrapper for vg and pv operations #317

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

abhilashshetty04
Copy link
Contributor

Why is this PR required? What issue does it fix?:
This PR adds required wrapper funcations for PV and VG operations. Its required for new tests to be automated

What this PR does?:
Same as above

Does this PR require any upgrade changes?:
NA

Checklist:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

@dsharma-dc
Copy link
Contributor

What is the rationale behind these wrappers? Sometime earlier we discussed that we'll leave these operations as it exists today, until we integrate to a common mechanism to manage lvm including Mayastor LVM backend.

@abhilashshetty04
Copy link
Contributor Author

What is the rationale behind these wrappers? Sometime earlier we discussed that we'll leave these operations as it exists today, until we integrate to a common mechanism to manage lvm including Mayastor LVM backend.

This is going to be used in the ci tests. We have scenarios where there is multiple vg in the node, there is a scenario to do vgextend ans see provisioning work.

@abhilashshetty04 abhilashshetty04 force-pushed the vgops_ci_wrapper branch 4 times, most recently from 1340667 to 73b136d Compare June 24, 2024 06:37
tests/utils.go Outdated Show resolved Hide resolved
tests/utils.go Outdated Show resolved Hide resolved
@sinhaashish
Copy link
Member

can you please squash the relevant commits. I see commits 856c95e83e12a4bdd2cb8d6a5e6b2a1e7ce80db3 and f1a328cd2758e5a8f52ca0aabde3abc46a91d73b are modifying the same file.

tests/utils.go Outdated
filename, "--show",
}

stdout_loop, _, err := execAtLocal("sudo", nil, args_loop...)
Copy link
Member

Choose a reason for hiding this comment

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

We can run the test using sudo, e.g.: https://github.com/openebs/dynamic-localpv-provisioner/blob/v3.5.x/Makefile#L146

If the test is using a shell script, e.g. test.sh, run it using sudo bash -c "./test.sh"

We should not call sudo implicitly like so.

Copy link
Contributor

Choose a reason for hiding this comment

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

The main ci script in lvm repo runs as non-root user in github actions. Most of the work done inside that script is doing sudo today e.g. sudo vgcreate. These new wrappers seem to be emulating same.

Copy link
Member

Choose a reason for hiding this comment

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

This feels like tech debt. But oh well.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could have a wrapper for calls which require sudo. In general IMHO it's better to use sudo only for operations which need it rather than blindly run everything with sudo as that seems a lot more dangerous.

tests/utils.go Show resolved Hide resolved
tests/provision_test.go Show resolved Hide resolved
@abhilashshetty04 abhilashshetty04 force-pushed the vgops_ci_wrapper branch 2 times, most recently from c01df96 to 52bcb45 Compare June 25, 2024 12:41
@abhilashshetty04 abhilashshetty04 merged commit d119d7f into develop Jun 25, 2024
10 checks passed
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